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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 7 inline comments as done.
hctim 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,
----------------
vitalybuka wrote:
> it should be next to other kw_ stuff
hoisted them up


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:123
+  // (15 + 4 + 2 + 2 + 2 + 3 + 1 + 1 + 1 + 1) == 32.
   unsigned SubClassData : GlobalValueSubClassDataBits;
 
----------------
vitalybuka wrote:
> It's subclass data, but you keep this data in this class.
this is the spare bits over for the subclass to use. because we keep an additional bit in this class, we give one less bit out to the subclass.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:331
+  bool hasSanitizerMetadata() const { return HasSanitizerMetadata; }
+  const SanitizerMetadata &getSanitizerMetadata() const;
+  void setSanitizerMetadata(const SanitizerMetadata &Meta);
----------------
vitalybuka wrote:
> get/set for each flag?
as discussed offline, no point with our nice new members.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1265
         return true;
+    } else if (isSanitizer(Lex.getKind())) {
+      if (parseSanitizer(GV))
----------------
vitalybuka wrote:
> it would be nice to keep existing  "else if" style one per kw
> with separate set/get it should be easy
> 
i think there's enough commonality with the `hasSanitizerMetadata() -> getSanitizerMetadata -> set flag -> setSanitizerMetadata -> lex` that i'd normally opt to factor this out anyway. cleaned up the helper function using the new method of setting attributes though.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1282
   std::vector<unsigned> FwdRefAttrGrps;
   if (parseFnAttributeValuePairs(Attrs, FwdRefAttrGrps, false, BuiltinLoc))
     return true;
----------------
vitalybuka wrote:
> why it's not a part of parseFnAttributeValuePairs ?
as discussed offline, global attributes have different semantics / no generalized parsing, unlike functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126100