The start of software tracing. #2

vext01 · 2018-12-31T15:09:36Z

Here's the long-awaited PR for the beginnings of software tracing.

See the commit messages for a description of the changes.

Notes:

The body of the software tracing function is not yet implemented. This will come next. I plan for it to store the locations it receives into a thread local vector. A function will then be implemented to acquire the contents.
I've decided not to test any of our new behaviours in existing tests, as we should eventually have our own tests if we decide to keep this code. I will raise an issue if/when we merge.
At some point in the future, we will have to have a compile-time switch to choose between software or hardware tracing. We would need some kind of conditional ignore flag for tests. Any test I've marked FIXME ignore_swt can be run if software tracing was not built in. Note that the choice between hardware or software tracing will need to be compile-time, as the standard library must have been annotated if we want software tracing. In a hardware tracing scenario, we don't want the overhead of the calls to the software trace function.

Comments? Questions? I expect there will be many.

(Happy new year, for later!)

vext01 · 2018-12-31T15:09:44Z

bors try

vext01 · 2018-12-31T15:12:19Z

Wait, the order of the commits is wrong.

bors · 2018-12-31T15:12:59Z

try

Build failed

buildbot/buildbot-build-script

vext01 · 2018-12-31T15:15:26Z

False alarm. Github's web interface orders the commits by date, not by the order in git.

The order is correct in git, i.e. the test change comes after the MIR pass change. I only committed the tests first to make the output of git status readable. There are so many test changes!

bors try

ltratt · 2018-12-31T16:00:41Z

Can you explain no_trace vs yk_swt_rec_loc? Also, would I be an idiot if I couldn't see either being defined in 74a44dc? I think that commit is in the wrong place in the list?

src/librustc_mir/transform/add_yk_swt_calls.rs

ltratt · 2018-12-31T16:02:43Z

src/librustc_mir/transform/add_yk_swt_calls.rs

+        // The original blocks are copied and the recorder function returns to a copy.
+        let mut copied_blocks = Vec::new();
+
+        // New local decls are required to accomodate the (unit) return value of the recorder func.


We are forced to allocate space for the return value of the call, even though it's (). That's just the way it is AFAICS.

So is it just one new decl or ...?

Since we add one call for each block in the MIR, we add one new local decl per-block.

Ah, right, so that's the crucial bit of missing information! That should go into the uber-comment I suggested a few lines above.

src/librustc_mir/transform/add_yk_swt_calls.rs

ltratt · 2018-12-31T16:06:02Z

src/librustc_mir/transform/add_yk_swt_calls.rs

+    }
+}
+
+/// Given a `MirSource`, decides if we should annotate the correpsonding MIR.


Should this be "Given a MirSource, return true if the corresponding MIR should be annotated or false otherwise." Although I don't think "annotate" is the right word here: we really mean something like "should_be_traced"?

Should this be

It should be clear from the name of the function that true means we should.

annotate vs. should_be_traced

We are annotating blocks with calls, right?

I'm not sure about should_be_traced. How about should_transform?

You'd be amazed at how many functions that return bool aren't clear about what true/false means, so I find it best to be explicit. It's a bit like axes on graphs: often stating "higher is better" (or "lower is better") massively helps readers.

I'm actually wondering if we've got this function the wrong way around (so to speak). Maybe instead of "this can be annotated/transformed", it would be clearer to have the function be "can't be annotated/transformed"? Then the function might be called something like is_untraceable.

Maybe instead of "this can be annotated/transformed", it would be clearer to have the function be "can't be annotated/transformed"

I'm not too fussed. Happy to give that a go.

On a side note, we will have to come back later and make a record (in the binary somewhere) of what was not traceable. We will need this at runtime. So perhaps your suggestion is a good one.

ltratt · 2018-12-31T16:08:11Z

src/librustc_mir/transform/add_yk_swt_calls.rs

+        }
+    }
+
+    // We can't call the software tracing function if there is no libcore.


This comment, and the rest of the comments in this function, state what the code does. The code is simple enough that the comments aren't very useful in that capacity. I'm wondering if we need to state why the code is here for these things? e.g. we might say "We can't trace a function if this crate doesn't have a link to libcore, as that's needed for the trace function"?

OK, will review these.

src/librustc_mir/transform/add_yk_swt_calls.rs

ltratt · 2018-12-31T16:15:34Z

Also, would I be an idiot if I couldn't see either being defined in 74a44dc? I think that commit is in the wrong place in the list?

Ah, I assume this is the github ordering problem you alluded to earlier? When we come to rebasing things later, it might be worth changing the date stamps on commits to make this a bit clearer?

vext01 · 2018-12-31T16:16:06Z

Can you explain no_trace vs yk_swt_rec_loc?

Do you mean, why do we apply no_trace to the trace func?

Also, would I be an idiot if I couldn't see either being defined in 74a44dc? I think that commit is in the wrong place in the list?

Github got the order of the commits wrong. See this comment:
#2 (comment)

ltratt · 2018-12-31T16:17:15Z

Do you mean, why do we apply no_trace to the trace func?

No, I mean I'm not sure why we sometimes use one and sometimes the other.

vext01 · 2018-12-31T16:21:00Z

No, I mean I'm not sure why we sometimes use one and sometimes the other.

no_trace is an attribute which can be applied at the item- or crate-level to suppress the MIR transform for that item or crate.

yk_swt_rec_loc is the function that the MIR pass adds calls to as long as the containing function or crate was not no_trace.

Makes sense?

vext01 · 2018-12-31T16:28:55Z

When we come to rebasing things later, it might be worth changing the date stamps on commits to make this a bit clearer?

We can do if you want. I don't think it matters too much though. It won't be the first or last time that someone re-orders commits in a re-base.

ltratt · 2018-12-31T16:29:41Z

We can do if you want. I don't think it matters too much though. It won't be the first or last time that someone re-orders commits in a re-base.

Point taken. And, IMHO, it is a github bug not to show them in parent/child order.

vext01 · 2018-12-31T16:31:27Z

IMHO, it is a github bug not to show them in parent/child order.

I tend to agree.

Just to prove that the commits are ordered right:

$ git log
commit 74a44dc00adb50ad16b5d6df944e721531fc80ce
Author: Edd Barrett <[email protected]>
Date:   Mon Dec 31 14:52:13 2018 +0000

    Revise tests in light of Yorick software tracing MIR pass.

commit 3504c9c2c1c740a660a2120079c39aaa5defa1f4
Author: Edd Barrett <[email protected]>
Date:   Mon Dec 31 14:54:03 2018 +0000

    Add the Yorick software tracing MIR pass and associated functionality.

    This change:

     - Adds a MIR pass which adds, for each MIR block, a call to a tracing
       (language item) function. The arguments to this function uniquely
       identify a location in the MIR. A collection of such locations will
       form a trace.

     - Adds a "no_trace" attribute, which informs the compiler to not apply
       the above MIR pass to an item or crate. This is needed for tests, but
       also to prevent infinite recursion on the tracing function itself.

commit 7df215dfbe530ae9a5a3e622360e0b9b7ee138f0
Author: Edd Barrett <[email protected]>
Date:   Mon Dec 31 14:52:54 2018 +0000

    Make CI build stricter.

    This helps to find bugs earlier.

commit a9abb9a59c2ab2c7935582e9b3dade9c76571fca
Merge: 289ad6e992 98b0860a5c
Author: bors[bot] <bors[bot]@users.noreply.github.com>
Date:   Thu Nov 29 15:23:46 2018 +0000

    Merge #1
...

bors · 2018-12-31T18:51:44Z

try

Build succeeded

And happy new year from bors! 🎉

buildbot/buildbot-build-script

vext01 · 2019-01-02T13:17:56Z

How's this? I think it's an improvement :)

ltratt · 2019-01-02T14:19:15Z

src/librustc_mir/transform/add_yk_swt_calls.rs

-        let mut replace_blocks = Vec::new();
-
-        // The original blocks are copied and the recorder function returns to a copy.
+        let mut replacement_blocks = Vec::new();


The convention in this function seems to be blk for blocks? I'm certainly a fan of the shorter version! It might be nice to find a shorter, and perhaps more accurate version of "replacement". Something like "shadow" perhaps?

ltratt · 2019-01-02T14:22:07Z

src/librustc_mir/transform/add_yk_swt_calls.rs

+/// | B0 |-->| B1 |  Becomes: | B0' |-->| Rec |-->| B2 |-->| B1' |-->| Rec |-->| B3 |
+/// +----+   +----+           +-----+   +-----+   +----+   +-----+   +-----+   +----+
+///
+/// Where:


I broadly like this, but the naming is really confusing (B0 becomes B0, B0', and B2?!). How about something like: B0 is the original; S0 is the shadow version of B0 (i.e. the copy we've made); and C0 is the copied version of B0 with a different terminator?

[This also goes along with my comment below about "replacement", which isn't a very helpful term here, since it suggests that B0 is entirely replaced. "Shadow" is my 10-second-solution, but maybe we want something which suggests "a new inserted thing which calls the old one"?]

I'm OK with B -> S, but I'd like it if the indices matched the indices in the backing vector. I'm trying to convey the fact that we have to manipulate the MIR in the context of a list, and new blocks pushed will go on the end. So Ci I'm not so keen on.

The structure of the CFG makes the ordering of the blocks completely clear doesn't it?

On the matter of the name, maybe we could call the new blocks "record blocks"? That gets their intent across quite clearly I think.

The structure of the CFG makes the ordering of the blocks completely clear doesn't it?

In the ASCII art diagram, the blocks are not shown in order they appear in the vector, which is why I named the blocks with their indices.

On the matter of the name, maybe we could call the new blocks "record blocks"?

I'm not keen on "record blocks", because IDs that are recorded into the trace are in fact the shadows. How about copy_blks?

After a quick conversation we're thinking that "shadow blocks" and "user blocks" might be appropriate terms.

ltratt · 2019-01-02T14:23:11Z

src/librustc_mir/transform/add_yk_swt_calls.rs

-    // wrapper are also marked `#[no_trace]` to prevent infinite recursion.
+/// Given a `MirSource`, decides if it is possible for us to trace (and thus whether we should
+/// transform) the MIR. Returns `true` if we cannot trace, otherwise `false`.
+fn is_untraceable(tcx: TyCtxt<'a, 'tcx, 'tcx>, src: MirSource) -> bool {


How about "Given a MirSource, return false if it should not be traced, or true otherwise"? The bit about transforming is, IMHO, irrelevant here: what the caller of this function chooses to do with the information is up to them.

Hrm, something is only untraceable if it's not possible to apply the transform. i.e. "untraceable" here is defined in terms of the transform. The concepts are tightly coupled. Unless you feel strongly, I'd rather leave it in.

You said above that you'll also need to call this function in contexts where you're not transforming anything, but simply want to know if it can/can't be traced. I know that "can't be traced" also means "can't be transformed", but this function is really only concerned about the former, isn't it?

You said above that you'll also need to call this function in contexts where you're not transforming anything, but simply want to know if it can/can't be traced

Maybe there's confusion with the no_trace attribute? The is_untraceable function has only one call site.

ltratt · 2019-01-02T14:23:59Z

src/librustc_mir/transform/add_yk_swt_calls.rs

        }
    }

-    // We can't call the software tracing function if there is no libcore.
+    // We can't call the software tracing function if there is no libcore because that's where the


When we say "no libcore" do we mean "if this crate doesn't have a link to libcore" or ...?

If this crate doesn't depend upon libcore, then we can't call something (the tracer func) in libcore.

Aha, so I think "depend upon" is the key missing phrase then?

ltratt · 2019-01-02T14:24:16Z

src/librustc_mir/transform/add_yk_swt_calls.rs

    if attr::contains_name(tcx.hir.krate_attrs(), "no_core") {
-        return false;
+        return true;
    }

    // The libcompiler_builtins crate is special and we can't annotate it.


s/can't/mustn't/? Also why mustn't we annotate it?

To be honest, the compiler_builtins crate is a bit of a mystery to me. All I know is that strange things happen if you do transform it.

Ah: s/annotate/transform/ too.

If you like, we can remove this check, run bors, and observe the error message for clues as to what to put in this comment? I don't recall exactly what happens off the top of my head.

It might be a good idea. If we've already forgotten, someone in the future's going to worry about it too. Since this is such a critical part of the whole infrastructure, I suspect our future selves will be grateful for such hints :)

Okie dokie! Here goes then.

vext01 · 2019-01-02T15:42:09Z

bors try

stand by for compilation error :)

bors · 2019-01-02T17:18:01Z

try

Build failed

buildbot/buildbot-build-script

vext01 · 2019-01-08T10:10:50Z

The relevant part of the log is:

undefined reference to `core::yk_swt::yk_swt_rec_loc_wrap'

I don't understand why this is the case, but since libcompiler_builtins is a load of wrapped C and asm code which we can't transform anyway, I'm still in favour of skipping this crate.

We can add a comment like:

/// Transforming libcompiler_builtins gives undefined references (linker magic?), but
/// even if it didn't the code within is just a load of wrapped C and asm code which we
/// can't transform anyway.

Thoughts?

ltratt · 2019-01-08T14:58:59Z

More-or-less fine with me, but why don't we tell people that it's "core::yk_swt::yk_swt_rec_loc_wrap" that gives an error, just in case that rings bells with them?

vext01 · 2019-01-08T15:36:21Z

OK, there's a few more fixes.

There's a few comments awaiting.

vext01 · 2019-01-08T16:34:47Z

That's the last fix in. Happy for me to squash?

ltratt · 2019-01-08T17:22:08Z

Please squash.

This helps to find bugs earlier.

This change: - Adds a MIR pass which adds, for each MIR block, a call to a tracing (language item) function. The arguments to this function uniquely identify a location in the MIR. A collection of such locations will form a trace. - Adds a "no_trace" attribute, which informs the compiler to not apply the above MIR pass to an item or crate. This is needed for tests, but also to prevent infinite recursion on the tracing function itself.

vext01 · 2019-01-08T17:30:28Z

splat.

ltratt · 2019-01-08T18:44:17Z

I don't think you've pushed anything?

vext01 · 2019-01-09T09:35:39Z

Oops! Try now.

ltratt · 2019-01-09T09:36:20Z

bors r+

2: The start of software tracing. r=ltratt a=vext01 Here's the long-awaited PR for the beginnings of software tracing. See the commit messages for a description of the changes. Notes: - The body of the software tracing function is not yet implemented. This will come next. I plan for it to store the locations it receives into a thread local vector. A function will then be implemented to acquire the contents. - I've decided not to test any of our new behaviours in existing tests, as we should eventually have our own tests if we decide to keep this code. I will raise an issue if/when we merge. - At some point in the future, we will have to have a compile-time switch to choose between software or hardware tracing. We would need some kind of conditional ignore flag for tests. Any test I've marked `FIXME ignore_swt` can be run if software tracing was not built in. Note that the choice between hardware or software tracing will need to be compile-time, as the standard library must have been annotated if we want software tracing. In a hardware tracing scenario, we don't want the overhead of the calls to the software trace function. Comments? Questions? I expect there will be many. (Happy new year, for later!) Co-authored-by: Edd Barrett <[email protected]>

bors · 2019-01-09T13:14:04Z

Build succeeded

buildbot/buildbot-build-script

Some review feedback and other misc tweaks

Use arenas to avoid Lrc in queries #2 The `Remove subtle Default impl for Value` makes the compilation stop due earlier due to cycle errors, since there's no longer a default value to continue the compilation with. Based on rust-lang/rust#59540.

@ghost

Rollup of 6 pull requests Successful merges: - #59545 (Use arenas to avoid Lrc in queries #2) - #61054 (Suggest dereferencing on assignment to mutable borrow) - #61056 (tweak discriminant on non-nullary enum diagnostic) - #61082 (fix dangling reference in Vec::append) - #61086 (Box::into_unique: do the reborrow-to-raw *after* destroying the Box) - #61098 (Fix overflowing literal lint in loops) Failed merges: r? @ghost

update master

@ayosec

Stabilize `#[track_caller]`. # Stabilization Report RFC: [2091] Tracking issue: rust-lang/rust#47809 ## Summary From the [rustc-dev-guide chapter][dev-guide]: > Take this example program: ```rust fn main() { let foo: Option<()> = None; foo.unwrap(); // this should produce a useful panic message! } ``` > Prior to Rust 1.42, panics like this `unwrap()` printed a location in libcore: ``` $ rustc +1.41.0 example.rs; example.exe thread 'main' panicked at 'called `Option::unwrap()` on a `None` value',...core\macros\mod.rs:15:40 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace. ``` > As of 1.42, we get a much more helpful message: ``` $ rustc +1.42.0 example.rs; example.exe thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', example.rs:3:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` > These error messages are achieved through a combination of changes to `panic!` internals to make use of `core::panic::Location::caller` and a number of `#[track_caller]` annotations in the standard library which propagate caller information. The attribute adds an implicit caller location argument to the ABI of annotated functions, but does not affect the type or MIR of the function. We implement the feature entirely in codegen and in the const evaluator. ## Bottom Line This PR stabilizes the use of `#[track_caller]` everywhere, including traits and extern blocks. It also stabilizes `core::panic::Location::caller`, although the use of that function in a const context remains gated by `#![feature(const_caller_location)]`. The implementation for the feature already changed the output of panic messages for a number of std functions, as described in the [1.42 release announcement]. The attribute's use in `Index` and `IndexMut` traits is visible to users since 1.44. ## Tests All of the tests for this feature live under [src/test/ui/rfc-2091-track-caller][tests] in the repo. Noteworthy cases: * [use of attr in std] * validates user-facing benefit of the feature * [trait attribute inheritance] * covers subtle behavior designed during implementation and not RFC'd * [const/codegen equivalence] * this was the result of a suspected edge case and investigation * [diverging function support] * covers an unresolved question from the RFC * [fn pointers and shims] * covers important potential sources of unsoundness ## Documentation The rustc-dev-guide now has a chapter on [Implicit Caller Location][dev-guide]. I have an [open PR to the reference][attr-reference-pr] documenting the attribute. The intrinsic's [wrapper] includes some examples as well. ## Implementation History * 2019-10-02: [`#[track_caller]` feature gate (RFC 2091 1/N) #65037](rust-lang/rust#65037) * Picked up the patch that @ayosec had started on the feature gate. * 2019-10-13: [Add `Instance::resolve_for_fn_ptr` (RFC 2091 #2/N) #65182](rust-lang/rust#65182) * 2019-10-20: ~~[WIP Add MIR argument for #[track_caller] (RFC 2091 3/N) #65258](rust-lang/rust#65258 * Abandoned approach to send location as a MIR argument. * 2019-10-28: [`std::panic::Location` is a lang_item, add `core::intrinsics::caller_location` (RFC 2091 3/N) #65664](rust-lang/rust#65664) * 2019-12-07: [Implement #[track_caller] attribute. (RFC 2091 4/N) #65881](rust-lang/rust#65881) * 2020-01-04: [libstd uses `core::panic::Location` where possible. #67137](rust-lang/rust#67137) * 2020-01-08: [`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]` #67887](rust-lang/rust#67887) * 2020-01-20: [Fix #[track_caller] and function pointers #68302](rust-lang/rust#68302) (fixed #68178) * 2020-03-23: [#[track_caller] in traits #69251](rust-lang/rust#69251) * 2020-03-24: [#[track_caller] on core::ops::{Index, IndexMut}. #70234](rust-lang/rust#70234) * 2020-04-08 [Support `#[track_caller]` on functions in `extern "Rust" { ... }` #70916](rust-lang/rust#70916) ## Unresolveds ### From the RFC > Currently the RFC simply prohibit applying #[track_caller] to trait methods as a future-proofing > measure. **Resolved.** See the dev-guide documentation and the tests section above. > Diverging functions should be supported. **Resolved.** See the tests section above. > The closure foo::{{closure}} should inherit most attributes applied to the function foo, ... **Resolved.** This unknown was related to specifics of the implementation which were made irrelevant by the final implementation. ### Binary Size I [instrumented track_caller to use custom sections][measure-size] in a local build and discovered relatively minor binary size usage for the feature overall. I'm leaving the issue open to discuss whether we want to upstream custom section support. There's an [open issue to discuss mitigation strategies][mitigate-size]. Some decisions remain about the "right" strategies to reduce size without overly constraining the compiler implementation. I'd be excited to see someone carry that work forward but my opinion is that we shouldn't block stabilization on implementing compiler flags for redaction. ### Specialization There's an [open issue][specialization] on the semantics of the attribute in specialization chains. I'm inclined to move forward with stabilization without an exact resolution here given that specialization is itself unstable, but I also think it should be an easy question to resolve. ### Location only points to the start of a call span rust-lang/rust#69977 was resolved by rust-lang/rust#73182, and the next step should probably be to [extend `Location` with a notion of the end of a call](rust-lang/rust#73554). ### Regression of std's panic messages #70963 should be resolved by serializing span hygeine to crate metadata: rust-lang/rust#68686. [2091]: https://github.com/rust-lang/rfcs/blob/master/text/2091-inline-semantic.md [dev-guide]: https://rustc-dev-guide.rust-lang.org/codegen/implicit-caller-location.html [specialization]: rust-lang/rust#70293 [measure-size]: rust-lang/rust#70579 [mitigate-size]: rust-lang/rust#70580 [attr-reference-pr]: rust-lang/reference#742 [wrapper]: https://doc.rust-lang.org/nightly/core/panic/struct.Location.html#method.caller [tests]: https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-2091-track-caller [const/codegen equivalence]: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2091-track-caller/caller-location-fnptr-rt-ctfe-equiv.rs [diverging function support]: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2091-track-caller/diverging-caller-location.rs [use of attr in std]: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2091-track-caller/std-panic-locations.rs [fn pointers and shims]: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2091-track-caller/tracked-fn-ptr-with-arg.rs [trait attribute inheritance]: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2091-track-caller/tracked-trait-impls.rs [1.42 release announcement]: https://blog.rust-lang.org/2020/03/12/Rust-1.42.html#useful-line-numbers-in-option-and-result-panic-messages

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

``` Benchmark #1: ./raytracer_cg_clif_pre Time (mean ± σ): 9.553 s ± 0.129 s [User: 9.543 s, System: 0.008 s] Range (min … max): 9.438 s … 9.837 s 10 runs Benchmark softdevteam#2: ./raytracer_cg_clif_post Time (mean ± σ): 9.463 s ± 0.055 s [User: 9.452 s, System: 0.008 s] Range (min … max): 9.387 s … 9.518 s 10 runs Summary './raytracer_cg_clif_post' ran 1.01 ± 0.01 times faster than './raytracer_cg_clif_pre' ```

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`

vext01 assigned ltratt Dec 31, 2018

bors bot added a commit that referenced this pull request Dec 31, 2018

Try #2:

7147033

bors bot added a commit that referenced this pull request Dec 31, 2018

Try #2:

6246233

ltratt reviewed Dec 31, 2018

View reviewed changes

src/librustc_mir/transform/add_yk_swt_calls.rs Outdated Show resolved Hide resolved

ltratt reviewed Dec 31, 2018

View reviewed changes

src/librustc_mir/transform/add_yk_swt_calls.rs Outdated Show resolved Hide resolved

ltratt reviewed Dec 31, 2018

View reviewed changes

src/librustc_mir/transform/add_yk_swt_calls.rs Outdated Show resolved Hide resolved

ltratt reviewed Dec 31, 2018

View reviewed changes

src/librustc_mir/transform/add_yk_swt_calls.rs Outdated Show resolved Hide resolved

ltratt reviewed Dec 31, 2018

View reviewed changes

src/librustc_mir/transform/add_yk_swt_calls.rs Show resolved Hide resolved

ltratt reviewed Jan 2, 2019

View reviewed changes

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

Try #2:

fa2ca0f

vext01 added 3 commits January 8, 2019 17:28

Make CI build stricter.

9496d34

This helps to find bugs earlier.

Revise tests in light of Yorick software tracing MIR pass.

04cfa02

vext01 force-pushed the yk-swt branch from 262d33b to 04cfa02 Compare January 9, 2019 09:35

bors bot merged commit 04cfa02 into master Jan 9, 2019

bors bot deleted the yk-swt branch January 9, 2019 13:14

bors bot pushed a commit that referenced this pull request Jun 4, 2019

Merge pull request #2 from alexcrichton/azure-pipelines

e6830c1

Some review feedback and other misc tweaks

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

Merge pull request #2 from rust-lang/master

00ef461

update master

ptersilie added a commit to ptersilie/ykrustc that referenced this pull request Oct 13, 2020

Prime softdevteam#2

2e9283b

The start of software tracing. #2

The start of software tracing. #2

Conversation

vext01 commented Dec 31, 2018

vext01 commented Dec 31, 2018

vext01 commented Dec 31, 2018

bors bot commented Dec 31, 2018

try

Build failed

vext01 commented Dec 31, 2018

ltratt commented Dec 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Dec 31, 2018

vext01 commented Dec 31, 2018

ltratt commented Dec 31, 2018

vext01 commented Dec 31, 2018

vext01 commented Dec 31, 2018

ltratt commented Dec 31, 2018

vext01 commented Dec 31, 2018

bors bot commented Dec 31, 2018

try

Build succeeded

vext01 commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Jan 2, 2019

bors bot commented Jan 2, 2019

try

Build failed

vext01 commented Jan 8, 2019

ltratt commented Jan 8, 2019

vext01 commented Jan 8, 2019

vext01 commented Jan 8, 2019

ltratt commented Jan 8, 2019

vext01 commented Jan 8, 2019

ltratt commented Jan 8, 2019

vext01 commented Jan 9, 2019

ltratt commented Jan 9, 2019

bors bot commented Jan 9, 2019

Build succeeded