[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov added inline comments.


================
Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:33
+
+class SanitizerMetadataFactory {
+  SanitizerMetadataFactory(const SanitizerMetadataFactory &) = delete;
----------------
Not sure if this class follows the 'factory' design pattern, which typically it would have some sort of 'produce' or 'create' method. Maybe SanitizerMatadataWriter or SanitizerMatadataCreator? 


================
Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:46-48
+  void setASanSpecificMetadata(llvm::GlobalVariable::SanitizerMetadata &Meta,
+                               llvm::GlobalVariable *GV, SourceLocation Loc,
+                               QualType Ty, bool IsDynInit = false);
----------------
This could be private I think.


================
Comment at: compiler-rt/lib/asan/asan_globals.cpp:90
+
+  DataInfo info;
+  Symbolizer::GetOrInit()->SymbolizeData(g.beg, &info);
----------------
Outside of the scope of this change. It looks like DataInfo's constructor is calling memset on it's fields. I would have probably designed this differently because now it looks like potential uninitialized when we check for 'info.line != 0'. 


================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:205
   kw_nest,
+  kw_no_sanitize,
+  kw_no_sanitize_address,
----------------
Why do we need to add these to lex? I am surprised that didn't have the need so far. 


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:312
+    void RemoveSanitizer(GlobalSanitizer S) { Sanitizer &= ~S; }
+    bool HasSanitizer(GlobalSanitizer S) const {
+      if (Sanitizer == GlobalSanitizer::NoSanitize)
----------------
I suspect that there might be another place where that logic exists. I am worried that we are duplicating it. Do you might looking for it in the code base?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493