[PATCH] D126100: Add sanitizer-specific GlobalValue attributes.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:429
+  // Extra sanitizer attributes that are used for global variables (GV's).
+  // GV's mentioned in -fsanitize-ignorelist=<file>.
+  kw_no_sanitize,
----------------
it should be next to other kw_ stuff


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:123
+  // (15 + 4 + 2 + 2 + 2 + 3 + 1 + 1 + 1 + 1) == 32.
   unsigned SubClassData : GlobalValueSubClassDataBits;
 
----------------
It's subclass data, but you keep this data in this class.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:316-328
+  struct SanitizerMetadata {
+    enum GlobalSanitizer : unsigned {
+      NoSanitize = 1 << 0,
+      NoAddress = 1 << 1,
+      NoHWAddress = 1 << 2,
+      NoMemtag = 1 << 3,
+    };
----------------



================
Comment at: llvm/include/llvm/IR/GlobalValue.h:331
+  bool hasSanitizerMetadata() const { return HasSanitizerMetadata; }
+  const SanitizerMetadata &getSanitizerMetadata() const;
+  void setSanitizerMetadata(const SanitizerMetadata &Meta);
----------------
get/set for each flag?


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:55
 
+using SanitizerMetadata = GlobalValue::SanitizerMetadata;
+using GlobalSanitizer = SanitizerMetadata::GlobalSanitizer;
----------------
you should put these inside of fuctions where it's used.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1265
         return true;
+    } else if (isSanitizer(Lex.getKind())) {
+      if (parseSanitizer(GV))
----------------
it would be nice to keep existing  "else if" style one per kw
with separate set/get it should be easy



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1282
   std::vector<unsigned> FwdRefAttrGrps;
   if (parseFnAttributeValuePairs(Attrs, FwdRefAttrGrps, false, BuiltinLoc))
     return true;
----------------
why it's not a part of parseFnAttributeValuePairs ?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3547
+        static_cast<llvm::GlobalValue::SanitizerMetadata::GlobalSanitizer>(
+            Record[16]);
+    Meta.IsDynInit = static_cast<bool>(Record[17]);
----------------
Why DynInit has own record?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126100