Add flag to treat LLVM errors as fatal #77
Conversation
tamird
force-pushed
the
rewrite-IR
branch
2 times, most recently
from
6624b7d
to
a732161
Compare
src/llvm/mod.rs
Outdated
|
|
||
| LLVMPositionBuilderBefore(builder, instruction); | ||
| // Call instructions can't have a name; attempting to set one crashes LLVM. | ||
| let name = [0]; |
There was a problem hiding this comment.
nit: it means the same thing but might be more obvious if you wrote this ['\0']
There was a problem hiding this comment.
This should be more obvious now.
src/llvm/mod.rs
Outdated
|
|
||
| // Some debug info generated by rustc seems to trigger a segfault in the | ||
| // BTF code in llvm, so strip it until that is fixed | ||
| LLVMStripModuleDebugInfo(module); | ||
|
|
||
| // Collect up all the memory intrinsics. We're going to replace these with |
There was a problem hiding this comment.
nit: this whole thing to rewrite the memory intrinsics feels like it could belong in its own function.
There was a problem hiding this comment.
It could. What does it get us? I'm not a fan of indirection for its own sake.
There was a problem hiding this comment.
Ack. I think it can improve readability in so far as a human can summarize what the function does. By having the logic to do this rewriting inline, it becomes more difficult to internalize the code structure from a single pass. I am not religious here.
There was a problem hiding this comment.
This code is no longer in this PR.
|
I think adding the flag is good, but it should be disabled by default until the upstream LLVM conversation is resolved. I don't think we need to do the intrinsics => call change. The NewPM stuff should go in a separate PR. |
Cargo.toml
Outdated
| debug = true | ||
|
|
||
| [patch.crates-io] | ||
| aya-rustc-llvm-proxy = { git = "https://github.com/aya-rs/rustc-llvm-proxy.git" } |
There was a problem hiding this comment.
This needs to be removed before merging yeah?
There was a problem hiding this comment.
You tell me - would you like to cut a release, or should we just take a git dep on the thing?
There was a problem hiding this comment.
Pulled this out of this PR.
What's the resolution you're referring to? It seems that there's no way to not treat these errors as fatal and still avoid miscompilation.
Are you suggesting we string match on that error and ignore it?
Sure, it will. I wanted to demonstrate that the IR rewriting change prevents the miscompilation that otherwise happens. |
LLVM crashes happen relatively often during development, and we want to be able to unwind through Rust when they do.
Move conversions to Rust strings out.
This flag is set to false by default while we figure out which errors we see in practice.
This is apparently the contract we're supposed to uphold. Currently we sometimes miscompile things.