Skip to content

[Sanitizers][ABI] Remove too strong assert in asan_abi_shim #81696

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

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

wrotki
Copy link
Contributor

@wrotki wrotki commented Feb 14, 2024

Recently we enabled building the shim for arm64_32 arch. On this arch, sizeof(uptr) == sizeof(unsigned long) == 4 - so this assert will fail in runtime.

Need to just remove this assert

rdar://122927166

@llvmbot
Copy link
Member

llvmbot commented Feb 14, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Mariusz Borsa (wrotki)

Changes

Recently we enabled building the shim for arm64_32 arch. On this arch, sizeof(uptr) == sizeof(unsigned long) == 4 - so this assert will fail in runtime.

Need to just remove this assert

rdar://122927166


Full diff: https://github.com/llvm/llvm-project/pull/81696.diff

1 Files Affected:

  • (modified) compiler-rt/lib/asan_abi/asan_abi_shim.cpp (-1)
diff --git a/compiler-rt/lib/asan_abi/asan_abi_shim.cpp b/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
index 35c45dff96f6d2..523b322618b850 100644
--- a/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
+++ b/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
@@ -54,7 +54,6 @@ void *__asan_memmove(void *dest, const void *src, uptr n) {
 
 // Functions concerning RTL startup and initialization
 void __asan_init(void) {
-  static_assert(sizeof(uptr) == 8);
   static_assert(sizeof(u64) == 8);
   static_assert(sizeof(u32) == 4);
 

@thurstond
Copy link
Contributor

Suggestion: relax the assertion to sizeof(uptr) == 4 || sizeof(uptr) == 8 instead of removing it entirely

@usama54321
Copy link
Member

LGTM

@usama54321 usama54321 self-requested a review February 14, 2024 03:46
@wrotki wrotki requested a review from rsundahl February 14, 2024 03:49
Copy link
Contributor

@rsundahl rsundahl left a comment

Choose a reason for hiding this comment

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

Looks fine to me. It was predictive for this very moment, but then there was no good reason to assume anything about it's size in the first place so was an overly restrictive in the first place.

Recently we enabled building the shim for arm64_32 arch. On this arch, sizeof(uptr) == sizeof(unsigned long) == 4 - so this assert will fail in runtime.

Relaxing the assert per suggestion on the PR

rdar://122927166
@wrotki wrotki force-pushed the remove_invalid_assert branch from 7bc8143 to 5ec31f0 Compare February 14, 2024 03:55
@wrotki
Copy link
Contributor Author

wrotki commented Feb 14, 2024

Suggestion: relax the assertion to sizeof(uptr) == 4 || sizeof(uptr) == 8 instead of removing it entirely

Sure, will do that, thanks!

Suggestion: relax the assertion to sizeof(uptr) == 4 || sizeof(uptr) == 8 instead of removing it entirely

Did that, thanks for the suggestion!

@wrotki wrotki merged commit 3f738a4 into llvm:main Feb 14, 2024
@wrotki wrotki deleted the remove_invalid_assert branch February 14, 2024 04:14
wrotki added a commit to swiftlang/llvm-project that referenced this pull request Feb 15, 2024
Recently we enabled building the shim for arm64_32 arch. On this arch,
sizeof(uptr) == sizeof(unsigned long) == 4 - so this assert will fail in
runtime.

Need to just remove this assert

rdar://122927166

Co-authored-by: Mariusz Borsa <[email protected]>
(cherry picked from commit 3f738a4)
wrotki added a commit to swiftlang/llvm-project that referenced this pull request Feb 15, 2024
[Sanitizers][ABI] Remove too strong assert in asan_abi_shim (llvm#81696)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants