Software Trace Function in C. #4

vext01 · 2019-01-18T10:12:19Z

This is a DNMY (do no merge yet) PR. It needs some discussion.

As discussed earlier this week, this is an attempt to implement the tracing function in C so that our MIR pass doesn't see the innards of the tracing mechanisms and it's dependencies.

This means we can trace a lot more than we could have before, where e.g. thread locals and vectors used in the tracing mechanism were blocked from being traced.

I printed the trace from one of my tests, and it looks highly plausable (tm):

MirLoc { crate_hash: 7687613001418581878, def_idx: 14, bb_idx: 1 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 0 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 1 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 }
MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 }
...

I can see a loop!

Discussion Points

Low-level of abstraction.

Because the tracing function is now in C, and because we no longer need Vec or std::thread from libstd to trace, I opted to move the whole mechanism into libcore. This is better in the sense that we can trace stuff even if libstd isn't present (a dependency of the crate being traced).

On the other hand, working in libcore is limiting and it's evident from the low level of abstraction in the API you see. I'd have liked to, for example, have given the user an iterator and a Drop implementation for the trace struct, but these are libstd concepts.

I propose that in the next PR we make the libcore interface even more low-level -- make stop_tracing return a tuple, not a struct -- and then move the trace struct up into libstd where it can have better abstraction.

This should mean that VM authors get a nice API to start and stop tracing via libstd, but they can still trace no_std crates that their VMs might depend upon.

Traits

Looking to the future, we will eventually have two backends (sw/hw) and a compile time switch to choose one. I suspect that we will want to offer the same API regardless of the backend, so we probably want a place to keep the common parts (e.g. struct MirLoc can probably be shared) and a trait for the API which includes stuff like getting an iterator over a trace.

This should be a PR in the near future.

C Errors

Currently the C tracing code will crash hard (with err() or errx()) if there is an error. We could implement something like we did in hwtracer where each FFI call passes down an error struct pointer, which can be read by Rust after the call.

Since this change can be hidden from the user in abstractions, I think this is low-priority for now.

Do you agree with the above plan?

Cheers

ltratt · 2019-01-18T10:14:57Z

src/libcore/yk_swt.rs

+    /// but we can do neither due to libcore restrictions.
+    buf: *mut MirLoc,
+    /// The number of items in the above array.
+    len: u32,


This should be usize I think?

Sure. The only downside would be that it wouldn't fit in a register on 32-bit. Does it bother us?

On a 32-bit machine, usize will be 32 bits.

Duh! Yep. Brainfart :P

ltratt · 2019-01-18T10:17:35Z

src/libcore/yk_swt.rs

+        self.len
+    }
+
+    /// Return a pointer to the raw trace buffer. Provided so that the consumer can free it.


"Can" is a very vague word. Are they required to free it? If so, this should probably be fn buf(self) -> .... If not... this seems very dangerous!

Good point.

If you agree with the abstraction discussion point above this becomes moot, but I'm happy to implement your suggestion in the interim.

I haven't fully understood how the abstraction discussion will solve this, but if all we have to do is turn &self into self, we might as well do it in this PR.

ltratt · 2019-01-18T10:18:41Z

src/libcore/yk_swt.rs

+    }
+
+    /// Returns the location at index `idx` or `None` if the index is out of bounds.
+    pub fn loc<'a>(&'a self, idx: u32) -> Option<&'a MirLoc> {


I suggest we make this panic if idx is out of bounds and return MirLoc directly rather than through an Option. It makes things quite a bit faster and given we'll be using this a lot, it's worth it.

ltratt · 2019-01-18T10:21:17Z

src/libcore/yk_swt_impl.c

+        if (tl_trace_cap >= UINT32_MAX / 2) {
+            errx(EXIT_FAILURE, "%s: trace capacity would overflow", __func__);
+        }
+        size_t new_cap = tl_trace_cap * 2;


This is a bit dangerous, I think: it means we're logarithmically increasing the buffer size. In other words, the bigger the buffer is, the more likely that we end up wasting lots of memory. We probably want some sort of threshold here that says something like "if the buffer is bigger than XMiB, then only add 1MiB extra storage" or similar.

it means we're logarithmically increasing

exponentially, yeah.

How about we grow linearly for now? We can be clever later.

Exponentially, oops!

I'm happy to simplify and be cleverer later.

ltratt · 2019-01-18T10:21:53Z

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

+
+pub fn main() {
+    start_tracing();
+    let _ = work();


Why the let _ =?

I was trying to encourage the compiler to not remove the call altogether.

I'm about 98% certain that assigning to the dummy variable won't affect what the compiler does in that way. You might want the black_box function?

OK, I'll look into that.

ltratt · 2019-01-18T10:22:21Z

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

+
+    for idx in 0..len {
+        let loc = trace.loc(idx).unwrap();
+        assert_ne!(loc.crate_hash, 0); // A crate hash is so unlikely to be zero.


"So unlikely"?!

In the same way a hash collision is unlikely to cause a problem in git.

Is it the wording that bothers you, or the assertion itself?

The wording, definitely bothers me ;) Given that the assertion is incorrect in some cases, I'm also unsure if it's a wise thing to be asserting.

OK, let's kill the assert.

vext01 · 2019-01-18T10:38:03Z

By the way, this code is not immune from the signal handling issue we discussed the other day. It's not yet clear to me whether clever use of atomic bools could detect if we have re-entered the trace recorder a second time and do something (panic or invalidate the trace).

Perhaps we can think about that later.

ltratt · 2019-01-18T11:05:13Z

If we have a thread-local atomic (I know that sounds like a contradiction!) bool, we can set it when we start tracing and unset it when we're done. If someone tries to set it when it's set... boom!

vext01 · 2019-01-18T11:09:16Z

Yeah, that's what I was thinking. We might even get away with relaxed ordering.

vext01 · 2019-01-18T11:48:08Z

I'm thinking of promoting tl_tracing_en to an atomic bool too, so as to strengthen the "already tracing" / "not already tracing" checks which could otherwise fail in the event of whack signal handlers. Are you happy for me to try this?

ltratt · 2019-01-18T11:49:38Z

A relaxed bool is definitely fine for this use case. I thought the idea would basically be to make tl_tracing_en the atomic bool? I don't think we need two bools doing the same thing?

vext01 · 2019-01-18T11:54:55Z

A relaxed bool is definitely fine

Actually I was unable to convince myself that relaxed is OK. I think we need acquire and release to ensure that the body of the function remains between the enclosing set/clear of the flag.

memory_order_relaxed: Relaxed operation: there are no synchronization or ordering constraints imposed on other reads or writes, only this operation's atomicity is guaranteed.

I don't think that's sufficient for our case.

I thought the idea would basically be to make tl_tracing_en the atomic bool? I don't think we need two bools doing the same thing?

The problem is, any value of tl_tracing_en is valid when calling the trace recorder function, so it cannot be used as a re-entrancy check. We'd need a second flag.

ltratt · 2019-01-18T11:56:06Z

Actually I was unable to convince myself that relaxed is OK. I think we need acquire and release to ensure that the body of the function remains between the enclosing set/clear of the flag.

That's irrelevant. We simply need to see if the bool is set to know if tracing is occurring. Other side-effects in the program are of no use.

The problem is, any value of tl_tracing_en is valid when calling the trace recorder function, so it cannot be used as a re-entrancy check. We'd need a second flag.

What does "any value ... is valid" mean?

vext01 · 2019-01-18T12:07:01Z

That's irrelevant. We simply need to see if the bool is set to know if tracing is occurring. Other side-effects in the program are of no use.

I don't think that's right.

The trace recorder basically does this:

record_trace(loc) {
    old_flag = set_atomic_entered_flag();
    if old_flag { panic };

    store_loc(loc);
    trace_len ++;

    clear_atomic_entered_flag();
}

If we use relaxed orderings, the compiler could happily emit:

record_trace(loc) {
    store_loc(loc);
    // P1
    trace_len ++;

    old_flag = set_atomic_entered_flag();
    if old_flag { panic };
    clear_atomic_entered_flag();
}

Suppose we were interrupted at P1 and the handler re-entered, we would not detect this.

What does "any value ... is valid" mean?

tl_tracing_en is used to indicate if the recorder should do anything at all: true = record the location, false just return. Both cases are valid, so I don't see how a re-entrancy check can be performed without a separate flag.

I wonder if I should post my code, as it probably communicates it better than I can explain in prose.

ltratt · 2019-01-18T12:31:40Z

You're right: we need to do it as a lock. So we need to use acquire/release ordering for the set/unset of the bool respectively.

vext01 · 2019-01-18T14:30:49Z

You're right: we need to do it as a lock

Well, it's not a lock per-se, but in order to reliably decide if we have re-entered, the flag must be true for the entire duration of the function, i.e. with the full function body between the flag set and clear.

I've also managed to convince myself that the checks in start/stop_tracing will still work without atomics, even in the presence of signal handlers.

The last commit should address all of the outstanding comments. I also forgot to add some test files, and included them in this commit.

I did full tests at the time of raising the PR, but not since our modifications, so it may explode yet :)

Ready for re-review.

ltratt · 2019-01-18T14:33:16Z

I still don't understand why we have tl_tracing_en and tl_entered_recorder. Surely we only need one?

vext01 · 2019-01-18T14:35:18Z

I still don't understand why we have tl_tracing_en and tl_entered_recorder. Surely we only need one?

I don't see how, but I may be wrong. Which one do you think can be removed?

ltratt · 2019-01-18T14:35:41Z

What is the difference between the two? They both say "I'm tracing" I think?

vext01 · 2019-01-18T14:41:24Z

tl_tracing_en: is the thread currently tracing? Used by the trace recorder function to decide whether to store a location into the trace buffer.
tl_entered_recorder: is the thread inside the trace recorder function? Used to check if a thread has re-entered the trace recorder function.

I think we both agree that tl_tracing_en is required to decide whether to store a location or not, but now try to show me how to prevent the thread re-entering the recorder function using only tl_tracing_en. I don't think it can be done, as entering the recorder with tl_tracing_en = true and with tl_tracing_en = false are both common and valid scenarios. tl_tracing_en cannot be used for a re-entrancy check as far as I can see.

ltratt · 2019-01-18T16:15:18Z

If tl_tracing_en is false, then tl_entered_recorder must be false too I think?

vext01 · 2019-01-18T19:58:15Z

If tl_tracing_en is false, then tl_entered_recorder must be false too I think?

Afraid not. Each block calls the trace recorder regardless of if the thread is currently tracing. Inside the tracing impl, the recorder then bails out early if the thread is not tracing:

    // If tracing is not currently active, then we do nothing.
    if (!tl_tracing_en) {
        goto done;
    }

ltratt · 2019-01-18T20:00:34Z

OK, I think I'm with you now.

I'm wondering if we can find better names for these? I'm not sure the tl_ prefix is useful (we don't use gl_ for globals!). Maybe just tracing and in_recorder or something like that?

vext01 · 2019-01-18T20:02:33Z

Maybe just tracing and in_recorder

Fine by me. The only reason I added the prefix was to make it clear it's thread local, but maybe it isn't as useful as first thought.

ltratt · 2019-01-18T20:02:58Z

OK, shall we try them on for size?

vext01 · 2019-01-18T20:11:46Z

I'm just applying those, and wondering if tracing is too generic and easily shadowed accidentally? Is tracing_enabled OK?

ltratt · 2019-01-18T20:14:57Z

tracing_enabled feels a bit too passive to me. Maybe do_trace? Anyway, I'm not too fussed, so I'll go with whatever you pick.

vext01 · 2019-01-18T20:46:01Z

That should address your outstanding comments.

ltratt · 2019-01-18T22:14:25Z

Please squash.

ltratt · 2019-01-24T14:46:34Z

src/libcore/yk_swt_impl.c

+// On success the trace buffer is returned and the number of locations it
+// holds is written to `*ret_trace_len`. It is the responsibility of the caller
+// to free the trace buffer. A NULL pointer is returned on error (if tracing
+// wasn't started, or the trace was invalidated).


Maybe add "Calling this function without having previously called yk_swt_start_tracing_impl will lead to undefined behaviour."?

Not true. I've designed it in such a way that it would return NULL reliably.

Even if it's currently safe, the "undefined behaviour" sentence is a big red flag to callers that says "you actually have to do this!", plus it gives us a get-out-of-jail-free card in the future if we need to change things.

you actually have to do this

Well, you don't have to, but why you wouldn't I don't know.

plus it gives us a get-out-of-jail-free card in the future if we need to change things

See you should have started with this argument :)

ltratt · 2019-01-24T14:47:14Z

src/libcore/yk_swt_impl.c


    return ret_trace;
 }

 // Call this to safely mark the trace invalid.
+// We don't free the trace buffer here, as this may be called in a signal handler


The second sentence should be inside the function IMHO: it's not something that affects the caller.

vext01 · 2019-01-24T15:05:59Z

Give this a shot.

ltratt · 2019-01-24T15:06:41Z

Please squash.

vext01 · 2019-01-24T15:14:58Z

Like so

ltratt · 2019-01-24T15:15:51Z

Could we beef up the main commit message a bit, with some rough details of how things work?

vext01 · 2019-01-24T15:22:29Z

Try that.

ltratt · 2019-01-24T15:23:58Z

"We try to handle the case where the Rust stack overflow detector's signal handler interrupts a trace" We don't just try -- we do handle that case! Can you maybe add in a bit about what an "invalidated trace" is, and how we might hit it?

vext01 · 2019-01-24T15:36:19Z

Try this.

ltratt · 2019-01-24T15:36:55Z

Good stuff!

bors r+

4: Software Trace Function in C. r=ltratt a=vext01 This is a DNMY (do no merge yet) PR. It needs some discussion. As discussed earlier this week, this is an attempt to implement the tracing function in C so that our MIR pass doesn't see the innards of the tracing mechanisms and it's dependencies. This means we can trace a lot more than we could have before, where e.g. thread locals and vectors used in the tracing mechanism were blocked from being traced. I printed the trace from one of my tests, and it looks highly plausable (tm): ``` MirLoc { crate_hash: 7687613001418581878, def_idx: 14, bb_idx: 1 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 0 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 1 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } ... ``` I can see a loop! # Discussion Points ## Low-level of abstraction. Because the tracing function is now in C, and because we no longer need `Vec` or `std::thread` from `libstd` to trace, I opted to move the whole mechanism into `libcore`. This is better in the sense that we can trace stuff even if libstd isn't present (a dependency of the crate being traced). On the other hand, working in libcore is limiting and it's evident from the low level of abstraction in the API you see. I'd have liked to, for example, have given the user an iterator and a `Drop` implementation for the trace struct, but these are `libstd` concepts. I propose that in the next PR we make the libcore interface even more low-level -- make `stop_tracing` return a tuple, not a struct -- and then move the trace struct up into `libstd` where it can have better abstraction. This should mean that VM authors get a nice API to start and stop tracing via libstd, but they can still trace `no_std` crates that their VMs might depend upon. ## Traits Looking to the future, we will eventually have two backends (sw/hw) and a compile time switch to choose one. I suspect that we will want to offer the same API regardless of the backend, so we probably want a place to keep the common parts (e.g. `struct MirLoc` can probably be shared) and a trait for the API which includes stuff like getting an iterator over a trace. This should be a PR in the near future. ## C Errors Currently the C tracing code will crash hard (with `err()` or `errx()`) if there is an error. We could implement something like we did in hwtracer where each FFI call passes down an error struct pointer, which can be read by Rust after the call. Since this change can be hidden from the user in abstractions, I think this is low-priority for now. Do you agree with the above plan? Cheers Co-authored-by: Edd Barrett <[email protected]>

vext01 · 2019-01-24T15:38:24Z

Damn "a trace is invalidated is when".

Let me cancel bors and fix this.

bors · 2019-01-24T15:39:56Z

Build failed

buildbot/buildbot-build-script

ltratt · 2019-01-24T15:40:08Z

OK.

This is implemented in C because if it were implemented in Rust, then the tracer code would trace itself and everything it calls (or we'd end up annotating a lot of the standard library `#[no_trace]`). With each call to the trace recorder, we build a "MIR location" struct from the call arguments and insert this into a thread local trace buffer. A successful tracing session will return the trace buffer to the consumer when we stop tracing. If however something happens which renders the trace data unreliable, we say the trace was "invalidated" and return NULL. For example, a trace is invalidated when the Rust stack overflow detector's signal handler interrupts a trace. A trace is also invalidated when when the trace buffer can't be a allocated or re-allocated. User signal handlers remain an open problem, as we are unable to force code outside of our hands to invalidate a trace.

Since the software tracing function is now implemented in C, tracing no longer interferes in these tests.

vext01 · 2019-01-24T15:41:37Z

You can kick bors again now

ltratt · 2019-01-24T15:42:52Z

bors r+

4: Software Trace Function in C. r=ltratt a=vext01 This is a DNMY (do no merge yet) PR. It needs some discussion. As discussed earlier this week, this is an attempt to implement the tracing function in C so that our MIR pass doesn't see the innards of the tracing mechanisms and it's dependencies. This means we can trace a lot more than we could have before, where e.g. thread locals and vectors used in the tracing mechanism were blocked from being traced. I printed the trace from one of my tests, and it looks highly plausable (tm): ``` MirLoc { crate_hash: 7687613001418581878, def_idx: 14, bb_idx: 1 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 0 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 1 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 2 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 3 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 6 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 7 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 8 } MirLoc { crate_hash: 7687613001418581878, def_idx: 16, bb_idx: 9 } ... ``` I can see a loop! # Discussion Points ## Low-level of abstraction. Because the tracing function is now in C, and because we no longer need `Vec` or `std::thread` from `libstd` to trace, I opted to move the whole mechanism into `libcore`. This is better in the sense that we can trace stuff even if libstd isn't present (a dependency of the crate being traced). On the other hand, working in libcore is limiting and it's evident from the low level of abstraction in the API you see. I'd have liked to, for example, have given the user an iterator and a `Drop` implementation for the trace struct, but these are `libstd` concepts. I propose that in the next PR we make the libcore interface even more low-level -- make `stop_tracing` return a tuple, not a struct -- and then move the trace struct up into `libstd` where it can have better abstraction. This should mean that VM authors get a nice API to start and stop tracing via libstd, but they can still trace `no_std` crates that their VMs might depend upon. ## Traits Looking to the future, we will eventually have two backends (sw/hw) and a compile time switch to choose one. I suspect that we will want to offer the same API regardless of the backend, so we probably want a place to keep the common parts (e.g. `struct MirLoc` can probably be shared) and a trait for the API which includes stuff like getting an iterator over a trace. This should be a PR in the near future. ## C Errors Currently the C tracing code will crash hard (with `err()` or `errx()`) if there is an error. We could implement something like we did in hwtracer where each FFI call passes down an error struct pointer, which can be read by Rust after the call. Since this change can be hidden from the user in abstractions, I think this is low-priority for now. Do you agree with the above plan? Cheers Co-authored-by: Edd Barrett <[email protected]>

bors · 2019-01-24T19:49:50Z

Build succeeded

buildbot/buildbot-build-script

update from origin 2020-06-18

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`

vext01 assigned ltratt Jan 18, 2019

ltratt reviewed Jan 18, 2019

View reviewed changes

ltratt reviewed Jan 24, 2019

View reviewed changes

vext01 force-pushed the yk-c-trace-rec branch from 1f31122 to 6d83393 Compare January 24, 2019 15:14

vext01 force-pushed the yk-c-trace-rec branch from 6d83393 to 5ebb7f3 Compare January 24, 2019 15:22

vext01 force-pushed the yk-c-trace-rec branch from 5ebb7f3 to fd1aef1 Compare January 24, 2019 15:36

vext01 added 2 commits January 24, 2019 15:40

Remove dummy language items impls from tests. No longer needed.

12a2499

Since the software tracing function is now implemented in C, tracing no longer interferes in these tests.

vext01 force-pushed the yk-c-trace-rec branch from fd1aef1 to 12a2499 Compare January 24, 2019 15:40

bors bot merged commit 12a2499 into master Jan 24, 2019

bors bot deleted the yk-c-trace-rec branch January 24, 2019 19:49

vext01 mentioned this pull request Jan 25, 2019

Preliminary trace recording. #3

Closed

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

Merge pull request #4 from rust-lang/master

7ef9eb3

update from origin 2020-06-18

Software Trace Function in C. #4

Software Trace Function in C. #4

Conversation

vext01 commented Jan 18, 2019

Discussion Points

Low-level of abstraction.

Traits

C Errors

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019 • edited Loading

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 2019

vext01 commented Jan 18, 2019

ltratt commented Jan 18, 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

vext01 commented Jan 24, 2019

ltratt commented Jan 24, 2019

vext01 commented Jan 24, 2019

ltratt commented Jan 24, 2019

vext01 commented Jan 24, 2019

ltratt commented Jan 24, 2019

vext01 commented Jan 24, 2019

ltratt commented Jan 24, 2019

vext01 commented Jan 24, 2019

bors bot commented Jan 24, 2019

Build failed

ltratt commented Jan 24, 2019

vext01 commented Jan 24, 2019

ltratt commented Jan 24, 2019

bors bot commented Jan 24, 2019

Build succeeded

vext01 commented Jan 18, 2019 •

edited

Loading