cranelift: Add LibCalls to the interpreter #4782
Conversation
afonso360
force-pushed
the
interp-libcalls
branch
from
efc3f77 to
745be05
Compare
afonso360
force-pushed
the
interp-libcalls
branch
from
745be05 to
32cc761
Compare
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
afonso360
force-pushed
the
interp-libcalls
branch
from
34a940f to
7383c70
Compare
|
Huh. I was going to suggest that if there are libcalls with undefined behavior on some inputs then we should be limiting what we pass them to ensure that we only use them in ways consistent with the semantics we want. But I went to check how libcalls like These libcalls are constructed in Rust source:
While these are constructed in the x64 ISLE rules (but not any other backend?):
|
That sounds like a good idea.
Yeah, x86 doesn't have native instructions for these without extensions, so we have to lower them as libcalls if some extensions are disabled. I only know about AArch64 but they have equivalents for these in the base ISA. Not sure about s390x but i assume its the same We can emit something like I think the float libcalls don't trap at all. |
|
I've changed this in a few ways, and since we are going to change the input of the fuzzer we might as well do it properly and generate multiple libcalls, so we now do that. I've also removed the unused libcalls and a few accessory bits. |
There was a problem hiding this comment.
This looks great! I was briefly sad that Opcode::Call handling got so much more complicated, but it all looks like useful complexity to me, so 👍
When we're ready to merge fuzzgen changes, I'm in favor of this one. I do have a couple suggestions, but I'd also merge it as-is.
| .with_libcall_handler(&|libcall: LibCall, args: SmallVec<[DataValue; 1]>| { | ||
| use LibCall::*; | ||
| Ok(smallvec![match (libcall, &args[..]) { | ||
| (CeilF32, [DataValue::F32(a)]) => DataValue::F32(a.ceil()), | ||
| (CeilF64, [DataValue::F64(a)]) => DataValue::F64(a.ceil()), | ||
| (FloorF32, [DataValue::F32(a)]) => DataValue::F32(a.floor()), | ||
| (FloorF64, [DataValue::F64(a)]) => DataValue::F64(a.floor()), | ||
| (TruncF32, [DataValue::F32(a)]) => DataValue::F32(a.trunc()), | ||
| (TruncF64, [DataValue::F64(a)]) => DataValue::F64(a.trunc()), | ||
| _ => unreachable!(), | ||
| }]) | ||
| }); |
There was a problem hiding this comment.
What do you think of defining this function in function_generator.rs next to the array of allowed libcalls?
There was a problem hiding this comment.
The only reason I'm hesitant to do this is that fuzzgen is supposed to be executor agnostic (i.e. icache needs none of this), so it feels kinda weird to have a interpreter specific function there.
There was a problem hiding this comment.
Ah, that makes sense. But this is weird for the same reason that the function references are: only outer fuzz target knows which functions and which libcalls are okay to call. So I think the set of allowed libcalls (and functions) should be provided by the fuzz target. But I also don't think that needs to block merging this PR, it's just something I think we should revisit soon.
There was a problem hiding this comment.
So I think the set of allowed libcalls (and functions) should be provided by the fuzz target.
I like that!
But I also don't think that needs to block merging this PR, it's just something I think we should revisit soon.
👍
👋 Hey,
This does not fix any of the issues reported by the fuzzer so far, but I think that is just a coincidence, since at some point it will generate a libcall and we cannot handle those either.
Edit: Actually this may have already been reported, but have returned the same error as #4758 so it wasn't reported separately.
This allows the interpreter to register libcall handlers and invoke them when called.
I switched the fuzzer generated libcall from Udiv to Ishl, reason being that we don't want libcalls that can trap. Both functions have identical signatures so we shouldn't have any input format changes to the fuzzer.