Improve miri's error reporting in check_in_alloc #57128

RalfJung · 2018-12-26T12:08:57Z

The check_in_alloc function currently takes a InboundsCheck for error reporting purposes. That's suboptimal for two reasons: (a) passing InboundsCheck::Live/MaybeDead sounds like it would affect the check that is performed, but it does not; and (b) the error still doesn't actually say what we tried to do with this pointer (access memory, in-bounds arithmetic, pointer equality test, ...).

The InboundsCheck argument should be replaced by something else, and that something else should also be put into the PointerOutOfBounds variant of EvalErrorKind. That could either be an &'static str or a more informative enum. I'd probably start with the string, since it seems unlikely we will want to use this for anything but displaying errors to the user, and it seems okay to not be able to use format!.

Cc @oli-obk

The text was updated successfully, but these errors were encountered:

FrankSD · 2018-12-30T05:27:10Z

Can i please take this up?I am a first timer contributor

FrankSD · 2018-12-30T05:44:10Z

I just forked and cloned rust-lang project on my local but my local branch folder rust/src/librustc/mir/interpret/ is missing pointer.rs and allocation.rs files.The pointer.rs file does contain the check_in_alloc function.It seems i am missing something basic here....my understanding all the code in the current master are uptodate.I clone the current master branch

RalfJung · 2018-12-30T18:58:56Z

@FrankSD sure, you can give it a try. Let's see if we can start by getting Rust to compile on your system :)

my local branch folder rust/src/librustc/mir/interpret/ is missing pointer.rs and allocation.rs files

What does git log say is the latest commit?

Also, did you maybe accidentally look into src/librustc_mir/interpret? Mind the _ vs /.

If you are on Linux/macOS, you can always do find -name pointer.rs:

$ find -name pointer.rs
./src/librustc/mir/interpret/pointer.rs

FrankSD · 2019-01-01T06:53:39Z

git log last commit:

 frank@frank-desktop ~/new-rust/rust $ git log
commit caed80ba4ba8d9f4d3fa8aa9af6c4092d779cd9d
Merge: ede5551 002f03b
Author: bors <bors@rust-lang.org>
Date:   Sun Aug 26 09:41:28 2018 +0000

   Auto merge of #53629 - nnethercote:lazier-SparseBitMatrix, r=nikomatsakis
   
   Lazier sparse bit matrix
   
   A small NLL win.
   
   r? @nikomatsakis

And find -name pointer.rs doesnt return anything

rank@frank-desktop ~/new-rust/rust $ find -name pointer.rs

RalfJung · 2019-01-01T12:01:41Z

Date: Sun Aug 26 09:41:28 2018 +0000

Your compiler is 4 months outdated. Try getting the latest version, it will have the files you are missing.

Maybe what happened is that you forked rustc 4 months ago, and then you cloned your fork. Forks don't get updated when the main rsutc repo moves along. You should always clone the main rustc repo, and only push to your fork for creating PRs.

FrankSD · 2019-01-01T20:12:08Z

Thank you@RalfJung....that make sense

FrankSD · 2019-01-02T06:04:54Z

It compiles fine and all the files i can see now....but it fails when trying to install. I have resolved most of the issues but trying to install bootstrap,did fails with :-
"

 "failed to run: /home/frank/new-rust/rust/build/bootstrap/debug/bootstrap install
Build completed unsuccessfully in 0:00:01

FrankSD · 2019-01-03T06:35:45Z

I have resolved the build issue.I am starting to check the Issue

RalfJung · 2019-01-03T10:47:33Z

Notice also that there is no reason to install your locally built Rust (I never did that, didn't even know it is possible). Why did you even want to do that?

FrankSD · 2019-01-03T13:11:05Z

I was following the official Rust Contribution guide where it mention do
x.py build && x.py install .

RalfJung · 2019-01-03T13:32:05Z

Could you send me the URL to that? At https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md it doesn't say anything about x.py install.

FrankSD · 2019-01-03T17:08:45Z

Sorry i think this was from the Readme.md https://github.com/rust-lang/rust/blob/master/README.md

RalfJung · 2019-01-04T13:39:58Z

Oh I see. That's instructions for installing Rust, not for doing development on rustc itself. The CONTRIBUTING.md I linked above is a better place for you to start.

FrankSD · 2019-01-04T22:08:10Z

@RalfJung.........Can you please guide on reproducing this Issue.?

RalfJung · 2019-01-05T12:58:51Z

I am not sure what you are asking. This issue is about refactoring check_in_alloc. Fixing this issue will have very minimal effect on the observable behavior of the compiler (some CTFE [compile-time function evaluation] error messages might change, but that's not even the goal).

So there's nothing to reproduce.

FrankSD · 2019-01-07T01:06:20Z

Got it.Sorry i am relatively new to rust so i am learning along the way.

RalfJung · 2019-01-09T12:23:32Z

So, do you understand what this issue is about, and what it takes to fix it?

FrankSD · 2019-01-09T18:16:13Z

I do understand what this issue is all about. Correct me if I am wrong, what I understand is: instead of using InboundsCheck type, use &'static str type to report errors. I think one thing I am still trying to understand is how "check_in_alloc function work" specifically when "PointerOutOfBounds" comes into a picture. So before I can touch any code I need to wrap my head around the big picture. As I said I am totally newbie to Rust... actually i am learning alot now. And I am excited to make my first Pull Request ever :). I will appreciate if you can enlighten me along the way. Thanks Sir.

RalfJung · 2019-01-12T13:58:18Z

Correct me if I am wrong, what I understand is: instead of using InboundsCheck type, use &'static str type to report errors

Yes. :)

Here's a somewhat more detailed list of steps to follow for this PR:

Replace InboundsCheck by &'static str here and here. Also rename the check field to something else, maybe msg or details.
You can now compile with ./x.py --stage 1 build src/libstd. This change will have a bunch of fallout, the rest of the PR is about making things compile again.
Some places will complain about the field rename, that should be clear from the error message.

Other places will be calling check_in_alloc with the wrong type for the last argument. Replace those by strings, messages explaining in what context this out-of-bounds pointer occurred -- was it doing a memory access, just pointer arithmetic, or what? If you don't know, put in some dummy text just to make it compile. We can fix that later :)

I hope this will get you started. Never hesitate to ask!

(I will be on a conference for the next 10 days, so I might not be able to reply before I come back. Feel free to ask anyone on Discord or Zulip or IRC as well.)

FrankSD · 2019-01-14T17:15:47Z

Thank you so much@RalfJung

FrankSD · 2019-01-24T05:59:18Z

Thank you for letting me know about Discord and Zulip..........

FrankSD · 2019-01-25T05:51:03Z

i have fixed most of the errors below is the only error i am still working on......Any idea why i am getting " expected identifier" Error
--> src/librustc/mir/interpret/pointer.rs:5:32
|
5 | AllocId, EvalResult, msg::&'static str="To be determined"
| ^ expected identifier

oli-obk · 2019-01-25T08:15:20Z

Looks like a rebase or search-and-replace gone wrong to me, can you open a PR so we can have a look at the code?

FrankSD · 2019-01-27T00:34:00Z

I dont think a rebase or search and replace gone wrong......... i am simply asking what is the correct rust syntax for ``` --> src/librustc/mir/interpret/pointer.rs:5:32 change?

RalfJung · 2019-01-27T10:20:44Z

That's hard to say without any context. Could you push this somewhere and point us to the right branch?

saleemjaffer · 2019-02-14T07:05:33Z

@RalfJung Can I take a shot at this issue? Newbie to rust.

RalfJung · 2019-02-14T08:16:31Z

@FrankSD what is your current status on this?

FrankSD · 2019-02-14T14:37:50Z

Sorry I am working on this..i hv been busy with my current job but I should submit a pull request shortly

…

On Feb 14, 2019 2:17 AM, "Ralf Jung" ***@***.***> wrote: @FrankSD <https://github.com/FrankSD> what is your current status on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFXIJCWPSVKIOZ3YyJ_oqhUJqTsXadRnks5vNRt_gaJpZM4ZhsUa> .

FrankSD · 2019-04-01T15:22:55Z

Sorry guys i have been so busy with my current work.Can someone please take this on?I have given it a short several times but i keep getting lots of errors on my branch.I would like to see when someone fixes this issue.Thanks for your understanding

LooMaclin · 2019-04-01T19:51:19Z

@FrankSD np, don't worry about this, i take it

LooMaclin · 2019-04-02T03:47:27Z

@RalfJung @oli-obk

&'static str doesn't implementing Decodable (parent enum marked as RustcDecodable)
so, I created enum that represents all messages and implementsDisplay
I found use of InboundsCheck in logic and it confused me: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/memory.rs#L442-L450

RalfJung · 2019-04-02T20:11:30Z

(I'm traveling, will come back to you next weekend.)

RalfJung · 2019-04-08T12:40:06Z

&'static str doesn't implementing Decodable (parent enum marked as RustcDecodable)

Ouch. @oli-obk any suggestions? I don't like the overhead of a new enum for this, that makes adding a new usage site very invoncenient.

I found use of InboundsCheck in logic and it confused me: /src/librustc_mir/interpret/memory.rs@master#L442-L450

Indeed the argument is meaningful for check_bounds_ptr and get_size_and_align, but not for check_in_alloc. So it should only be removed from the latter.

oli-obk · 2019-04-08T12:55:19Z

We can just use String. No need to be efficient in the error case.

oli-obk · 2019-04-08T12:56:03Z

Hmm... actually we do need to be efficient, since we catch errors. Alternatively we can use a Cow, but enums are the best choice here oterhwise.

@RalfJung

…error_reporting_in_check_in_alloc, r=RalfJung Improve miri error reporting in check_in_alloc Fixes rust-lang#57128 r? @RalfJung @oli-obk

@RalfJung

…ting_in_check_in_alloc, r=RalfJung Improve miri error reporting in check_in_alloc Fixes #57128 r? @RalfJung @oli-obk

RalfJung added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-const-eval Area: Constant evaluation (MIR interpretation) labels Dec 26, 2018

LooMaclin mentioned this issue Apr 2, 2019

Improve miri error reporting in check_in_alloc #59627

Merged

bors added a commit that referenced this issue May 27, 2019

Auto merge of #59627 - LooMaclin:issue_57128_improve_miri_error_repor…

1a56ec4

…ting_in_check_in_alloc, r=RalfJung Improve miri error reporting in check_in_alloc Fixes #57128 r? @RalfJung @oli-obk

bors closed this as completed in #59627 May 27, 2019

Improve miri's error reporting in check_in_alloc #57128

Improve miri's error reporting in check_in_alloc #57128

Comments

RalfJung commented Dec 26, 2018

FrankSD commented Dec 30, 2018

FrankSD commented Dec 30, 2018

RalfJung commented Dec 30, 2018 • edited Loading

FrankSD commented Jan 1, 2019

RalfJung commented Jan 1, 2019

FrankSD commented Jan 1, 2019

FrankSD commented Jan 2, 2019 • edited Loading

FrankSD commented Jan 3, 2019

RalfJung commented Jan 3, 2019

FrankSD commented Jan 3, 2019

RalfJung commented Jan 3, 2019

FrankSD commented Jan 3, 2019

RalfJung commented Jan 4, 2019

FrankSD commented Jan 4, 2019

RalfJung commented Jan 5, 2019

FrankSD commented Jan 7, 2019

RalfJung commented Jan 9, 2019

FrankSD commented Jan 9, 2019 • edited by ishitatsuyuki Loading

RalfJung commented Jan 12, 2019 • edited Loading

FrankSD commented Jan 14, 2019

FrankSD commented Jan 24, 2019

FrankSD commented Jan 25, 2019

oli-obk commented Jan 25, 2019

FrankSD commented Jan 27, 2019

RalfJung commented Jan 27, 2019 via email

saleemjaffer commented Feb 14, 2019

RalfJung commented Feb 14, 2019

FrankSD commented Feb 14, 2019 via email

FrankSD commented Apr 1, 2019

LooMaclin commented Apr 1, 2019

LooMaclin commented Apr 2, 2019 • edited Loading

RalfJung commented Apr 2, 2019

RalfJung commented Apr 8, 2019

oli-obk commented Apr 8, 2019

oli-obk commented Apr 8, 2019

RalfJung commented Dec 30, 2018 •

edited

Loading

FrankSD commented Jan 2, 2019 •

edited

Loading

FrankSD commented Jan 9, 2019 •

edited by ishitatsuyuki

Loading

RalfJung commented Jan 12, 2019 •

edited

Loading

LooMaclin commented Apr 2, 2019 •

edited

Loading