afonso360 opened PR #4467 from fuzzgen-float
to main
:
:wave: Hey,
This PR adds support for generating floats in fuzzgen and adds a bunch of instructions.
I think we have all the float ops, except bit ops that operate on floats.
So far this has found #4446 and #4460. We can merge this before #4460 but it will probably start crashing in oss-fuzz.
afonso360 edited PR #4467 from fuzzgen-float
to main
:
:wave: Hey,
This PR adds support for generating floats in fuzzgen and adds a bunch of instructions.
I think we have all the float ops, except bit ops that operate on floats.
So far this has found #4446 and #4460. We can merge this before #4460 but it will probably start crashing in oss-fuzz.
cc: @jameysharp
jameysharp requested jameysharp for a review on PR #4467.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
The second
args[1]
should beargs[2]
.It'd be nice to not duplicate all this code for each arity. Looking through
InstBuilder
, I see there's aMultiAry
case. Can that replace theNullAry
/Unary
/Binary
/Ternary
split?
afonso360 submitted PR review.
afonso360 created PR review comment:
Good Catch! I missed the existence of
MultiAry
but it looks like a much better solution. I'll replace it.
afonso360 updated PR #4467 from fuzzgen-float
to main
.
jameysharp submitted PR review.
cfallin created PR review comment:
Hmm, I'm surprised that this works, and I suspect it does only accidentally: we should be building the argument list in the pool owned by the
DataFlowGraph
. TheValueList
is just an index into the pool, soValueList
s need to be built with the proper pool and will silently fail if referring to another pool. (This is a kind of unfortunate logical-unsafety but the type system doesn't give us a way to tie the list to a particular pool.)
cfallin submitted PR review.
afonso360 updated PR #4467 from fuzzgen-float
to main
.
afonso360 submitted PR review.
afonso360 created PR review comment:
Fixed. I wonder how the fuzzer didn't immediately catch this, I only ran it for a couple of minutes after making this change, but I would have expected it to fail instantly.
Thanks!
jameysharp created PR review comment:
Thanks for fixing the use of
ValueList
! I think you don't need a dependency oncranelift-entity
now, right?
jameysharp submitted PR review.
afonso360 updated PR #4467 from fuzzgen-float
to main
.
afonso360 submitted PR review.
afonso360 created PR review comment:
Right, Fixed!
jameysharp has enabled auto merge for PR #4467.
jameysharp has disabled auto merge for PR #4467.
afonso360 updated PR #4467 from fuzzgen-float
to main
.
jameysharp has enabled auto merge for PR #4467.
jameysharp merged PR #4467.
Last updated: Dec 23 2024 at 13:07 UTC