Skip to content

CMake: Add -fsanitize-undefined-trap-on-error to MaxSan configuration#146

Merged
ednolan merged 1 commit intobemanproject:mainfrom
ednolan:enolan_ubsantrap1
Mar 16, 2025
Merged

CMake: Add -fsanitize-undefined-trap-on-error to MaxSan configuration#146
ednolan merged 1 commit intobemanproject:mainfrom
ednolan:enolan_ubsantrap1

Conversation

@ednolan
Copy link
Copy Markdown
Member

@ednolan ednolan commented Mar 16, 2025

Previously, the following implementation of identity would pass all tests under MaxSan:

struct identity {
    // Returns `t`.
    template <class T>
    constexpr T&& operator()(T&& t) const noexcept {
        int x = std::numeric_limits<int>::max();
        ++x;
        return std::forward<T>(t);
    }

    using is_transparent = __is_transparent;
};

This is because UndefinedBehaviorSanitizer would diagnose the integer overflow by printing, but would not abort the program:

[ RUN      ] IdentityTest.check_is_transparent
bemanproject/exemplar/include/beman/exemplar/identity.hpp:37:7: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
[       OK ] IdentityTest.check_is_transparent (0 ms)

After this change, the test correctly fails, with SIGILL:

The following tests FAILED:
          1 - IdentityTest.call_identity_with_int (ILLEGAL)

Previously, the following implementation of identity would pass all
tests under MaxSan:

```
struct identity {
    // Returns `t`.
    template <class T>
    constexpr T&& operator()(T&& t) const noexcept {
        int x = std::numeric_limits<int>::max();
        ++x;
        return std::forward<T>(t);
    }

    using is_transparent = __is_transparent;
};
```

This is because UndefinedBehaviorSanitizer would diagnose the integer
overflow by printing, but would not abort the program:

```
[ RUN      ] IdentityTest.check_is_transparent
bemanproject/exemplar/include/beman/exemplar/identity.hpp:37:7: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
[       OK ] IdentityTest.check_is_transparent (0 ms)
```

After this change, the test correctly fails, with SIGILL:

```
The following tests FAILED:
          1 - IdentityTest.call_identity_with_int (ILLEGAL)
```
Copy link
Copy Markdown
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

Good catch!

@ednolan ednolan merged commit 7af8ed3 into bemanproject:main Mar 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants