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 no longer builds after rust-lang/rust#95624 #95627

Closed
rust-highfive opened this issue Apr 3, 2022 · 22 comments · Fixed by #95815
Closed

miri no longer builds after rust-lang/rust#95624 #95627

rust-highfive opened this issue Apr 3, 2022 · 22 comments · Fixed by #95815
Assignees
Labels
A-miri Area: The miri tool C-bug Category: This is a bug.

Comments

@rust-highfive
Copy link
Collaborator

Hello, this is your friendly neighborhood mergebot.
After merging PR #95624, I observed that the tool miri no longer builds.
A follow-up PR to the repository https://github.com/rust-lang/miri is needed to fix the fallout.

cc @Dylan-DPC, do you think you would have time to do the follow-up work?
If so, that would be great!

@rust-highfive rust-highfive added A-miri Area: The miri tool C-bug Category: This is a bug. labels Apr 3, 2022
@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

#95035 makes this somewhat tricky, Miri will have to support more ways of using futex. (It supported enough for the thread parker implementation, but now it seems we are using more of that API.)

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

We support FUTEX_WAIT and FUTEX_WAKE, but not FUTEX_WAIT_BITSET.

@m-ou-se what would you say is a good place to learn about futex semantics, ideally suitable as a reference documentation that one can follow if one wants to do a reimplementation in an interpreter?

@m-ou-se
Copy link
Member

m-ou-se commented Apr 6, 2022

The man page for the Linux syscall is the best documentation/reference: https://man7.org/linux/man-pages/man2/futex.2.html

FUTEX_WAIT_BITSET shouldn't be hard to implement. I can work on Miri support for it. :)

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

That would be great. :)

Best to start with ./rustup-toolchain 306ba8357fb36212b7d30efb9eb9e41659ac1445 (rather than the usual ./rustup-toolchain HEAD) as otherwise you will have to also deal with other breakage that happened after the futex change.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 6, 2022

This adds support for FUTEX_WAIT_BITSET and FUTEX_WAKE_BITSET in Miri: rust-lang/miri#2054

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

The next trouble is #95469 -- somehow this broke printing to stdout in Miri on Windows targets, which now just loops infinitely. @ChrisDenton any idea what might be going on?

@ChrisDenton
Copy link
Member

ChrisDenton commented Apr 7, 2022

I made a quick summary in #95759 but still need to investigate properly. Short version is that in some cases we're passing asynchronous handles to child processes, which is unsound when read synchronously (but mostly seemed to work in practice).

Though I'm not actually sure why that should make it loop infinitely since it's calling std::sys::abort_internal?

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022 via email

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022 via email

@ChrisDenton
Copy link
Member

Oh right! Hm... I'm not really sure why that be. The changes are limited to reading and writing from a handle.

@ChrisDenton
Copy link
Member

I had another look and as far I can tell there's no code path that doesn't call either NtReadFile or NtWriteFile. The only thing of note it does before that is call c::IO_STATUS_BLOCK::default().

Just to be clear, you're totally certain it was that PR that caused the issue?

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022 via email

@ChrisDenton
Copy link
Member

Then I'll admit to being stumped. 😕

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

Does the Windows stdout/stderr go through a "handle" or is it a separate code path?

@ChrisDenton
Copy link
Member

ChrisDenton commented Apr 7, 2022

Ah, I did recently change piped stdout/stderr to use a loop (in #95467). But it also uses new functions to do that (WriteFIleEx and SleepEx).

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

But Miri works fine on bbe9d27 which includes that PR...

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

Oh, I think it's pretty simple -- NtWriteFile is declared via compat_fn, and in Miri that function is just not available. Then what happens is that it panics, which triggers printing to stderr, so it tries calling NtWriteFile again, and the loop continues.

It doesn't make a lot of sense for NtWriteFile to be a compat_fn when the "fallback" branch consists in an infinite loop on any write to stdout/stderr...

@ChrisDenton
Copy link
Member

Oh! Sorry! I did that without even thinking and it didn't even occur to me look at that again. The fallback should probably just abort or return an error. I'll make a PR.

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

Uff, that compat stuff is using some nasty things which Miri would all have to support. :/

Is there any chance these functions could be called like regular 'extern fn', without all that compat craziness? I doubt we are making stdin/stdout optional so for all intents and purposes those functions have to exist for Rust programs to run on Windows.

@ChrisDenton
Copy link
Member

The problem is these Nt functions really should be loaded at runtime. I guess we could technically do it at load time but we'd at least need to wait until the raw_dylib feature is stabilized because we can't necessarily count on users having the right import library.

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2022

An alternative might be a hack like this: #95775.
Unfortunately I don't have the time to implement wild linker stuff in Miri for a platform that Miri only barely supports anyway...

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 7, 2022
…isDenton

make windows compat_fn (crudely) work on Miri

With rust-lang#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay.

Cc rust-lang#95627 `@ChrisDenton`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 8, 2022
…Denton

make windows compat_fn (crudely) work on Miri

With rust-lang#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay.

Cc rust-lang#95627 `@ChrisDenton`
@RalfJung
Copy link
Member

RalfJung commented Apr 8, 2022

Miri CI finally passes again. 🎉

This took a lot more time than I actually have, so I think next time Windows breaks in obscure ways I will have to just disable the tests for that platform and hope someone else can maintain that code.

@RalfJung RalfJung mentioned this issue Apr 8, 2022
@bors bors closed this as completed in 340f649 Apr 9, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
make windows compat_fn (crudely) work on Miri

With rust-lang/rust#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay.

Cc rust-lang/rust#95627 `@ChrisDenton`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants