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

[Mitch Phillips via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2767-2781
+  if (NoSanitizeL.containsGlobal(LangOpts.Sanitize.Mask, GV->getName(), Category))
     return true;
-  if (NoSanitizeL.containsLocation(EnabledAsanMask, Loc, Category))
+  if (NoSanitizeL.containsLocation(LangOpts.Sanitize.Mask, Loc, Category))
     return true;
   // Check global type.
   if (!Ty.isNull()) {
     // Drill down the array types: if global variable of a fixed type is
----------------
hctim wrote:
> vitalybuka wrote:
> > can this lines be landed separately?
> sure
(now we do touch a little bit here regardless)


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:55
+
+  bool IsExcluded = CGM.isInNoSanitizeList(GV, Loc, Ty);
+  IsExcluded |= (NoSanitizeMask == SanitizerKind::All);
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > 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)
> you you can introduce:
> 
> ```
> bool CodeGenModule::isInNoSanitizeList(SanitizerMask Kind, llvm::GlobalVariable *GV,
>                                        SourceLocation Loc) const {
> ```
> 
> similar to
> 
> ```
> bool CodeGenModule::isInNoSanitizeList(SanitizerMask Kind, llvm::Function *Fn,
>                                        SourceLocation Loc) const {
> ```
done, but bearing in mind if you have some global `src:` exclude in an `-fsanitize-ignorelist` that's designed to ignore some file for UBSan, and then you compile with `-fsanitize=address,undefined` and use that `-fsanitize-ignorelist`, then those GVs would also be ignored in ASan. The right way to go about that is to have the creator of the ignorelist make sure that the `src:` rule is in a `[undefined]` block. Added the expected use case to `sanitizer-special-case-list-globals.txt`.

i think it's small enough a change + relevant enough to this CL to not fork it out to a different patch and then have to do the cleanup twice.


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


================
Comment at: clang/lib/CodeGen/SanitizerMetadata.cpp:70
 
-  auto getNoSanitizeMask = [](const VarDecl &D) {
-    if (D.hasAttr<DisableSanitizerInstrumentationAttr>())
----------------
vitalybuka wrote:
> 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
reverted it back, lambda here seems very fancy for a simple farmer like me, but i can't deny that it's pretty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126929