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

Miri core engine: use throw_ub instead of throw_panic #66927

Merged
merged 2 commits into from
Dec 7, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 1, 2019

See #66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the InterpError::Panic variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2019
@rust-highfive

This comment has been minimized.

@wesleywiser
Copy link
Member

This is really great work cleaning up those error messages!

@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 1, 2019

📌 Commit 15f159a has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Dec 1, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors 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 Dec 1, 2019
}

#[inline]
fn signed_offset<'tcx>(&self, val: u64, i: i64) -> InterpResult<'tcx, u64> {
let (res, over) = self.overflowing_signed_offset(val, i128::from(i));
if over { throw_panic!(Overflow(mir::BinOp::Add)) } else { Ok(res) }
if over { throw_ub!(PointerArithOverflow) } else { Ok(res) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, so this is now specific enough to be equivalent to LLVM "inbounds GEP" overflowing, nice!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is the intention.

Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
Miri core engine: use throw_ub instead of throw_panic

See rust-lang#66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2019

@bors rollup=never
Let's not roll this up today as it'll likely break Miri again, and I'd like to have a nightly with Miri.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2019

@bors rollup-

Centril added a commit to Centril/rust that referenced this pull request Dec 7, 2019
Miri core engine: use throw_ub instead of throw_panic

See rust-lang#66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
@bors
Copy link
Contributor

bors commented Dec 7, 2019

⌛ Testing commit 15f159a with merge 517286a2264b49996c5c79e6a224250e0dd6ffc5...

@rust-highfive
Copy link
Collaborator

The job i686-mingw-1 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.
2019-12-07T09:30:37.4834926Z do so (now or later) by using -b with the checkout command again. Example:
2019-12-07T09:30:37.4834984Z 
2019-12-07T09:30:37.4835696Z   git checkout -b <new-branch-name>
2019-12-07T09:30:37.4835927Z 
2019-12-07T09:30:37.4836002Z HEAD is now at 517286a22 Auto merge of #66927 - RalfJung:engines-dont-panic, r=oli-obk
2019-12-07T09:30:37.5214624Z ##[section]Starting: Setup environment
2019-12-07T09:30:37.5320147Z ==============================================================================
2019-12-07T09:30:37.5320241Z Task         : Bash
2019-12-07T09:30:37.5320299Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-12-07T09:30:39.2429868Z BUILD_SOURCEBRANCHNAME=auto
2019-12-07T09:30:39.2429923Z BUILD_SOURCESDIRECTORY=D:\a\1\s
2019-12-07T09:30:39.2429997Z BUILD_SOURCEVERSION=517286a2264b49996c5c79e6a224250e0dd6ffc5
2019-12-07T09:30:39.2430077Z BUILD_SOURCEVERSIONAUTHOR=bors
2019-12-07T09:30:39.2430158Z BUILD_SOURCEVERSIONMESSAGE=Auto merge of #66927 - RalfJung:engines-dont-panic, r=oli-obk
2019-12-07T09:30:39.2430286Z CI_JOB_NAME=i686-mingw-1
2019-12-07T09:30:39.2430339Z COBERTURA_HOME=C:\cobertura-2.1.1
2019-12-07T09:30:39.2430419Z COMMONPROGRAMFILES=C:\Program Files\Common Files
2019-12-07T09:30:39.2430493Z COMMON_TESTRESULTSDIRECTORY=D:\a\1\TestResults
---
2019-12-07T09:30:39.2430781Z ChocolateyInstall=C:\ProgramData\chocolatey
2019-12-07T09:30:39.2430853Z ChromeWebDriver=C:\SeleniumWebDrivers\ChromeDriver
2019-12-07T09:30:39.2430918Z CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files
2019-12-07T09:30:39.2431004Z CommonProgramW6432=C:\Program Files\Common Files
2019-12-07T09:30:39.2431089Z ConstProp and ConstEval still use it, though. This will be addressed in future PRs.
2019-12-07T09:30:39.2431228Z ENDPOINT_URL_SYSTEMVSSCONNECTION=https://dev.azure.com/rust-lang/
2019-12-07T09:30:39.2431288Z EXEPATH=C:\Program Files\Git\bin
2019-12-07T09:30:39.2431372Z From what I can tell, all the error messages this removes are actually duplicates.
2019-12-07T09:30:39.2431434Z GCM_INTERACTIVE=Never
---
2019-12-07T09:30:39.2441312Z SYSTEM_TEAMPROJECTID=e71b0ddf-dd27-435a-873c-e30f86eea377
2019-12-07T09:30:39.2441774Z SYSTEM_TIMELINEID=3f3e4b70-f97a-4e68-afaa-e8b2a8366681
2019-12-07T09:30:39.2441858Z SYSTEM_TOTALJOBSINPHASE=16
2019-12-07T09:30:39.2441944Z SYSTEM_WORKFOLDER=D:\a
2019-12-07T09:30:39.2442118Z See https://github.com/rust-lang/rust/issues/66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.
2019-12-07T09:30:39.2442420Z TEMP=/tmp
2019-12-07T09:30:39.2442499Z TERM=cygwin
2019-12-07T09:30:39.2442579Z TF_BUILD=True
2019-12-07T09:30:39.2442644Z TMP=/tmp
---
2019-12-07T11:22:21.8187937Z 
2019-12-07T11:22:21.8188064Z 
2019-12-07T11:22:21.8906322Z failed to run: D:\a\1\s\build\bootstrap\debug\bootstrap test --exclude src/test/ui --exclude src/test/compile-fail
2019-12-07T11:22:21.8907085Z Build completed unsuccessfully in 1:42:05
2019-12-07T11:22:21.9561732Z make: *** [Makefile:89: ci-mingw-subset-1] Error 1
2019-12-07T11:22:22.0182798Z   local time: Sat Dec  7 11:22:22 CUT 2019
2019-12-07T11:22:22.5221818Z   network time: Sat, 07 Dec 2019 11:22:22 GMT
2019-12-07T11:22:22.5232211Z == end clock drift check ==
2019-12-07T11:22:22.6105885Z 
2019-12-07T11:22:22.6105885Z 
2019-12-07T11:22:23.0183985Z ##[error]Bash exited with code '2'.
2019-12-07T11:22:23.0802814Z ##[section]Starting: Checkout
2019-12-07T11:22:23.1583095Z ==============================================================================
2019-12-07T11:22:23.1583396Z Task         : Get sources
2019-12-07T11:22:23.1583465Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Dec 7, 2019

💔 Test failed - checks-azure

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 7, 2019
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2019
@wesleywiser
Copy link
Member

wesleywiser commented Dec 7, 2019

Actual error seems to be

Compiling compiler_builtins v0.1.18
error: failed to remove file `D:\a\1\s\build\i686-pc-windows-gnu\stage1-std\i686-pc-windows-gnu\release\deps\libcompiler_builtins-54fdf823ce6ac5da.rlib`

Caused by:
  Access is denied. (os error 5)
warning: build failed, waiting for other jobs to finish...

Seems spurious?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2019

@bors retry

@bors 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 Dec 7, 2019
@bors
Copy link
Contributor

bors commented Dec 7, 2019

⌛ Testing commit 15f159a with merge 5c5c8eb...

bors added a commit that referenced this pull request Dec 7, 2019
Miri core engine: use throw_ub instead of throw_panic

See #66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
@bors
Copy link
Contributor

bors commented Dec 7, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 5c5c8eb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 7, 2019
@bors bors merged commit 15f159a into rust-lang:master Dec 7, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #66927!

Tested on commit 5c5c8eb.
Direct link to PR: #66927

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Dec 7, 2019
Tested on commit rust-lang/rust@5c5c8eb.
Direct link to PR: <rust-lang/rust#66927>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
@RalfJung RalfJung deleted the engines-dont-panic branch December 8, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants