Add fuzzing support for relaxed atomics #8253
Conversation
stevenfontanella
force-pushed
the
fuzzing
branch
from
cb21412 to
f4c43d5
Compare
stevenfontanella
force-pushed
the
fuzzing
branch
from
f4c43d5 to
2fa281d
Compare
| random(std::move(input), wasm.features), | ||
| atomicMemoryOrders(wasm.features.hasRelaxedAtomics() | ||
| ? std::vector{MemoryOrder::AcqRel, MemoryOrder::SeqCst} | ||
| : std::vector{MemoryOrder::SeqCst}), |
There was a problem hiding this comment.
Just as a matter of style, we don't do any of the other init stuff in the constructor, so perhaps it would be better lower down, unless it is special somehow? See e.g. loggableTypes below.
There was a problem hiding this comment.
Is there a reason for that style? I think it's better to prefer member initializers when possible, since it means that atomicMemoryOrders is immediately valid when constructed, and not just after the right line has been executed. It also saves us a default construction of a vector in this case, and allows us to make atomicMemoryOrders const (which I forgot to do, but I think it should be). The C++ core guidelines recommend this FWIW: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-initialize
There was a problem hiding this comment.
It is more efficient the other way, yes, but efficiency doesn't matter here: this is reached once in an invocation of the fuzzer (it likely takes 0.0001% of execution time).
And, see loggableTypes below: we have some conditions as to what to push there, which I think reads more easily as normal code rather than a more clever ? : operation.
With all that said, I wouldn't be opposed to changing this, if you feel strongly. What I'd rather not do is break the current consistency. Let's either land this using the old format, then update them all later, or update all the existing ones first before landing this?
Part of #8165. We already don't run v8 fuzz handlers for relaxed atomics (link) since this feature isn't implemented in V8 yet. To test, ran
fuzz_opt.pyfor ~1 hour with no failures.