Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the generator-locals debuginfo test resilient to re-ordering. #63742

Open
wants to merge 1 commit into
base: master
from

Conversation

@vext01
Copy link
Contributor

commented Aug 20, 2019

Hi,

In my Rust fork a made an unrelated change and found that the generator-locals debuginfo test would fail.

The relevant part of the test looks like this:

fn main() {
    let mut a = 5;
    let mut b = || {
        let c = 6; // Live across multiple yield points

        let d = 7; // Live across only one yield point
        yield;
        _zzz(); // #break
        a = d;

        let e = 8; // Live across zero yield points
        _zzz(); // #break
        a = e;

        yield;
        _zzz(); // #break
        a = c;
    };
    Pin::new(&mut b).resume();
    Pin::new(&mut b).resume();
    Pin::new(&mut b).resume();
    _zzz(); // #break
}

The test asserts that at the second breakpoint a=7, but I got a=5.

Here's the thing, I believe both a=7 and a=5 to be valid, as any code between the two yields (including the break point itself) could be re-ordered.

Assuming that is correct, how can we fix this? Here I've opted to remove the "un-anchored" breakpoint (and its associated checks), however, we could have just as easily (I think) called _zzz() through a black_box (probably passing (a, c, d, e) though too).

Thoughts?

Fix non-determinism in the test.
Since the second break-point is not anchored in any way, the compiler is
free to re-order code and give different outcomes.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

This is unfortunately a bit beyond me, so I'm going to...

r? @michaelwoerister

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@tmandry

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I would prefer the second fix you mentioned, since those checks are there for valid testing reasons. (I have a feeling some other debuginfo tests need updating as well, but feel free to file an issue if you don't feel like doing that now.)

If you don't have time to work on this, feel free to assign to me and I can prioritize fixing it.

@tmandry

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Does anyone know if we actually need to pass the values through to the black box function? My intuition would be that a black_box function prevents reordering of any operation around it, which should be sufficient to make the test work without a bunch of extra noise.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I don't know a lot about generators but if the compiler re-orders code (in a debug build) as described above then that seems like a bug to me. We should look into that rather than hide the problem in the test case.

@rust-lang/compiler Nominating for discussion during the triage meeting.

Meanwhile it would be good to take a look whether the code is actually re-ordered at the machine code level.

@vext01, what target triple are you seeing this issue on?

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I don't know a lot about generators but if the compiler re-orders code (in a debug build) as described above then that seems like a bug to me.

A little on this: the test uses the default optimisation level, which I believe is 2. However, I did try adding -C opt-level=0 and saw the same failure.

I should also mention that my fork uses a MIR pass to add a call to every block in the original MIR. This is probably part responsible for triggering the re-ordering, however not on its own (this test used to pass with that MIR pass in effect). The change in my fork which tipped this test to a fail was (oddly) enumerating the DefPathTable after code-gen.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@vext01, what target triple are you seeing this issue on?

x86_64-linux-gnu

vext01 added a commit to vext01/ykrustc_pv that referenced this pull request Aug 21, 2019
Discover additional DefIds by looking in the def_path_table.
The "definition path table" is a mapping from DefIds to definition
paths. There's an entry for every item than appears in the compiler's
metadata, thus making it an excellent discovery tool for our serialiser
code to find DefIds in dependent crates (if you run with `-Z
always-encode-mir`)!

We also switch to using tcx.mir_keys() for finding the DefIds in the
current crate. Previously we used tcx.collect_and_partition_mono_items()
which may miss some stuff.

A future change will remove the work list used in the serialised, as this
code supersedes it. That will be a fairly large change, so saving it for
another PR.

Another future change will enable `-Z always-encode-mir` by default for
ykrustc.

One of the tests we disabled here is being discussed upstream here:
rust-lang/rust#63742

When this is resolved, we might have to revisit this test in our fork.
vext01 added a commit to vext01/ykrustc_pv that referenced this pull request Aug 22, 2019
Discover additional DefIds by looking in the def_path_table.
The "definition path table" is a mapping from DefIds to definition
paths. There's an entry for every item than appears in the compiler's
metadata, thus making it an excellent discovery tool for our serialiser
code to find DefIds in dependent crates (if you run with `-Z
always-encode-mir`)!

A future change will remove the work list used in the serialised, as this
code supersedes it. That will be a fairly large change, so saving it for
another PR.

Another future change will enable `-Z always-encode-mir` by default for
ykrustc.

One of the tests we disabled here is being discussed upstream here:
rust-lang/rust#63742

When this is resolved, we might have to revisit this test in our fork.
vext01 added a commit to softdevteam/ykrustc that referenced this pull request Aug 22, 2019
Discover additional DefIds by looking in the def_path_table.
The "definition path table" is a mapping from DefIds to definition
paths. There's an entry for every item than appears in the compiler's
metadata, thus making it an excellent discovery tool for our serialiser
code to find DefIds in dependent crates (if you run with `-Z
always-encode-mir`)!

A future change will remove the work list used in the serialised, as this
code supersedes it. That will be a fairly large change, so saving it for
another PR.

Another future change will enable `-Z always-encode-mir` by default for
ykrustc.

One of the tests we disabled here is being discussed upstream here:
rust-lang/rust#63742

When this is resolved, we might have to revisit this test in our fork.
@hdhoang

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

ping from triage @michaelwoerister, any updates on this?

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Hopefully, @rust-lang/compiler will discuss this in the next triage meeting.

@joelpalmer

This comment has been minimized.

Copy link

commented Sep 9, 2019

Ping from Triage: Was this discussed in the @rust-lang/compiler Triage meeting? @michaelwoerister - Any updates?

@eddyb

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@michaelwoerister wrote in #63742 (comment):

I don't know a lot about generators but if the compiler re-orders code (in a debug build) as described above then that seems like a bug to me.

@vext01 Do you have any indication that there are any LLVM optimizations running? I would expect debuginfo tests would result in <optimized out> all over the place if we'd let LLVM optimize anything.

How sure are you that your pass doesn't actually move things around in the MIR?

EDIT: also, wouldn't a be accessed behind a pointer to the generator state? Can LLVM even optimize that sort of access right now? (I guess maybe after inlining everything)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.