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

[Mitch Phillips via Phabricator via cfe-commits]

hctim marked 5 inline comments as done.
hctim added inline comments.


================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:397
+  // GV's mentioned in -fsanitize-ignorelist=<file>.
+  kw_no_sanitize,
+  // GV's with __attribute__((no_sanitize("address"))).
----------------
vitalybuka wrote:
> do we need no_sanitize on IR level?
> We should map it to particular no_sanitize_nnnnn
Removed and mapped it to the `no_sanitize_*`.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:329
+    }
+    void deserialize(unsigned V) {
+      if (V & (1 << 0)) NoSanitize = true;
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > serialization should not be in this header, this bit stuff is /Bitcode/ specific, both should be there
> > serialization should not be in this header, this bit stuff is /Bitcode/ specific, both should be there
> 
> I don't see it's done
hmm, yeah, sorry bout that. bad upload i guess.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:3543-3552
+    if (MD.NoSanitize) {
+      Out << ", no_sanitize";
+    } else {
+      if (MD.NoAddress)
+        Out << ", no_sanitize_address";
+      if (MD.NoHWAddress)
+        Out << ", no_sanitize_hwaddress";
----------------
vitalybuka wrote:
> even if it looks redundant I would not recommend to optimize here,
> AsmWriter should not care about semantics. if you let SanitizerMetadata assign this way, then AsmWriter should store it as is.
roger, done


================
Comment at: llvm/lib/IR/Globals.cpp:70
   setPartition(Src->getPartition());
+  if (Src->hasSanitizerMetadata())
+    setSanitizerMetadata(Src->getSanitizerMetadata());
----------------
vitalybuka wrote:
> It should copy unconditionally.
> E.g. if "this"  had Metadata, but src does not , it must be reset here.
thanks for the catch.

we do have an invariant about "don't call getSanitizerMetadata when HasSanitizerMetada is false", and i'd rather not force these structs to be stored if there's no metadata, but i have fixed the issue.


================
Comment at: llvm/lib/IR/Globals.cpp:231
+  getContext().pImpl->GlobalValueSanitizerMetadata[this] = Meta;
+  HasSanitizerMetadata = true;
+}
----------------
vitalybuka wrote:
> 
as above, rather not waste memory on structs if they don't exist, so keeping the invariant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126100