Skip to content
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.

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

Enable more sanitizers in make develop #1588

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Dec 30, 2024

  • -fsanitize=undefined encompasses multiple checks we were specifying
  • "detect_leaks=1" for __asan_default_options checks for memory leaks (except for with macOS clang++, which does not support LSan)
  • -fsanitize=float-divide-by-zero is an extra UBSan check (and reveals a UB bug to fix with fixed-point DIV and LOG)

@Rangi42 Rangi42 added tests This affects the test suite builds This affects the build process or release artifacts labels Dec 30, 2024
@Rangi42 Rangi42 added this to the 0.9.1 milestone Dec 30, 2024
@Rangi42 Rangi42 requested a review from ISSOtm December 30, 2024 15:35
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

clang: error: unsupported option '-fsanitize=leak' for target 'arm64-apple-darwin23.6.0'

Hmm. Does BSD make have a way to not pass this flag for that compiler? (Or we can just start using gmake... 🥺)

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

Actually, can't we just leave out -fsanitize=leak entirely? Because -fsanitize=address already checks for memory leaks (at least if we pass ASAN_OPTIONS=detect_leaks=1), as well as more things like OOB writes.

ISSOtm
ISSOtm previously requested changes Dec 30, 2024
CMakeLists.txt Outdated Show resolved Hide resolved
@Rangi42 Rangi42 changed the title Use -fsanitize=leak and -fsanitize=undefined Use -fsanitize=undefined and ASAN_OPTIONS=detect_leaks=1 Dec 30, 2024
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

-fsanitize=undefined enables "All of the checks listed above other than float-divide-by-zero, unsigned-integer-overflow, implicit-conversion, local-bounds and the nullability-* group of checks." I've enabled float-divide-by-zero, local-bounds, and nullability, but not unsigned-integer-overflow or implicit-conversion because those are technically defined behavior and I'm not sure if we actually rely on them or not.

Edit: never mind:

g++: error: unrecognized argument to ‘-fsanitize=’ option: ‘local-bounds’
g++: error: unrecognized argument to ‘-fsanitize=’ option: ‘nullability’

Edit 2: g+ would also not support unsigned-integer-overflow or implicit-conversion, so they're moot anyway.

Edit 3: Also, I was right, we do rely on them. When I locally build make CXX=clang++ develop and ran the asm tests, -fsanitize=unsigned-integer-overflow and -fsanitize=implicit-conversion complained about a lot of things. They're both well-defined behaviors, so there's no real need for us to check for them, even though those checks exist for "suspicious behavior". ("Issues caught by these sanitizers are not undefined behavior, but are often unintentional.")

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

@ISSOtm We have no good solution for conditionally enabling flags based on the OS/compiler in the Makefile, because we're committed to supporting the super-limited BSD/POSIX make. However, it's trivial in cmake. Given that enabling -fsanitize=float-divide-by-zero already just caught a UB issue, I'd quite like to enable more of the checks that -fsanitize=undefined doesn't cover, on the platforms that support those checks. Would it be acceptable for the cmake build to potentially check more things than the make build does?

test/run-tests.sh Outdated Show resolved Hide resolved
@Rangi42 Rangi42 force-pushed the sanitizers branch 2 times, most recently from abdd51d to dee0cc4 Compare December 31, 2024 04:23
@Rangi42 Rangi42 changed the title Use -fsanitize=undefined and ASAN_OPTIONS=detect_leaks=1 Enable more sanitizers in make develop Dec 31, 2024
- `-fsanitize=undefined` encompasses multiple checks we were specifying

- "detect_leaks=1" for `__asan_default_options` checks for memory leaks
  (except for with macOS clang++, which does not support LSan)

- `-fsanitize=float-divide-by-zero` is an extra UBSan check
  (and reveals a UB bug to fix with fixed-point `DIV` and `LOG`)
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Seems good! Thank you.

@ISSOtm ISSOtm merged commit 8363f25 into gbdev:master Dec 31, 2024
23 checks passed
@Rangi42 Rangi42 deleted the sanitizers branch December 31, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds This affects the build process or release artifacts tests This affects the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants