[asan] Limit priority of ctor to kMax-1

[vitalybuka]

Reserve maximal availibe priority to
runtime. We need to run code in sanitizer
runtime after all C++ constructors.
https://clang.llvm.org/docs/AttributeReference.html#constructor

[llvmbot]

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

@llvm/pr-subscribers-llvm-transforms

Author: Vitaly Buka (vitalybuka)

Changes

Reserve maximal availibe priority to
runtime. We need to run code in sanitizer
runtime after all C++ constructors.
https://clang.llvm.org/docs/AttributeReference.html#constructor

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+24)
• (modified) llvm/test/Instrumentation/AddressSanitizer/instrument_late_initializer.ll (+1-1)

diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 9fb1df7ab2b79..734cbd0515ea8 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -133,6 +133,7 @@ const char kAsanModuleDtorName[] = "asan.module_dtor";
 static const uint64_t kAsanCtorAndDtorPriority = 1;
 // On Emscripten, the system needs more than one priorities for constructors.
 static const uint64_t kAsanEmscriptenCtorAndDtorPriority = 50;
+static const uint64_t kMaxCtorAndDtorPriority = 65535;
 const char kAsanReportErrorTemplate[] = "__asan_report_";
 const char kAsanRegisterGlobalsName[] = "__asan_register_globals";
 const char kAsanUnregisterGlobalsName[] = "__asan_unregister_globals";
@@ -1993,6 +1994,29 @@ void ModuleAddressSanitizer::createInitializerPoisonCalls(
       poisonOneInitializer(*F, ModuleName);
     }
   }
+  assert(ClInitializers);
+  updateGlobalCtors(M, [](Constant *C) -> Constant * {
+    ConstantStruct *CS = dyn_cast<ConstantStruct>(C);
+    if (!CS)
+      return C;
+    auto *Priority = cast<ConstantInt>(CS->getOperand(0));
+    if (Priority->getLimitedValue() != kMaxCtorAndDtorPriority)
+      return C;
+    // As optimization, runtime needs to execute callback just after all
+    // constructors. We going to set priority to the max allowed value. However,
+    // the default constructor priorily is already max, so as-is we will not be
+    // able to guaranty desired order. So reduce the priority by one to reserve
+    // max value for the constructor in runtime.
+    StructType *EltTy = cast<StructType>(CS->getType());
+    Constant *CSVals[3] = {
+        ConstantInt::getSigned(Priority->getType(),
+                               kMaxCtorAndDtorPriority - 1),
+        CS->getOperand(1),
+        CS->getOperand(2),
+    };
+    return cast<ConstantStruct>(
+        ConstantStruct::get(EltTy, ArrayRef(CSVals, EltTy->getNumElements())));
+  });
 }
 
 const GlobalVariable *
diff --git a/llvm/test/Instrumentation/AddressSanitizer/instrument_late_initializer.ll b/llvm/test/Instrumentation/AddressSanitizer/instrument_late_initializer.ll
index 45d526a42c9f7..2225ccb217208 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/instrument_late_initializer.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/instrument_late_initializer.ll
@@ -9,7 +9,7 @@ target triple = "x86_64-unknown-linux-gnu"
 @g = internal global i32 0, align 4, sanitize_address_dyninit  ; With dynamic initializer.
 
 ;.
-; CHECK: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__late_ctor, ptr null }, { i32, ptr, ptr } { i32 1, ptr @asan.module_ctor, ptr @asan.module_ctor }]
+; CHECK: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65534, ptr @__late_ctor, ptr null }, { i32, ptr, ptr } { i32 1, ptr @asan.module_ctor, ptr @asan.module_ctor }]
 ;.
 ; NOINIT: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__late_ctor, ptr null }, { i32, ptr, ptr } { i32 1, ptr @asan.module_ctor, ptr @asan.module_ctor }]
 ;.

[MaskRay]

asan changing the priority of a llvm.global_ctors element might be surprising. Is this change to work with some specific code or specific IR code generators that use 65535 (instrumentation cannot do anything with .init_array.65535 in inline assembly)? Can we change these tools instead?

[vitalybuka]

asan changing the priority of a llvm.global_ctors element might be surprising. Is this change to work with some specific code or specific IR code generators that use 65535 (instrumentation cannot do anything with .init_array.65535 in inline assembly)? Can we change these tools instead?

No. The goal to be able to call asan runtime callback just after the last dynamic constructor.

[MaskRay]

asan changing the priority of a llvm.global_ctors element might be surprising. Is this change to work with some specific code or specific IR code generators that use 65535 (instrumentation cannot do anything with .init_array.65535 in inline assembly)? Can we change these tools instead?

No. The goal to be able to call asan runtime callback just after the last dynamic constructor.

Is there more information about this decision? What would break if the order is different?

So priority 65535 is unsuffixed .init_array. Lots of code might use inline asm .section .init_array,"aw",@init_array.
Instrumentation cannot safely do anything with them (e.g. adding .65534 suffix).

If instrumentation changes IR 65535 elements in llvm.global_ctors but not asm .section .init_array, this could be risky as well.

Execution order:

a.o:(.init_array.1) b.o:(.init_array.1)
a.o:(.init_array.2) b.o:(.init_array.2)
...
a.o:(.init_array.65533) b.o:(.init_array.65533)
a.o:(.init_array.65534) b.o:(.init_array.65534)
a.o:(.init_array) b.o:(.init_array)

[vitalybuka]

asan changing the priority of a llvm.global_ctors element might be surprising. Is this change to work with some specific code or specific IR code generators that use 65535 (instrumentation cannot do anything with .init_array.65535 in inline assembly)? Can we change these tools instead?

No. The goal to be able to call asan runtime callback just after the last dynamic constructor.

Is there more information about this decision? What would break if the order is different?

So priority 65535 is unsuffixed .init_array. Lots of code might use inline asm .section .init_array,"aw",@init_array. Instrumentation cannot safely do anything with them (e.g. adding .65534 suffix).

If instrumentation changes IR 65535 elements in llvm.global_ctors but not asm .section .init_array, this could be risky as well.

Execution order:

a.o:(.init_array.1) b.o:(.init_array.1)
a.o:(.init_array.2) b.o:(.init_array.2)
...
a.o:(.init_array.65533) b.o:(.init_array.65533)
a.o:(.init_array.65534) b.o:(.init_array.65534)
a.o:(.init_array) b.o:(.init_array)

I see what you mean. :(

Is there more information about this decision? What would break if the order is different?

There is O(N^2) in init order checking, and it's heavy on our profiles, especial with dynamic linking:

Now we for each TU:

1. poison ALL program globals, except the current TU
2. init globals in TU
3. Unpoison ALL globals
4. repeat

With patches:

1. unpoison globals in prev TU
2. poison globals in the current TU
3. init globals in TU
4. do nothing
5. repeat

But at the end we need to unpoison all globals once, before we get to main()
So I didn't figure out a better way to call code reliably at the end of .init_array

[MaskRay]

asan changing the priority of a llvm.global_ctors element might be surprising. Is this change to work with some specific code or specific IR code generators that use 65535 (instrumentation cannot do anything with .init_array.65535 in inline assembly)? Can we change these tools instead?

No. The goal to be able to call asan runtime callback just after the last dynamic constructor.

Is there more information about this decision? What would break if the order is different?
So priority 65535 is unsuffixed .init_array. Lots of code might use inline asm .section .init_array,"aw",@init_array. Instrumentation cannot safely do anything with them (e.g. adding .65534 suffix).
If instrumentation changes IR 65535 elements in llvm.global_ctors but not asm .section .init_array, this could be risky as well.
Execution order:

a.o:(.init_array.1) b.o:(.init_array.1)
a.o:(.init_array.2) b.o:(.init_array.2)
...
a.o:(.init_array.65533) b.o:(.init_array.65533)
a.o:(.init_array.65534) b.o:(.init_array.65534)
a.o:(.init_array) b.o:(.init_array)

I see what you mean. :(

Is there more information about this decision? What would break if the order is different?

There is O(N^2) in init order checking, and it's heavy on our profiles, especial with dynamic linking:

Now we for each TU:

1. poison ALL program globals, except the current TU
2. init globals in TU
3. Unpoison ALL globals
4. repeat

With patches:

1. unpoison globals in prev TU
2. poison globals in the current TU
3. init globals in TU
4. do nothing
5. repeat

But at the end we need to unpoison all globals once, before we get to main() So I didn't figure out a better way to call code reliably at the end of .init_array

I see. The ODR indicator optimization might intrigue you to investigate the initialization order check performance:) I have some notes about the performance issue and I did not find a good way to improve the coverage (b.so accessing a) or fix the performance issue without main/__libc_start_main hacks, and I did not think further.

(clang++ -fsanitizje=address a0.cc a1.cc b.so -o a) In check_initialization_order=1,strict_init_order=1 mode,

• globals in b0.cc and b1.cc are registered
• b0.cc: __asan_before_dynamic_init poisons b1. Global initialization is run
• b1.cc: __asan_before_dynamic_init poisons b0. Global initialization is run
• globals in a0.cc and a1.cc are registered
• a0.cc: __asan_before_dynamic_init poisons b0,b1,a1. Global initialization is run. __asan_register_globals unpoisons b0,b1,a1
• a1.cc: __asan_before_dynamic_init poisons b0,b1,a0. Global initialization is run. __asan_register_globals unpoisons b0,b1,a0

Note, violations due to b.so accessing a cannot be detected.

---

-Wl,--wrap=main, while ugly, might be a solution but it is incompatible with existing -Wl,--wrap=main uses, so this might need to be opt-in or some opt-out mechanism might be needed.

[vitalybuka]

Thanks to @MaskRay we can abandon this in favor of #101837