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

Normalize column numbers #1626

Closed
wants to merge 1 commit into from
Closed

Conversation

est31
Copy link
Member

@est31 est31 commented Nov 15, 2020

@est31
Copy link
Member Author

est31 commented Nov 15, 2020

I can't run miri tests with ./x.py test --stage 1 src/tools/miri because I get an error that 1.50.0-dev is not a valid Rust version number. So no idea whether it fixes the issue pointed out here: rust-lang/rust#79002 (comment)

Edit: for some reason it works now. I've confirmed that it fixes the bug.

@RalfJung
Copy link
Member

Thanks for helping with Miri!

Needed by rust-lang/rust#79002

Note that it's fine to land a PR that breaks Miri, and then fix Miri separately.

So I don't think we need to support both the case with and without column numbers at the same time. I'd prefer to instead adjust normalization to expect column numbers to be present. Also column numbers in backtrace-std.rs should not be normalized away; we want to make sure they are correct instead.

@est31
Copy link
Member Author

est31 commented Nov 15, 2020

@RalfJung note that it did break the build of the rollup instead of just going through with it and then setting toolstate: rust-lang/rust#79002 (comment) . Not sure what's broken, but maybe the issue is that tests broke instead of just the compile?

So this PR is needed I think.

Also note that on Windows, column numbers are being omitted in debuginfo and thus also won't show up in the backtrace (see rust-lang/rust#42921). Thus I think the test may not require column numbers to be present.

@RalfJung
Copy link
Member

It broke just the PR CI of the rollup, not the bors build, right? I am pretty sure it would have worked fine in bors. Miri is allowed to break, and something is seriously wrong if that does not work.

Also note that on Windows, column numbers are being omitted in debuginfo and thus also won't show up in the backtrace (see rust-lang/rust#42921). Thus I think the test may not require column numbers to be present.

Miri uses its own way to obtain debug info. I would be very surprised if what you said is true in Miri.

@est31
Copy link
Member Author

est31 commented Nov 15, 2020

@RalfJung alright then I'll change the PR to assume column numbers, and change rust-lang/rust#79002 to remove the miri submodule update.

@est31 est31 changed the title Support also normalizing column numbers Normalize column numbers Nov 15, 2020
@RalfJung RalfJung added the S-blocked-on-rust Status: Blocked on landing a Rust PR label Nov 15, 2020
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge this once rust-lang/rust#79002 lands. (rust-version will need updating then but I can do that.)

@RalfJung RalfJung mentioned this pull request Nov 19, 2020
@RalfJung
Copy link
Member

Two things broke at once, so I had to fix them together in #1630. I made your PR part of that. Thanks for helping fixing Miri!

@RalfJung RalfJung closed this Nov 19, 2020
bors added a commit that referenced this pull request Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked-on-rust Status: Blocked on landing a Rust PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants