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

fix: Disable gcc sanitizer on macos by default #255

Merged

Conversation

FeignClaims
Copy link
Contributor

According to this comment, gcc now dosen't support libsanitizer on macOS with arm64 and macOS Ventuna or later with x86_64.

@FeignClaims FeignClaims force-pushed the fix/disable_gcc_sanitizer_on_macos_by_default branch 2 times, most recently from 0fd0926 to 6337d19 Compare March 27, 2024 14:31
According to https://github.com/orgs/Homebrew/discussions/3384#discussioncomment-6264292,
gcc now dosen't support libsanitizer on macOS with arm64 and macOS Ventuna or later with x86_64.
@FeignClaims FeignClaims force-pushed the fix/disable_gcc_sanitizer_on_macos_by_default branch from 6337d19 to c41a9e8 Compare March 27, 2024 14:31
Copy link
Owner

@aminya aminya left a comment

Choose a reason for hiding this comment

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

But this issue didn't happen inside CI. We should be able to reproduce it to disable it?

@FeignClaims
Copy link
Contributor Author

But this issue didn't happen inside CI. We should be able to reproduce it to disable it?

The issue happens inside the DynamicProjectOptions.cmake, which sets UBSAN and ASAN for gcc by default (except for windows):

  if((CMAKE_CXX_COMPILER_ID MATCHES ".*Clang.*" OR CMAKE_CXX_COMPILER_ID MATCHES ".*GNU.*")
     AND WIN32
  )
   ...
 else()
   ...
 endif()

So use dynamic_project_options instead of project_options should trigger the issue, but I failed to figure out how to add such a test.

@FeignClaims FeignClaims requested a review from aminya April 1, 2024 06:26
@FeignClaims FeignClaims marked this pull request as draft April 1, 2024 13:01
@FeignClaims
Copy link
Contributor Author

FeignClaims commented Apr 1, 2024

Convert to a draft until

  • find a way to test this.
  • detect MacOS OS version to keep gcc sanitizers enable on x86_64 macOS Monterey or older.

@FeignClaims FeignClaims force-pushed the fix/disable_gcc_sanitizer_on_macos_by_default branch 4 times, most recently from c12844b to 040d9b1 Compare April 16, 2024 08:29
@FeignClaims FeignClaims force-pushed the fix/disable_gcc_sanitizer_on_macos_by_default branch from d97c0df to f5e7f3a Compare April 16, 2024 08:50
@FeignClaims
Copy link
Contributor Author

@aminya The issue happened in the new gcc macos-13 ci as expected.

- Make sanitizer enabling in dynamic_project_options consistent with
  detect_sanitizers_support.
- detect MacOS OS version to keep gcc sanitizers enable on x86_64 macOS Monterey or older.
@FeignClaims FeignClaims force-pushed the fix/disable_gcc_sanitizer_on_macos_by_default branch from 51c3eff to 6f25c7f Compare April 16, 2024 11:34
@FeignClaims FeignClaims marked this pull request as ready for review April 16, 2024 11:41
@FeignClaims
Copy link
Contributor Author

Ready to merge.

changes:

  • Disable gcc sanitizers on x86_64 MacOS 13+ and arm64 MacOS.
  • Add tests for this by adding a gcc macos-13 ci run and enabling sanitizers for it. (Sadly, I can only test arm64 MacOS munally as github actions dosen't support it.)
  • For consistency, use detect_sanitizers_support inside dynamic_project_options to detect whether enabling sanitizers or not.

@aminya
Copy link
Owner

aminya commented Apr 16, 2024

Thanks for the investigation. It would be great if you could create an issue for the upstream so that they can look into the problem.

@aminya aminya merged commit 4a8145d into aminya:main Apr 16, 2024
20 checks passed
@FeignClaims FeignClaims deleted the fix/disable_gcc_sanitizer_on_macos_by_default branch April 17, 2024 00:26
@FeignClaims
Copy link
Contributor Author

Although no related issue, it seems that the upstream already knows the issue and chooses not to support this for now (see iains/gcc-darwin-arm64@e722a1f).

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