Some fixes for try_call#10593
Conversation
Fixed miscompilationThe following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here): use std::ffi::CString;
fn main() {
let s = CString::new(b"ar");
panic!("{s:?}");
}Reduced so far to: pub struct MyCString {
inner: Vec<u8>,
}
#[inline(never)]
fn push(v: &mut Vec<u8>) {
v.push(0);
}
impl MyCString {
#[inline(never)]
pub fn new() -> MyCString {
let mut buffer = const { MyCString { inner: Vec::new() } };
push(&mut buffer.inner);
buffer
}
}
fn main() {
let s = MyCString::new();
panic!("{:?}", s.inner); // nul-terminator gets lost
}Edit: Pushed a commit to fix this miscompilation. |
fitzgen
left a comment
There was a problem hiding this comment.
LGTM, but we should have a test for the first commit before this merges. Thanks!
There was a problem hiding this comment.
This pass is used by cg_clif, I assume? We should probably have filetests for these changes, but I see that we don't have the infrastructure to run this pass on filetests at the moment, and I don't want to necessarily block on that. But longer term, we should either gain that functionality, so that we can hold it to our usual code standards, or else this pass should move into cg_clif (since I don't believe it relies on anything that isn't publicly exported).
There was a problem hiding this comment.
This pass is part of the normal optimization pipeline of Cranelift and runs unconditionally.
There was a problem hiding this comment.
Huh. rust-analyzer wasn't finding uses of it for me.
Do we exercise it under the test optimize filetest mode? If so, can you add a filetest for it?
There was a problem hiding this comment.
Context::optimize contains an unconditional call to self.eliminate_unreachable_code(isa)?; which in turn calls this function.
There was a problem hiding this comment.
As for testing, I'm not sure how to do that given that the clif ir printing doesn't show a list of all exception tables, but rather only shows them as part of a try_call instruction.
There was a problem hiding this comment.
I think that's correct -- I can't see a way to test for dangling exception tables either unless we change the print format. I think this is probably fine to merge as-is -- the other direction (avoiding false-removals of actually used exception tables) is the more important one and is implicitly tested.
7e9503e to
ffb3d8f
Compare
cfallin
left a comment
There was a problem hiding this comment.
LGTM on the additional commits -- just one thought on the verifier change below. I'm excited that you found the memory-fence issue and have things working with cg_clif now!
… used This way other consumers of Cranelift don't have to pay the cost of the way Wasmtime will implement unwinding on exceptions.
|
Looked over all commits and this looks good -- I'm going to do a short followup based on @bjorn3's |
Together with a change to allow the system_v call conv, this is enough to pass most of cg_clif's test suite (including panicking tests). In addition rustc's compiletest now works just fine in panic=unwind mode. Not all rustc tests pass yet, but the test failures seem to be caused by either cg_clif or bugs in the tests themself.
The second commit is not strictly necessary for correctness, but does make the optimizer work better with try_call.