[PATCH] D127911: Delete 'llvm.asan.globals' for global metadata.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

The rest is LGTM



================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:67-72
+  if (FsanitizeArgument.has(SanitizerKind::Address) && !Meta.NoAddress) {
     IsDynInit &= !CGM.isInNoSanitizeList(SanitizerKind::Address |
                                              SanitizerKind::KernelAddress,
                                          GV, Loc, Ty, "init");
     Meta.IsDynInit = IsDynInit;
   }
----------------
I recommend to move this change into another patch

and it should probably be:
Meta.IsDynInit &= IsDynInit && FsanitizeArgument.has(SanitizerKind::Address) && !Meta.NoAddress && !CGM.isInNoSanitizeLis;


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:34
 #include "llvm/BinaryFormat/MachO.h"
+#include "llvm/Demangle/Demangle.h"
 #include "llvm/IR/Argument.h"
----------------
Please don't demangle in this patch, or keep as close as possible to the current behaviour
Also isn't demangling by compliler-rt is better? mangled form is shorter.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1355-1356
   // at all, we assume it has dynamic initializer (in other TU).
-  //
-  // FIXME: Metadata should be attched directly to the global directly instead
-  // of being added to llvm.asan.globals.
-  return G->hasInitializer() && !GlobalsMD.get(G).IsDynInit;
+  if (G->hasSanitizerMetadata() && G->getSanitizerMetadata().IsDynInit)
+    return false;
+
----------------
I believe previous was like this.
if you want to change that lets do another patch. 


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2330
+        ConstantInt::get(IntptrTy, Meta.IsDynInit),
+        ConstantInt::get(IntptrTy, 0),
         ConstantExpr::getPointerCast(ODRIndicator, IntptrTy));
----------------
hctim wrote:
> vitalybuka wrote:
> > MD was fine, less changed lines
> done
Constant::getNullValue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127911