Stream: t-compiler

Topic: Create attribute to delay_span_bug


traxys (Oct 30 2019 at 15:35, on Zulip):

@mw eddyb told me to ask you would have more clues than him on this behavior. to make a test for issue #65401, I changed the attribute rustc_error to be able to take rustc_error(delayed) that calls tcx.sess.delay_span_bug(tcx.def_span(def_id), "compilation successful");, but when trying to compile twice some code with this attribute is does not exhibit the issue the snippet eddyb provided showed, do you have any idea why ?

pnkfelix (Oct 30 2019 at 15:51, on Zulip):

hmm maybe the problem is more subtle, in that not every delay_span_bug is hidden?

pnkfelix (Oct 30 2019 at 15:51, on Zulip):

I don't know if I'll have time to give your code a spin, but if I manage to allocate more time I'll try.

traxys (Oct 30 2019 at 15:58, on Zulip):

The check_for_rustc_errors_attr is called in the first line of codegen_crate in src/librustc_codegen_ssa/base.rs, could it mean that no codegen is done ?

mw (Oct 30 2019 at 15:58, on Zulip):

@traxys does the test case containing the attributes generate any other errors that might "shadow" the delayed bug?

mw (Oct 30 2019 at 15:59, on Zulip):

make sure you are compiling a library with some code in it.

traxys (Oct 30 2019 at 16:06, on Zulip):

the test case is

#![feature(rustc_attrs)]

#[rustc_error(delayed)]
fn main () {}
traxys (Oct 30 2019 at 16:14, on Zulip):

@mw @nikomatsakis: @eddyb suggested that the problem is that check_for_rustc_errors_attr is properly not behind a query, and told me to ask you what should be the proper way to do it

mw (Oct 31 2019 at 08:55, on Zulip):

It does not look like queries should have an influence on this.

mw (Oct 31 2019 at 08:56, on Zulip):

I don't see the test case in your PR. Do you use revisions?

mw (Oct 31 2019 at 08:57, on Zulip):

i.e. the test needs to be compiled twice in order for the problem to show

traxys (Oct 31 2019 at 10:09, on Zulip):

I am compiling this twice by hand, calling the built rustc. The problem is that the attribute I wrote in the PR does not trigger the issue even though it triggers a delay_span_bug

mw (Oct 31 2019 at 10:28, on Zulip):

did you make sure to use incremental compilation? when using rustcdirectly, you have to pass the -Cincremental commandline arg explicitly.

mw (Oct 31 2019 at 10:29, on Zulip):

both invocations must specify the same incremental cache directory.

mw (Oct 31 2019 at 10:30, on Zulip):

as in:

# invocation 1
rustc -Cincremental=/tmp/xyz lib.rs

# invocation 2
rustc -Cincremental=/tmp/xyz lib.rs
pnkfelix (Oct 31 2019 at 10:33, on Zulip):

maybe the better place to start is: @traxys , you are able to observe the bug as originally documented, right?

pnkfelix (Oct 31 2019 at 10:33, on Zulip):

(in other words, I don't want to be asking them to check (metaphorically) if their computer is plugged in if there's obvious evidence that it is)

traxys (Oct 31 2019 at 10:41, on Zulip):

Yes with the snippet provided by eddyb

traxys (Oct 31 2019 at 10:50, on Zulip):

(Also I think I have some correct code for turning the function into a query, should I put in the PR ?)

eddyb (Oct 31 2019 at 10:55, on Zulip):

@mw without a query the delay_span_bug would run every time, but inside a query, the query would be cached on the first run

traxys (Oct 31 2019 at 10:56, on Zulip):

@mw yes ../build/x86_64-unknown-linux-gnu/stage1/bin/rustc error.rs -Cincremental=foo is exactly the invocation
I do it twice, and it fails, but with a correct snippet I see the issue

traxys (Oct 31 2019 at 10:57, on Zulip):

@eddyb I am unsure of the code I wrote turning it into a query, but it still does not exihibit the issue

eddyb (Oct 31 2019 at 10:57, on Zulip):

can you push to a branch, or gist a diff?

eddyb (Oct 31 2019 at 10:57, on Zulip):

@traxys something very important is that the query shouldn't have eval_always

eddyb (Oct 31 2019 at 10:58, on Zulip):

because then I think you get the behavior that the query is never cached

eddyb (Oct 31 2019 at 10:59, on Zulip):

tbh at this point it might be good to just fix the issue and not add a test - this is about ICEs, after all

eddyb (Oct 31 2019 at 10:59, on Zulip):

seems like producing an ICE with just the right problems attached to it is hard

traxys (Oct 31 2019 at 11:01, on Zulip):

I have really few ideas on what I did, I have no knowledge of the query structure. https://github.com/traxys/rust/tree/rustc_error_delayed but the three last commits are what I did, sorry for the poor names it is my testing branch

eddyb (Oct 31 2019 at 11:01, on Zulip):

if you use git gui then you can switch it to amend mode and keep modifying the last commit, to avoid needing many small commits

eddyb (Oct 31 2019 at 11:02, on Zulip):

@traxys that looks fine

eddyb (Oct 31 2019 at 11:03, on Zulip):

I wonder if you need cache_on_disk_if { true } or something

traxys (Oct 31 2019 at 11:04, on Zulip):

I there somwhere all the attributes are described ?

traxys (Oct 31 2019 at 11:08, on Zulip):

(I'll have the results in twenty minutes when rustc finishes compiling, I am not near my best computer, I need to setup SSH on it)

traxys (Oct 31 2019 at 11:10, on Zulip):

Wouldn't it also need load_cached(tcx, something) ?

eddyb (Oct 31 2019 at 11:22, on Zulip):

some queries only have cache_on_disk_if, I think the load_cached is only for unsupported types or something

traxys (Oct 31 2019 at 11:52, on Zulip):

@eddyb It worked ! It shows the bug

mw (Oct 31 2019 at 11:55, on Zulip):

Oh, right we want the delayed bug to happen in something that gets skipped in the second run...

traxys (Oct 31 2019 at 11:57, on Zulip):

Does the tcx.span_err stay between runs, or did it break the bare #[rustc_error] for incremental compilation ?

mw (Oct 31 2019 at 11:59, on Zulip):

the behavior of the regular #[rustc_error] should not change

mw (Oct 31 2019 at 11:59, on Zulip):

so it still needs to be triggered outside of a query

traxys (Oct 31 2019 at 12:01, on Zulip):

That would bring code duplication wouldn't it ? Because to check for #[rustc_error(delay_span_bug)] I first need to find the rustc_error and then check if there is the delay_span_bug ident

mw (Oct 31 2019 at 12:01, on Zulip):

You should also be able to use tcx.ensure().check_for_rustc_errors_attr() instead of tcx.check_for_rustc_errors_attr(). Then the query result does not need to be cached on disk.

mw (Oct 31 2019 at 12:04, on Zulip):

That would bring code duplication wouldn't it ? Because to check for #[rustc_error(delay_span_bug)] I first need to find the rustc_error and then check if there is the delay_span_bug ident

finding the attribute can still happen outside of any query. It's just the triggering of delay_span_bug that needs to happen inside the query

traxys (Oct 31 2019 at 12:05, on Zulip):

So a query named rustc_error_delay_span_bug for example ? That just calls delay_span_bug ?

mw (Oct 31 2019 at 12:05, on Zulip):

so I would:

mw (Oct 31 2019 at 12:07, on Zulip):

then this code can just call the trigger_delay_span_bug query:
https://github.com/traxys/rust/commit/31a7cc84d038f355860d2e627ef14104f0432335#diff-9a36a3f0dd5416d91736e46c7c1cb614R45

traxys (Oct 31 2019 at 12:07, on Zulip):

I had renamed to delay_span_bug (eddyb told it made more sense because span_bug was not called before), so delay_span_bug_from_query on delayed_from_inside_query ?

mw (Oct 31 2019 at 12:08, on Zulip):

delay_span_bug_from_inside_query sounds good

mw (Oct 31 2019 at 12:08, on Zulip):

it's fine if it's a verbose name

mw (Oct 31 2019 at 12:08, on Zulip):

better to be explicit

traxys (Oct 31 2019 at 12:08, on Zulip):

I don't see many other uses for it than this test

mw (Oct 31 2019 at 12:09, on Zulip):

for some context about "ensuring" a query:

mw (Oct 31 2019 at 12:10, on Zulip):

it's the same as calling a query but then throwing away the result

mw (Oct 31 2019 at 12:10, on Zulip):

you only use it when you really only care about the side effects of a query (like emitting errors)

mw (Oct 31 2019 at 12:11, on Zulip):

(hm, now I wonder if this would actually work in that case, let me take a look...)

mw (Oct 31 2019 at 12:14, on Zulip):

yes, it should work

traxys (Oct 31 2019 at 12:17, on Zulip):

what should be the query input ? the CrateNum ?

mw (Oct 31 2019 at 12:18, on Zulip):

yes, CrateNum is fine

eddyb (Oct 31 2019 at 12:37, on Zulip):

btw, @mw I was thinking we should have a LocalCrate ZST that we use instead of CrateNum for queries where it's always LOCAL_CRATE (I'd rather not use () because that somewhat obscures the intent)

eddyb (Oct 31 2019 at 12:38, on Zulip):

kinda like LocalDefId (which I'm using to replace DefIndex in a few places as we speak)

mw (Oct 31 2019 at 12:39, on Zulip):

Yes, that has been suggested before I think it's a good idea

eddyb (Oct 31 2019 at 12:39, on Zulip):

it's on my "at some point in the next decade" TODO list

Zoxc (Oct 31 2019 at 13:41, on Zulip):

My PRs which moves the queries earlier added a LocalCrate type, but I changed it to () =P

traxys (Oct 31 2019 at 13:41, on Zulip):

Where should the test be ? src/test/ui/throw_incorrect_artifacts.rs ?

mw (Oct 31 2019 at 13:45, on Zulip):

src/test/incremental/delayed_span_bug.rs maybe?

traxys (Oct 31 2019 at 13:52, on Zulip):

Also the //~ ERROR annotation won't work with an ICE won't it ? I'll need to manually specify stderr ? (Not very fluent with tests)

eddyb (Oct 31 2019 at 14:10, on Zulip):

there should be some ICE tests already, probably added by @oli to test some CTFE-related edge cases

eddyb (Oct 31 2019 at 14:11, on Zulip):

just grep for part of the ICE message (some part that always shows up) in src/test/ui

traxys (Oct 31 2019 at 14:27, on Zulip):

the only tests that match stderr are the ui tests ? So I should just check for any error in both passes in incremental ?

traxys (Oct 31 2019 at 14:34, on Zulip):
// revisions: cfail1 fail2
// rustc-env:RUST_BACKTRACE=0
// failure-status: 101
// should-fail

#![feature(rustc_attrs)]

#[rustc_error(delay_span_bug)]
fn main() {}

This is my test so far, but the problem is that is passes on a branch where the issue is not fixed, as though the incremental state is not saved
Should I pass a flag -Cincremental=foo ?

eddyb (Oct 31 2019 at 14:36, on Zulip):

oh huh I didn't think about incremental vs UI

traxys (Oct 31 2019 at 14:46, on Zulip):

I think the problem here is that should-fail does not check that both revision fail, only the first one

traxys (Oct 31 2019 at 14:54, on Zulip):

I see a lot of rustc_partition_reused, is it needed ?

traxys (Oct 31 2019 at 21:15, on Zulip):

I checked the code for cfail, and it checks that there was no panic, should I add a header to disable this check ?

traxys (Nov 01 2019 at 14:26, on Zulip):

After looking at the doc and code of compiletest I really see no other way than to add either a revision kind cice or a flag along the lines of // expect-ice

pnkfelix (Nov 01 2019 at 14:28, on Zulip):

hmm. You cannot use error-pattern here?

pnkfelix (Nov 01 2019 at 14:28, on Zulip):

that is sort of our catch-all when you cannot make a nice UI test...

traxys (Nov 01 2019 at 14:41, on Zulip):

I'll look I'm very unfamilliar with the test harness

traxys (Nov 01 2019 at 14:43, on Zulip):

(Because it is not a UI but incremental test)

traxys (Nov 01 2019 at 14:47, on Zulip):

The problem I have I think is this function

 fn run_cfail_test(&self) {
         let proc_res = self.compile_test();
         self.check_if_test_should_compile(&proc_res);
         self.check_no_compiler_crash(&proc_res);
...
pnkfelix (Nov 01 2019 at 14:48, on Zulip):

ah, and this is definitely a compiler "crash"

traxys (Nov 01 2019 at 14:48, on Zulip):

Yes

traxys (Nov 01 2019 at 14:49, on Zulip):

So I would think I would need a way to disable a part of the check if a certain header is present

pnkfelix (Nov 01 2019 at 14:50, on Zulip):

By the way, @traxys

pnkfelix (Nov 01 2019 at 14:50, on Zulip):

i think @eddyb suggested yesterday that we might want to land the effect of your PR without a test

pnkfelix (Nov 01 2019 at 14:50, on Zulip):

because the change is important enough that we may not want to delay on resolving the questions surrounding how to test it properly

traxys (Nov 01 2019 at 14:51, on Zulip):

Yes, so I should put the commits for creating the test in another PR ?

traxys (Nov 01 2019 at 14:51, on Zulip):

And let this one be merged ?

traxys (Nov 01 2019 at 14:51, on Zulip):

(the bug fix)

pnkfelix (Nov 01 2019 at 14:52, on Zulip):

I think so. @eddyb , did I summarize your viewpoint accurately?

eddyb (Nov 01 2019 at 14:53, on Zulip):

yeah, we can land the very tiny fix even if we have no way to test it

traxys (Nov 01 2019 at 14:54, on Zulip):

How do I revert properly the test commits on the PR ? with a local git revert and then rebase ?

eddyb (Nov 01 2019 at 14:55, on Zulip):

testing around ICEs is cool/interesting in general, but I doubt it's as important

eddyb (Nov 01 2019 at 14:55, on Zulip):

@traxys it's simpler than that

eddyb (Nov 01 2019 at 14:55, on Zulip):

you can git checkout some-commit-hash -b new-branch-name

traxys (Nov 01 2019 at 14:56, on Zulip):

The commit hash being the place I want the PR to be at ?

eddyb (Nov 01 2019 at 14:56, on Zulip):

or git checkout HEAD~15 -b new-branch-name if you want to make a new branch without the last 15 commits in it

traxys (Nov 01 2019 at 14:57, on Zulip):

Ok so I need to switch the PR from one branch to another ?

eddyb (Nov 01 2019 at 14:57, on Zulip):

wait, do you want to make a new PR without the test changes, or do you want the current one to not have the test changes?

traxys (Nov 01 2019 at 14:57, on Zulip):

The second one I would think, but I could leave them in and wait for the PR to be merged to implement the rest of the test

eddyb (Nov 01 2019 at 14:58, on Zulip):

you can also do git branch new-branch-name to save the current branch to a new name, and then you can do whatever you want with the current branch

eddyb (Nov 01 2019 at 14:58, on Zulip):

oh, since you mentioned revert+rebase: there's a simpler way to do that thing too: in the file git rebase -i opens up you can just remove some of the pick lines

eddyb (Nov 01 2019 at 14:59, on Zulip):

(or comment them out I think?)

eddyb (Nov 01 2019 at 14:59, on Zulip):

and that will mean those commits will be gone after the rebase

eddyb (Nov 01 2019 at 14:59, on Zulip):

sorry for offering several options, and I didn't even mention git reset :P

pnkfelix (Nov 01 2019 at 15:00, on Zulip):

git reset --hard, even ...

eddyb (Nov 01 2019 at 15:00, on Zulip):

(git is really bad at having one single good way to do any one common task, sadly)

traxys (Nov 01 2019 at 15:00, on Zulip):

I never worked with a large project on git so I never had to use those options before x)

traxys (Nov 01 2019 at 15:07, on Zulip):

I removed the changes for the test and put them in another branch, @mw suggested that the fix is good

eddyb (Nov 01 2019 at 15:11, on Zulip):

testing it by hand on that one ICE that's not fixed yet (or any others that might be hinted at in src/test/ui), should be good

traxys (Nov 01 2019 at 18:59, on Zulip):

So for creating the test should I add a flag to disable the panic test ? like // should-ice ?

traxys (Nov 01 2019 at 19:00, on Zulip):

or // error-as-ice ?

Last update: Nov 16 2019 at 01:50UTC