Skip to content

Fix UBSan on CI #7173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlively merged 11 commits into from
Dec 21, 2024
Merged

Fix UBSan on CI #7173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
198dfb2
Update flake8 and fix errors
tlively Dec 20, 2024
6c0b3b6
Fix UBSan on CI
tlively Dec 20, 2024
12f6bea
fix type checks
tlively Dec 20, 2024
6730c4f
Merge branch 'fix-flake8' into fix-ubsan
tlively Dec 20, 2024
bfd980b
more
tlively Dec 20, 2024
14dc36f
Merge branch 'main' into fix-ubsan
tlively Dec 20, 2024
71d38b4
Merge branch 'main' into fix-ubsan
tlively Dec 20, 2024
72d32b3
exclude MSVC
tlively Dec 20, 2024
093dcbc
try something else
tlively Dec 20, 2024
6613592
fix
tlively Dec 20, 2024
9cc8a82
avoid UB calling std::abs
tlively Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/tools/wasm-ctor-eval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,30 +462,30 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
const size_t MaximumMemory = 100 * 1024 * 1024;

// TODO: handle unaligned too, see shell-interface
template<typename T> T* getMemory(Address address, Name memoryName) {
void* getMemory(Address address, Name memoryName, size_t size) {
auto it = memories.find(memoryName);
assert(it != memories.end());
auto& memory = it->second;
// resize the memory buffer as needed.
auto max = address + sizeof(T);
auto max = address + size;
if (max > memory.size()) {
if (max > MaximumMemory) {
throw FailToEvalException("excessively high memory address accessed");
}
memory.resize(max);
}
return (T*)(&memory[address]);
return &memory[address];
}

template<typename T> void doStore(Address address, T value, Name memoryName) {
// do a memcpy to avoid undefined behavior if unaligned
memcpy(getMemory<T>(address, memoryName), &value, sizeof(T));
// Use memcpy to avoid UB if unaligned.
memcpy(getMemory(address, memoryName, sizeof(T)), &value, sizeof(T));
}

template<typename T> T doLoad(Address address, Name memoryName) {
// do a memcpy to avoid undefined behavior if unaligned
// Use memcpy to avoid UB if unaligned.
T ret;
memcpy(&ret, getMemory<T>(address, memoryName), sizeof(T));
memcpy(&ret, getMemory(address, memoryName, sizeof(T)), sizeof(T));
return ret;
}

Expand Down
6 changes: 6 additions & 0 deletions src/wasm/literal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,8 +978,14 @@ Literal Literal::neg() const {
Literal Literal::abs() const {
switch (type.getBasic()) {
case Type::i32:
if (i32 == std::numeric_limits<int32_t>::min()) {
return *this;
}
return Literal(std::abs(i32));
case Type::i64:
if (i64 == std::numeric_limits<int64_t>::min()) {
return *this;
}
Copy link
Member

@kripken kripken Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, wasm doesn't have integer min, does it? Unless this is for SIMD somehow?

Copy link
Member

@kripken kripken Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@kripken kripken Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@tlively tlively Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an integer abs SIMD instruction, and we run a spec test for its behavior that shows this is WAI.

Copy link
Member

@kripken kripken Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sgtm then. Meanwhile I verified all uses of integer abs in OptimizeInstructions are valid.

return Literal(std::abs(i64));
case Type::f32:
return Literal(i32 & 0x7fffffff).castToF32();
Expand Down
Loading