Miri: improve spans of required_const failures #75396

RalfJung · 2020-08-11T10:48:24Z

In #75339 I added a loop evaluating all consts required by a function body. Unfortunately, if one of their evaluations fails, then the span used for that was that of the first statement in the function body, which happened to work form some existing test but is not sensible in general.

This PR changes it to point to the whole function instead, which is at least not wrong.

r? @oli-obk

RalfJung · 2020-08-11T10:50:09Z

To do better, we'd have to either

pass a span through the error machinery (i.e., add a Span field to InterpError) which (if present) overwrites the span of the topmost frame, or
make it so that the frame knows it is currently evaluating a const, and can report its span.

In the light of this chaos, I am not sure I want to add yet another Span field to errors, and such "overwriting" behavior is often confusing. So if we think that we should try to do better, my proposal would be to replace this field of Frame

    pub loc: Option<mir::Location>,

by something of a type like Result<mir::Location, Span>, so that we can say what the span is when we are not currently evaluating a statement in that frame.

oli-obk · 2020-08-11T11:02:49Z

Yes, I also prefer that proposal. Although Result isn't the best type here, I'm not sure the situation warrants its own enum (though that could make it more obvious that something unusual is going on here). The current None situation could be represented by Err(DUMMY_SP)

RalfJung · 2020-08-11T11:10:25Z

Or rather, Err(self.body.span). Yes.

RalfJung · 2020-08-11T12:54:34Z

All right, I implemented that now.

oli-obk

r=me with nits

src/librustc_mir/interpret/eval_context.rs

src/librustc_mir/interpret/intrinsics/caller_location.rs

src/test/ui/consts/const-err-multi.stderr

RalfJung · 2020-08-12T06:50:58Z

@bors r=oli-obk

bors · 2020-08-12T06:50:59Z

📌 Commit 8fc28352e8f99df9a8fd965587ffccc3bd682dcd has been approved by oli-obk

RalfJung · 2020-08-12T07:22:23Z

@bors r-
investigating branch CI failure

RalfJung · 2020-08-12T07:58:17Z

Hu, a test failure but local runs don't show anything??

RalfJung · 2020-08-12T08:17:50Z

Ah looks like that test was changed recently even though it already existed before... very strange.

RalfJung · 2020-08-12T08:21:15Z

@oli-obk for some reason uninhabited-const-issue-61744.stderr changed a lot, but only after I rebased. No idea what is happening.

RalfJung · 2020-08-12T08:21:57Z

Also how horrible is GitHub? Why can I not go to the git log from https://github.com/rust-lang/rust/blob/d45dcb509527d847c7c3bf459a0e796801067b33/src/test/ui/consts/uninhabited-const-issue-61744.stderr ? I thought that used to be possible?

RalfJung · 2020-08-12T08:23:34Z

Hm, that test also did not change recently. So it must be some other change on the rustc side that makes it such that in combination with this PR, the output for that test changes. WTF.

RalfJung · 2020-08-12T08:27:51Z

Oh I see, this is a stack overflow, so it likely interacts with #75338.

RalfJung

(wow GitHub is really making my life as hard as possible today. now it started a review...)

RalfJung · 2020-08-12T08:29:21Z

src/librustc_mir/interpret/eval_context.rs

        M::after_stack_push(self)?;
+        self.frame_mut().loc = Ok(mir::Location::START);


It's about the order of these two lines: for errors that happen during after_stack_push, so we want to use the span of the first statement, or the span of the entire body? IMO the latter makes more sense, the first statement has nothing to do with this.

For the stack overflow, probably the best span to use would be that of the call site.

yea, the body's span is good here, though it's actually the call site's fault. But lets leave such refactorings (using the call site span) to the future, I remember trying something similar a while back and it just complicated the span scheme even more

Hm, this wouldn't even be that hard I think... we can just call cur_span before pushing the new frame, then we should get the call site.

It's just not clear to me that we always want the call site. The callee could also be responsible for the problem -- in fact I'd usually think they are.

at runtime the decision whose job it is to create the stack frame is an ABI decision, but that doesn't really make sense for CTFE. But I also think your alluding to a different thing? If an required_const will fail regardless of the concrete generic parameters, then that's indeed the function's fault and not the caller's

If an required_const will fail regardless of the concrete generic parameters, then that's indeed the function's fault and not the caller's

Yes, that is one possible scenario I had in mind where blaming the caller seems wrong.

Co-authored-by: Oliver Scherer <[email protected]>

bors · 2020-08-12T09:19:00Z

📌 Commit fd32fe9 has been approved by oli-obk

bors · 2020-08-12T10:59:03Z

⌛ Testing commit fd32fe9 with merge 5c8f1c241f7babd2356f080b77258174dcbf5da6...

rust-log-analyzer · 2020-08-12T11:14:32Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

Build completed successfully in 0:02:06
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
error: embedded-book maintainer @ryankurte is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @thejpster is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @korken89 is not assignable in the rust-lang/rust repo

  To be assignable, a person needs to be explicitly listed as a
  collaborator in the repository settings. The simple way to
  fix this is to ask someone with 'admin' privileges on the repo
  to add the person or whole team as a collaborator with 'read'
  privileges. Those privileges don't grant any extra permissions
  so it's safe to apply them.
The build will fail due to this.
== clock drift check ==
  local time: Wed Aug 12 11:14:19 UTC 2020
  network time: Wed, 12 Aug 2020 11:14:20 GMT
  network time: Wed, 12 Aug 2020 11:14:20 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (3081) (node)
Terminate orphan process: pid (3109) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

bors · 2020-08-12T11:16:04Z

💔 Test failed - checks-actions

RalfJung · 2020-08-12T11:17:08Z

error: embedded-book maintainer @ryankurte is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @thejpster is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @korken89 is not assignable in the rust-lang/rust repo

Seems spurious to me... I hope.^^
@bors retry

bors · 2020-08-12T11:20:40Z

⌛ Testing commit fd32fe9 with merge 0ac354c868c8238b48198d9bac75bd17719505e3...

rust-log-analyzer · 2020-08-12T11:35:44Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

Build completed successfully in 0:01:55
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
error: embedded-book maintainer @thejpster is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @korken89 is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @ryankurte is not assignable in the rust-lang/rust repo

  To be assignable, a person needs to be explicitly listed as a
  collaborator in the repository settings. The simple way to
  fix this is to ask someone with 'admin' privileges on the repo
  to add the person or whole team as a collaborator with 'read'
  privileges. Those privileges don't grant any extra permissions
  so it's safe to apply them.
The build will fail due to this.
== clock drift check ==
  local time: Wed Aug 12 11:35:32 UTC 2020
  network time: Wed, 12 Aug 2020 11:35:33 GMT
  network time: Wed, 12 Aug 2020 11:35:33 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (3052) (node)
Terminate orphan process: pid (3077) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

bors · 2020-08-12T11:37:07Z

💔 Test failed - checks-actions

RalfJung · 2020-08-12T11:49:52Z

@rust-lang/infra
second time in a row that I got

error: embedded-book maintainer @thejpster is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @korken89 is not assignable in the rust-lang/rust repo
error: embedded-book maintainer @ryankurte is not assignable in the rust-lang/rust repo

Any idea what is happening?
@bors retry

pietroalbini · 2020-08-12T11:52:04Z

@bors treeclosed=1000

pietroalbini · 2020-08-12T11:56:34Z

Fixing CI in #75450.

bors · 2020-08-12T20:44:24Z

⌛ Testing commit fd32fe9 with merge 576d27c...

bors · 2020-08-12T23:03:21Z

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 576d27c to master...

rust-highfive assigned oli-obk Aug 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020

RalfJung force-pushed the miri-spans branch from 5940ee3 to eb3d1ff Compare August 11, 2020 12:56

oli-obk requested changes Aug 11, 2020

View reviewed changes

src/librustc_mir/interpret/eval_context.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/eval_context.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/intrinsics/caller_location.rs Outdated Show resolved Hide resolved

oli-obk reviewed Aug 11, 2020

View reviewed changes

src/test/ui/consts/const-err-multi.stderr Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2020

only set frame location during push after preamble is done

5154b66

RalfJung force-pushed the miri-spans branch from 8fc2835 to d45dcb5 Compare August 12, 2020 08:20

RalfJung commented Aug 12, 2020

View reviewed changes

RalfJung and others added 4 commits August 12, 2020 10:48

miri: fall back to whole-function span when loc==None

d6c988b

bless tests

4b4d43f

more precise span for erroneous consts during CTFE/Miri

d21e011

fix typos

a505e77

Co-authored-by: Oliver Scherer <[email protected]>

RalfJung force-pushed the miri-spans branch from d45dcb5 to a5f51d3 Compare August 12, 2020 08:48

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 12, 2020

bors merged commit 576d27c into rust-lang:master Aug 12, 2020

bors mentioned this pull request Aug 12, 2020

Use tracing spans to trace the entire MIR interp stack #75143

Merged

RalfJung deleted the miri-spans branch August 13, 2020 07:18

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri: improve spans of required_const failures #75396

Miri: improve spans of required_const failures #75396

RalfJung commented Aug 11, 2020

RalfJung commented Aug 11, 2020

oli-obk commented Aug 11, 2020

RalfJung commented Aug 11, 2020

RalfJung commented Aug 11, 2020

oli-obk left a comment

RalfJung commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020 •

edited

Loading

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung left a comment

RalfJung Aug 12, 2020

oli-obk Aug 12, 2020

RalfJung Aug 12, 2020

RalfJung Aug 12, 2020

oli-obk Aug 12, 2020

RalfJung Aug 12, 2020

bors commented Aug 12, 2020

bors commented Aug 12, 2020

rust-log-analyzer commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

bors commented Aug 12, 2020

rust-log-analyzer commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

pietroalbini commented Aug 12, 2020

pietroalbini commented Aug 12, 2020

bors commented Aug 12, 2020

bors commented Aug 12, 2020

		M::after_stack_push(self)?;
		self.frame_mut().loc = Ok(mir::Location::START);

Miri: improve spans of required_const failures #75396

Miri: improve spans of required_const failures #75396

Conversation

RalfJung commented Aug 11, 2020

RalfJung commented Aug 11, 2020

oli-obk commented Aug 11, 2020

RalfJung commented Aug 11, 2020

RalfJung commented Aug 11, 2020

oli-obk left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020 • edited Loading

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung commented Aug 12, 2020

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Aug 12, 2020

Choose a reason for hiding this comment

oli-obk Aug 12, 2020

Choose a reason for hiding this comment

RalfJung Aug 12, 2020

Choose a reason for hiding this comment

RalfJung Aug 12, 2020

Choose a reason for hiding this comment

oli-obk Aug 12, 2020

Choose a reason for hiding this comment

RalfJung Aug 12, 2020

Choose a reason for hiding this comment

bors commented Aug 12, 2020

bors commented Aug 12, 2020

rust-log-analyzer commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

bors commented Aug 12, 2020

rust-log-analyzer commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020

pietroalbini commented Aug 12, 2020

pietroalbini commented Aug 12, 2020

bors commented Aug 12, 2020

bors commented Aug 12, 2020

RalfJung commented Aug 12, 2020 •

edited

Loading