Fixing UB on Windows for trampoline allocations

[zacklj89]

Fixing the type of the constant to avoid undefined behavior with respect to overflow.

[llvmbot]

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

Author: Zack Johnson (zacklj89)

Changes

This is actually used on Windows, so removing UNUSED as well as fixing the type of the constant to avoid undefined behavior with respect to overflow.

---

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

1 Files Affected:

• (modified) compiler-rt/lib/interception/interception_win.cpp (+1-1)

diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index 1829358705fe82..3602b24401a3a1 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -339,7 +339,7 @@ struct TrampolineMemoryRegion {
   uptr max_size;
 };
 
-UNUSED static const uptr kTrampolineScanLimitRange = 1 << 31;  // 2 gig
+static const uptr kTrampolineScanLimitRange = 1ull << 31;  // 2 gig
 static const int kMaxTrampolineRegion = 1024;
 static TrampolineMemoryRegion TrampolineRegions[kMaxTrampolineRegion];
 

[mstorsjo]

This is actually used on Windows, so removing UNUSED

No, it's not used in i386 mode. It's used in some modes, but not all - therefore it is marked as unused to silence warnings in those build configuration. See 979c38c which introduced the attribute here.

[zacklj89]

This is actually used on Windows, so removing UNUSED

No, it's not used in i386 mode. It's used in some modes, but not all - therefore it is marked as unused to silence warnings in those build configuration. See 979c38c which introduced the attribute here.

Then shouldn't it be guarded by #if SANITIZER_WINDOWS64 rather than UNUSED? When I was debugging I was confused by the presence of UNUSED.

[mstorsjo]

This is actually used on Windows, so removing UNUSED

No, it's not used in i386 mode. It's used in some modes, but not all - therefore it is marked as unused to silence warnings in those build configuration. See 979c38c which introduced the attribute here.

Then shouldn't it be guarded by #if SANITIZER_WINDOWS64 rather than UNUSED? When I was debugging I was confused by the presence of UNUSED.

That's certainly an option too. IIRC I proposed that originally, but avoiding ifdefs and marking it unused was suggested here: https://reviews.llvm.org/D91852#inline-861716

[mstorsjo]

LGTM, thanks!

(At this point, it'd be good to update the commit description, which ends up as the commit message, to not refer to removing UNUSED any longer. Or whoever does the merge can do that fixup of the commit message before finishing the merge.)