[PATCH] D96571: [ASan] Introduce a way set different ways of emitting module destructors.

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h:13
+#define LLVM_TRANSFORMS_INSTRUMENTATION_ADDRESSSANITIZER_OPTIONS_H
+#include "llvm/ADT/StringRef.h"
+
----------------
vitalybuka wrote:
> unused include?
Oops. I thought I had removed that one. I'll fix this.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2175
+  if (DestructorKind != AsanDtorKind::None) {
+    IRBuilder<> IRB_Dtor(CreateAsanModuleDtor(M));
+    IRB_Dtor.CreateCall(AsanUnregisterElfGlobals,
----------------
vitalybuka wrote:
> Please fix all clang-tidy
I didn't change the variable name for the IRBuilder so this problem was in the code before my change. Should fixing this be in a separate change seeing as the code clean up isn't really related to change I'm making?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2265
   // gets closed.
-  IRBuilder<> IRB_Dtor(CreateAsanModuleDtor(M));
-  IRB_Dtor.CreateCall(AsanUnregisterGlobals,
-                      {IRB.CreatePointerCast(AllGlobals, IntptrTy),
-                       ConstantInt::get(IntptrTy, N)});
+  if (DestructorKind != AsanDtorKind::None) {
+    IRBuilder<> IRB_Dtor(CreateAsanModuleDtor(M));
----------------
vitalybuka wrote:
> Can you check if some pre-existing tests fail if constructor accidentally initialized it to  AsanDtorKind::None)
Sure. I'll test this. I would expect `test/asan/TestCases/Posix/dlclose-test.cpp` and the new `llvm/test/Instrumentation/AddressSanitizer/no_global_dtors.ll` to fail at a minimum but I will confirm my assumptions and report back.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96571/new/

https://reviews.llvm.org/D96571