Fix UBSan on CI #7173
Fix UBSan on CI #7173
Conversation
The flake8 we were running on CI was too old and began giving spurious errors about the uninterpreted contents of f-strings. Update to the latest flake8 and fix all the new errors, including the previously incorrect comment syntax in the .flake8 file. Also remove scripts/storage.py, since it didn't seem to be used for anything we currently need.
The UBSan builder started failing with an error about a misaligned store in wasm-ctor-eval.cpp. The store was already done via `memcpy` to avoid alignment issues, but apparently this is no longer enough. Add a cast to `(void*)` to further avoid giving the impression of guaranteed alignment.
|
Wooo it works! @kripken PTAL at the latest state. |
| case Type::i64: | ||
| if (i64 == std::numeric_limits<int64_t>::min()) { | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Wait, wasm doesn't have integer min, does it? Unless this is for SIMD somehow?
There was a problem hiding this comment.
Oh, it's not a wasm instruction, it's some internal computation over literals... in that case we don't have a spec to compare against. Why is returning the (negative) literal the right value for abs here? (I would guess maxint)
There was a problem hiding this comment.
Out of curiosity, I found a testcase that uses this,
(func $optimize-boolean (type $1) (param $0 i32) (param $1 i64) (result i32)
(select
(i32.const 0)
(i32.const 0)
(i32.rem_s
(local.get $0)
(i32.const 0)
)
)
)--optimize-instructions ends up doing abs on an integer here.
Reading the pass source, I'm not sure if this is a bug or not.
If you can confirm this PR doesn't change the outcome, that sounds ok, but I guess it's UB so we aren't sure it was consistent before?
There was a problem hiding this comment.
There is an integer abs SIMD instruction, and we run a spec test for its behavior that shows this is WAI.
There was a problem hiding this comment.
Ok, sgtm then. Meanwhile I verified all uses of integer abs in OptimizeInstructions are valid.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Any idea why this broke, btw? Compiler update on CI maybe?
|
Yeah they updated the latest ubuntu image recently, and I think it brought in subtly different versions of various things. |
The UBSan builder started failing with an error about a misaligned store
in wasm-ctor-eval.cpp. The store was already done via
memcpyto avoidalignment issues, but apparently this is no longer enough. Use
void*as the destination type to further avoid giving the impression of
guaranteed alignment.
Also fix UB when executing std::abs on minimum negative integers in
literal.cpp.