Stream: t-compiler

Topic: target_feature 1.1 #69274


Hanna Kruppe (Mar 07 2020 at 15:04, on Zulip):

Hey @LeSeulArtichaut, I'd like to help move that PR forward. The error you're currently stuck on is very weird. It'll be a while before I can poke at it myself, in the mean time can you tell me:

LeSeulArtichaut (Mar 07 2020 at 15:06, on Zulip):

Oh, hello @Hanna Kruppe, thanks for reaching out. I'm currently trying to debug this by manually adding the flags rusttest uses to try to find something interesting

LeSeulArtichaut (Mar 07 2020 at 15:07, on Zulip):

The rustc invocation I'm using is this one:

rustc +stage1 src/test/ui/rfcs/rfc-2396-target_feature-11/trait-impl.rs

Where stage1 is a toolchain I set up as the rustc-dev-guide recommands

LeSeulArtichaut (Mar 07 2020 at 15:07, on Zulip):

The ./x.py invocation I'm using is:

./x.py test -i --stage 1 src/test/ui/rfcs/rfc-2396-target_feature-11/trait-impl.rs
LeSeulArtichaut (Mar 07 2020 at 15:09, on Zulip):

Aha, I think I narrowed it. Using --emit metadata makes the test compile!

Hanna Kruppe (Mar 07 2020 at 15:10, on Zulip):

Huh

Hanna Kruppe (Mar 07 2020 at 15:11, on Zulip):

Ohhh, the check is in codegen_fn_attrs which is not run when not generating code!

Hanna Kruppe (Mar 07 2020 at 15:13, on Zulip):

It might make more sense in librustc_passes/check_attr.rs

LeSeulArtichaut (Mar 07 2020 at 15:14, on Zulip):

Alright, I'll come back in 1 hour once my computer has painfully compiled rustc :big_smile:

LeSeulArtichaut (Mar 07 2020 at 15:14, on Zulip):

More seriously though, thanks @Hanna Kruppe, I'd never have found out this detail myself

Hanna Kruppe (Mar 11 2020 at 08:58, on Zulip):

How did it go, @LeSeulArtichaut? Anything else I can do to help?

LeSeulArtichaut (Mar 11 2020 at 12:32, on Zulip):

Something still feels wrong to me. Take the error when #[target_feature] is applied to a safe function as it is today. It is emitted when using --emit metadata, even though it is located in codegen_fn_attrs

Hanna Kruppe (Mar 11 2020 at 12:43, on Zulip):

Oh, hm, you're right. I missed this call which forces codegen_fn_attrs to be computed in any case https://github.com/rust-lang/rust/blob/2917d993023dec5111147a1552ec78b206a5a37e/src/librustc_passes/check_attr.rs#L81

Hanna Kruppe (Mar 11 2020 at 12:43, on Zulip):

:thinking:

Hanna Kruppe (Mar 11 2020 at 12:44, on Zulip):

Now that I'm looking more carefully, there are also a few other places in type checking etc. that query the codegen attrs

Hanna Kruppe (Mar 11 2020 at 12:50, on Zulip):

Sanity check: does this debug! never execute with --emit metadata but prints something without --emit metadata?

LeSeulArtichaut (Mar 11 2020 at 16:53, on Zulip):

That's correct

LeSeulArtichaut (Mar 11 2020 at 16:58, on Zulip):

(Poke @Hanna Kruppe)

Hanna Kruppe (Mar 11 2020 at 17:09, on Zulip):

So I just realized that the aforementioned call to codegen_fn_attrs only applies to free functions, not methods. I'm seeing a few other places that I would expect to call it anyway (1 2), but IMO it's worth trying if adding a || target == Target::Method in check_attr.rs helps

LeSeulArtichaut (Mar 11 2020 at 19:39, on Zulip):

This seems to work. I ran ./x.py test on the whole test suite and got no other error than minor UI breakages related to the feature

LeSeulArtichaut (Mar 11 2020 at 19:39, on Zulip):

I've pushed my local branch to update the PR

LeSeulArtichaut (Apr 23 2020 at 16:57, on Zulip):

Hi @Hanna Kruppe, I've seen @NeoRaider's PR (#71205) to resolve the issue with attributes on closures, so I wondered if I could rebase my PR on his. What do you think?

Hanna Kruppe (Apr 23 2020 at 17:01, on Zulip):

I don't mind. It potentially means your PR will wait a little longer (that other PR doesn't seem to move very quickly either), but I guess it also depends on when you rebase and when I get the time to look at it again for the final r+.

LeSeulArtichaut (Apr 23 2020 at 17:21, on Zulip):

If I’m the only one concerned, I don’t mind that the PR will wait longer. The reason why I’d like to rebase is because of the UI failures that I had encountered, and that will probably be fixed by #71205.

LeSeulArtichaut (Apr 23 2020 at 17:21, on Zulip):

I think I’ll try to rebase locally and see where that leads me

Hanna Kruppe (Apr 23 2020 at 19:27, on Zulip):

Makes sense :+1:

LeSeulArtichaut (Apr 23 2020 at 20:18, on Zulip):

Maybe I can help push #71205 in return? :D

NeoRaider (Apr 23 2020 at 20:26, on Zulip):

Well, I think it is waiting for review, or someone who understands why my change fixes more things than expected? It's been lying there for a week, so I assume it will be triaged soon anyways

LeSeulArtichaut (Apr 25 2020 at 16:28, on Zulip):

Rebased :slight_smile:

LeSeulArtichaut (Apr 30 2020 at 16:47, on Zulip):

Hopefully this now passes the CI tests :fingers_crossed:

Last update: May 29 2020 at 18:00UTC