Preliminary trace recording. #3

vext01 · 2019-01-10T16:15:41Z

This shows that we can store MIR locations into a thread local vector at runtime.

It's a quick and dirty implementation that disables tracing for a bunch of crates that ideally we probably do want to trace at some point. As discussed on IRC, we are going to live with this compromise until a later date, where we might (e.g.) implement the trace recorder in another language that the MIR pass can't see.

I've added some tests of our own too.

I've run the smoke test subset of the rustc tests at stage one and fixed a few failures. Let's see what happens in a full run.

Any comments in the meantime?

vext01 · 2019-01-10T16:15:50Z

bors try

ltratt · 2019-01-10T16:52:30Z

src/librustc_mir/transform/add_yk_swt_calls.rs

@@ -18,6 +18,11 @@ use rustc::hir;
 use rustc::hir::def_id::{DefIndex, LOCAL_CRATE};
 use rustc::hir::map::blocks::FnLikeNode;

+// Crates the trace recorder depends upon.
+// This is effectively libstd and its dependencies (from its Cargo manifest).
+static RECORDER_DEPS: &'static [&'static str] = &["std", "alloc", "panic_unwind",


I think it might be clearer to call this something like CRATES_TRACE_BLACKLIST: in a sense, the fact that the crate is a dependency is less relevant than that we're going to avoid tracing it?

ltratt · 2019-01-10T16:53:37Z

src/libstd/yk_swt.rs

+    pub bb_idx: u32,
+}
+
+impl fmt::Debug for MirLoc {


Is it worth this manual code vs. #[derive(Debug)]?

The reason I did a custom one was because the derived one is quite long, with the names of the fields etc.

I wanted a shorter representation.

OK, but let's only do this sparingly, just because it's that bit more code to read and maintain.

ltratt · 2019-01-10T16:58:06Z

src/libstd/yk_swt.rs

+}
+
+// FIXME Anything used in `rec_loc` below cannot itself be traced, or we get infinite recursion. To
+// work sround this, many crates are ignored by the software tracing MIR pass (see


ltratt · 2019-01-10T16:58:22Z

src/libstd/yk_swt.rs

+        let mut trace_o = rc.borrow_mut();
+        match trace_o.as_mut() {
+            Some(trace) => trace.push(MirLoc{crate_hash, def_idx, bb_idx}),
+            None => (), // Tracing is disabled, do nothing.


"Disabled" or "not active"?

Both I guess. How about:

"We are not currently tracing, do nothing"

We are only resolving threads when I have pushed the fixes, right? This one was marked resolved.

We had resolved the discussion (and I trust you to do what you'd suggested you would do). No point having things hanging around in such a case I think?

Ah I see. This isn't quite what I thought. The problem with this strategy is that I have to go unfolding threads to see if I've addressed them or not (in code). I thought you would only resolve them when you saw the commit that addresses them.

But that might also be sub-optimal?

ltratt · 2019-01-10T16:59:45Z

src/libstd/yk_swt.rs

+#[cfg_attr(not(stage0), no_trace)]
+pub fn stop_tracing() -> Vec<MirLoc> {
+    TRACE.with(|rc| {
+        let trace_o = rc.borrow_mut().take();


Can't this be something like:

rc.borrow_mut().take.expect("tracing not started on this thread")

?

I think it could, yeah. Thanks.

ltratt · 2019-01-10T17:00:23Z

src/test/run-pass/yk_swt/collect-trace.rs

+
+#[inline(never)]
+fn work() -> u64{
+    let mut res = 47;


Any reason for the unusual numbers?

Can I suggest we choose less weird numbers? Otherwise it makes me think there's something specific about these (perhaps some threshold needs to be exceeded). I am a big fan of numbers < 10, or multiples of 10 ;)

Can do, yeah.

vext01 · 2019-01-10T17:10:14Z

Thanks for the comments. I'm going to wait for buildbot before applying changes.

ltratt · 2019-01-10T17:11:59Z

You can, I think, push changes even before buildbot has finished without affecting it. But it's up to you -- I'm easy either way!

vext01 · 2019-01-10T17:13:16Z

I configured it to run a build for each push. So perhaps I'll make the changes locally and wait for the push.

ltratt · 2019-01-10T17:18:47Z

I configured it to run a build for each push. So perhaps I'll make the changes locally and wait for the push.

Dumb question: what's the point of manually doing "try" then? I assumed we had to manually kick it to get it to do anything...

vext01 · 2019-01-10T17:21:46Z

You are right, I think I must be wrong.

bors · 2019-01-10T17:25:45Z

try

Build failed

buildbot/buildbot-build-script

vext01 · 2019-01-10T18:02:53Z

I'll start looking at the test failures first thing tomorrow. Cheers.

ltratt · 2019-01-10T21:16:25Z

src/librustc_mir/transform/add_yk_swt_calls.rs

-    // FIXME Anything which the trace recorder depends upon cannot be traced or we will get
-    // infinite recursion at runtime..
-    if RECORDER_DEPS.contains(&&*tcx.crate_name(LOCAL_CRATE).as_str()) {
+    // Don't transform any crate which is blacklisted. See the comment against BLACKLISTED_CRATES.


I don't think we need this comment (it just says what the line of code below does, and the line of code is very clear IMHO).

ltratt · 2019-01-21T08:33:59Z

Is this one still moving?

vext01 · 2019-01-25T10:53:01Z

This was rendered dead by #4

update from origin 2020-06-15

This is a combination of 18 commits. Commit softdevteam#2: Additional examples and some small improvements. Commit softdevteam#3: fixed mir-opt non-mir extensions and spanview title elements Corrected a fairly recent assumption in runtest.rs that all MIR dump files end in .mir. (It was appending .mir to the graphviz .dot and spanview .html file names when generating blessed output files. That also left outdated files in the baseline alongside the files with the incorrect names, which I've now removed.) Updated spanview HTML title elements to match their content, replacing a hardcoded and incorrect name that was left in accidentally when originally submitted. Commit softdevteam#4: added more test examples also improved Makefiles with support for non-zero exit status and to force validation of tests unless a specific test overrides it with a specific comment. Commit softdevteam#5: Fixed rare issues after testing on real-world crate Commit softdevteam#6: Addressed PR feedback, and removed temporary -Zexperimental-coverage -Zinstrument-coverage once again supports the latest capabilities of LLVM instrprof coverage instrumentation. Also fixed a bug in spanview. Commit softdevteam#7: Fix closure handling, add tests for closures and inner items And cleaned up other tests for consistency, and to make it more clear where spans start/end by breaking up lines. Commit softdevteam#8: renamed "typical" test results "expected" Now that the `llvm-cov show` tests are improved to normally expect matching actuals, and to allow individual tests to override that expectation. Commit softdevteam#9: test coverage of inline generic struct function Commit softdevteam#10: Addressed review feedback * Removed unnecessary Unreachable filter. * Replaced a match wildcard with remining variants. * Added more comments to help clarify the role of successors() in the CFG traversal Commit softdevteam#11: refactoring based on feedback * refactored `fn coverage_spans()`. * changed the way I expand an empty coverage span to improve performance * fixed a typo that I had accidently left in, in visit.rs Commit softdevteam#12: Optimized use of SourceMap and SourceFile Commit softdevteam#13: Fixed a regression, and synched with upstream Some generated test file names changed due to some new change upstream. Commit softdevteam#14: Stripping out crate disambiguators from demangled names These can vary depending on the test platform. Commit softdevteam#15: Ignore llvm-cov show diff on test with generics, expand IO error message Tests with generics produce llvm-cov show results with demangled names that can include an unstable "crate disambiguator" (hex value). The value changes when run in the Rust CI Windows environment. I added a sed filter to strip them out (in a prior commit), but sed also appears to fail in the same environment. Until I can figure out a workaround, I'm just going to ignore this specific test result. I added a FIXME to follow up later, but it's not that critical. I also saw an error with Windows GNU, but the IO error did not specify a path for the directory or file that triggered the error. I updated the error messages to provide more info for next, time but also noticed some other tests with similar steps did not fail. Looks spurious. Commit softdevteam#16: Modify rust-demangler to strip disambiguators by default Commit softdevteam#17: Remove std::process::exit from coverage tests Due to Issue #77553, programs that call std::process::exit() do not generate coverage results on Windows MSVC. Commit softdevteam#18: fix: test file paths exceeding Windows max path len

Don't run `resolve_vars_if_possible` in `normalize_erasing_regions` Neither `@eddyb` nor I could figure out what this was for. I changed it to `assert_eq!(normalized_value, infcx.resolve_vars_if_possible(&normalized_value));` and it passed the UI test suite. <details><summary> Outdated, I figured out the issue - `needs_infer()` needs to come _after_ erasing the lifetimes </summary> Strangely, if I change it to `assert!(!normalized_value.needs_infer())` it panics almost immediately: ``` query stack during panic: #0 [normalize_generic_arg_after_erasing_regions] normalizing `<str::IsWhitespace as str::pattern::Pattern>::Searcher` #1 [needs_drop_raw] computing whether `str::iter::Split<str::IsWhitespace>` needs drop softdevteam#2 [mir_built] building MIR for `str::<impl str>::split_whitespace` softdevteam#3 [unsafety_check_result] unsafety-checking `str::<impl str>::split_whitespace` softdevteam#4 [mir_const] processing MIR for `str::<impl str>::split_whitespace` softdevteam#5 [mir_promoted] processing `str::<impl str>::split_whitespace` softdevteam#6 [mir_borrowck] borrow-checking `str::<impl str>::split_whitespace` softdevteam#7 [analysis] running analysis passes on this crate end of query stack ``` I'm not entirely sure what's going on - maybe the two disagree? </details> For context, this came up while reviewing rust-lang/rust#77467 (cc `@lcnr).` Possibly this needs a crater run? r? `@nikomatsakis` cc `@matthewjasper`

Preliminary trace recording.

f95a776

vext01 assigned ltratt Jan 10, 2019

bors bot added a commit that referenced this pull request Jan 10, 2019

Try #3:

a7a94df

ltratt reviewed Jan 10, 2019

View reviewed changes

Address Laurie's comments.

573995f

ltratt reviewed Jan 10, 2019

View reviewed changes

vext01 closed this Jan 25, 2019

bors bot pushed a commit that referenced this pull request Jul 9, 2020

Merge pull request #3 from rust-lang/master

395256a

update from origin 2020-06-15

Preliminary trace recording. #3

Preliminary trace recording. #3

Uh oh!

Conversation

vext01 commented Jan 10, 2019

Uh oh!

vext01 commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Jan 10, 2019

Uh oh!

ltratt commented Jan 10, 2019

Uh oh!

vext01 commented Jan 10, 2019

Uh oh!

ltratt commented Jan 10, 2019

Uh oh!

vext01 commented Jan 10, 2019

Uh oh!

bors bot commented Jan 10, 2019

try

Build failed

Uh oh!

vext01 commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltratt commented Jan 21, 2019

Uh oh!

vext01 commented Jan 25, 2019

Uh oh!

Uh oh!

vext01 Jan 11, 2019 •

edited

Loading