[PATCH] D126929: Add sanitizer metadata attributes to clang IR gen.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

No need to recover my snapshot, I'll just comment here.



================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:20
 
+using GlobalVariable = llvm::GlobalVariable;
+using GVSanitizerMetadata = GlobalVariable::SanitizerMetadata;
----------------
Seems inconsistent with the rest.
it no so frequently use here to justify indirection.


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:55
+
+  bool IsExcluded = CGM.isInNoSanitizeList(GV, Loc, Ty);
+  IsExcluded |= (NoSanitizeMask == SanitizerKind::All);
----------------
it can be in some weird ubsan check ignore list, and this way it will propagate on asan/hwasan
I don't think you can avoid extending isInNoSanitizeList (in a separate patch)


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:60
+                         SanitizerKind::KernelAddress)) {
+    IsExcluded |= CGM.isInNoSanitizeList(GV, Loc, Ty, "address");
+    IsExcluded |= NoSanitizeSet.hasOneOf(SanitizerKind::Address |
----------------
the last arg is not sanitizer, just a particulate check of it
like "init" here llvm-project/compiler-rt/lib/asan/asan_ignorelist.txt


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:64
                                      bool IsDynInit) {
-  if (!isAsanHwasanOrMemTag(CGM.getLangOpts().Sanitize))
-    return;
----------------
May be early isAsanHwasanOrMemTag check here is useful to avoid string stuff below for compilation without sanitizers.


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:70
 
-  auto getNoSanitizeMask = [](const VarDecl &D) {
-    if (D.hasAttr<DisableSanitizerInstrumentationAttr>())
----------------
I don't insist but one it's cleaner with lambda and return
if you prefer your way please revert lambda in a separate patch


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:91-93
+  llvm::LLVMContext &VMContext = CGM.getLLVMContext();
   llvm::Metadata *LocDescr = nullptr;
   llvm::Metadata *GlobalName = nullptr;
----------------
undo this line move


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:118
+  SanitizerMask NoSanitizeMask;
+  for (auto *Attr : D.specific_attrs<NoSanitizeAttr>()) {
+    NoSanitizeMask |= Attr->getMask();
----------------
don't use {} for one liners


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:122
+
+  if (D.hasAttr<DisableSanitizerInstrumentationAttr>()) {
+    NoSanitizeMask = SanitizerKind::All;
----------------
this if probably should go before the loop


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:126
+
   std::string QualName;
   llvm::raw_string_ostream OS(QualName);
----------------
moving QualName calculation is unnecessary, please move it back before NoSanitizeMask


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126929