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

Band-aid fix to stop race conditions in llvm errors #45170

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

aidanhs
Copy link
Member

@aidanhs aidanhs commented Oct 10, 2017

This is a big hammer, but should be effective at completely removing a
few issues, including inconsistent error messages and segfaults when
LLVM workers race to report results

LLVM_THREAD_LOCAL has been present in LLVM since 8 months before 3.7
(the earliest supported LLVM version that Rust can use)

Maybe fixes #43402 (third time lucky?)

r? @alexcrichton


You can see that in

pub fn llvm_err(handler: &errors::Handler, msg: String) -> FatalError {
match llvm::last_error() {
Some(err) => handler.fatal(&format!("{}: {}", msg, err)),
None => handler.fatal(&msg),
}
}
pub fn write_output_file(
handler: &errors::Handler,
target: llvm::TargetMachineRef,
pm: llvm::PassManagerRef,
m: ModuleRef,
output: &Path,
file_type: llvm::FileType) -> Result<(), FatalError> {
unsafe {
let output_c = path2cstr(output);
let result = llvm::LLVMRustWriteOutputFile(
target, pm, m, output_c.as_ptr(), file_type);
if result.into_result().is_err() {
let msg = format!("could not write output to {}", output.display());
Err(llvm_err(handler, msg))
} else {
Ok(())
}
}
}
there's a small window where the static global error message (made thread local in this PR) could be altered by another thread.

Note that we can't use thread_local because gcc 4.7 (permitted according to the readme) does not support it.

Maybe ideally all the functions should be modified to not use a global, but this PR makes things deterministic at least. My only hesitation is whether errors are checked in different threads to where they occur, but I figure that's probably unlikely (and is less bad than racing code).

As an aside, segfault evidence before this patch when I was doing some debugging:

$ while grep 'No such file or directory' log2; do RUST_LOG=debug ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc -o "" y.rs >log2 2>&1; done
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
Segmentation fault (core dumped)
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory

This is a big hammer, but should be effective at completely removing a
few issues, including inconsistent error messages and segfaults when
LLVM workers race to report results

LLVM_THREAD_LOCAL has been present in LLVM since 8 months before 3.7
(the earliest supported LLVM version that Rust can use)

Maybe fixes #43402 (third time lucky?)
@alexcrichton
Copy link
Member

@bors: r+

Oh dear, that should definitely be a thread local!

@bors
Copy link
Contributor

bors commented Oct 10, 2017

📌 Commit 8628d51 has been approved by alexcrichton

@alexcrichton
Copy link
Member

@bors: p=1

@bors
Copy link
Contributor

bors commented Oct 10, 2017

⌛ Testing commit 8628d51 with merge ec016f8...

bors added a commit that referenced this pull request Oct 10, 2017
…al, r=alexcrichton

Band-aid fix to stop race conditions in llvm errors

This is a big hammer, but should be effective at completely removing a
few issues, including inconsistent error messages and segfaults when
LLVM workers race to report results

`LLVM_THREAD_LOCAL` has been present in LLVM since 8 months before 3.7
(the earliest supported LLVM version that Rust can use)

Maybe fixes #43402 (third time lucky?)

r? @alexcrichton

------

You can see that in https://github.com/rust-lang/rust/blob/5f578dfad0dd5d43b28eff71a7e857d10c3f55fe/src/librustc_trans/back/write.rs#L75-L100 there's a small window where the static global error message (made thread local in this PR) could be altered by another thread.

Note that we can't use `thread_local` because gcc 4.7 (permitted according to the readme) does not support it.

Maybe ideally all the functions should be modified to not use a global, but this PR makes things deterministic at least. My only hesitation is whether errors are checked in different threads to where they occur, but I figure that's probably unlikely (and is less bad than racing code).

As an aside, segfault evidence before this patch when I was doing some debugging:
```
$ while grep 'No such file or directory' log2; do RUST_LOG=debug ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc -o "" y.rs >log2 2>&1; done
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
Segmentation fault (core dumped)
error: could not write output to : No such file or directory
error: could not write output to : No such file or directory
```
@bors
Copy link
Contributor

bors commented Oct 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ec016f8 to master...

@bors bors merged commit 8628d51 into master Oct 10, 2017
@aidanhs aidanhs deleted the aphs-no-unsynchronised-llvm-err-global branch October 10, 2017 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious failure in issue-26092
3 participants