[sanitizer] Add "mainfile" prefix to sanitizer special case list

[MaskRay]

Summary

When an issue exists in the main file (caller) instead of an included file
(callee), using a src pattern applying to the included file may be
inappropriate if it's the caller's responsibility. Add mainfile prefix to check
the main filename.

For the example below, the issue may reside in a.c (foo should not be called
with a misaligned pointer or foo should switch to an unaligned load), but with
src we can only apply to the innocent callee a.h. With this patch we can use
the more appropriate mainfile:a.c.

//--- a.h
// internal linkage
static inline int load(int *x) { return *x; }

//--- a.c, -fsanitize=alignment
#include "a.h"
int foo(void *x) { return load(x); }

See updated clang/docs/SanitizerSpecialCaseList.rst for caveat using
mainfile due to C++ vague linkage functions.

[vitalybuka]

problem with included files that we don't know which non-inlined version of the function will endup in the binary
so using this option, user may unintentionally disable instrumentation on all included headers, even when included from a different place

[MaskRay]

In D129832#3654040, @vitalybuka wrote:

problem with included files that we don't know which non-inlined version of the function will endup in the binary
so using this option, user may unintentionally disable instrumentation on all included headers, even when included from a different place

This argument applies to vague linkage functions which are deduplicated.
There are many internal linkage function use cases which can benefit this, e.g. static inline.
But thanks for the comment. Let me improve the summary.

[vitalybuka]

In D129832#3654066, @MaskRay wrote:

In D129832#3654040, @vitalybuka wrote:

problem with included files that we don't know which non-inlined version of the function will endup in the binary
so using this option, user may unintentionally disable instrumentation on all included headers, even when included from a different place

This argument applies to vague linkage functions which are deduplicated.
There are many internal linkage function use cases which can benefit this, e.g. static inline.
But thanks for the comment. Let me improve the summary.

I understand how it benefit static inclines, it still unintentionally regress instrumentation for other code.
we need at least a warning in documentation

Also if this is not widespread problem, I would prefer we don't do that.
Having mainsrc: in ignore list will complicate debugging false negatives.

[vitalybuka]

mainsrc may still be useful, e.g. if a wildcard is used to ensure the prevailing one is ignored.

Not sure I understand, how can we make that? Usually it's hard to control what is included, especially indirectly.

[MaskRay]

Warn mainsrc for C++ vague linkage functions

[MaskRay]

In D129832#3654076, @vitalybuka wrote:

mainsrc may still be useful, e.g. if a wildcard is used to ensure the prevailing one is ignored.

Not sure I understand, how can we make that? Usually it's hard to control what is included, especially indirectly.

Elaborated in the updated clang/docs/SanitizerSpecialCaseList.rst

[vitalybuka]

clang/lib/CodeGen/CodeGenModule.cpp

2786

we search exactly the same MainFile.getName() in containsMainFile and containsFile ?

[vitalybuka]

clang/lib/CodeGen/CodeGenModule.cpp

2786

I see, it's some fallback branch

[vitalybuka]

clang/docs/SanitizerSpecialCaseList.rst

110–112

if this is transitionalt solution, would it be better just to use -fno-sanitize= on *.cpp file?

[MaskRay]

clang/docs/SanitizerSpecialCaseList.rst

108

Added: `(There is an action at a distance risk.)`

110–112

This provides a more convenient way than using -fno-sanitize= for a different set of files.

E.g. if a group wants to ignore a check for all `[a-m]* files, then can use mainsrc:[a-m]* instead of patching the build system.

[MaskRay]

update doc

[vitalybuka]

The patch is LGTM, but please consider to keep an ignorelist limited.

clang/docs/SanitizerSpecialCaseList.rst

110–112

Convenience of -fno-sanitize= approach that you make it visible to maintainer of component. Then it's their responsibility.
Ignore list is toolchain team debt.

[kstoimenov]

I think the name 'mainsrc' is slightly misleading because of the association with the 'main' function. Maybe something like primarysrc would be better to avoid this confusion?

[MaskRay]

In D129832#3654436, @vitalybuka wrote:

The patch is LGTM, but please consider to keep an ignorelist limited.

Thanks!

In D129832#3655393, @kstoimenov wrote:

I think the name 'mainsrc' is slightly misleading because of the association with the 'main' function. Maybe something like primarysrc would be better to avoid this confusion?

How about mainfile? Clang resources to the file as the main file. I picked src suffix just to be similar to src...

[vitalybuka]

I guess Kirills' complain is about main part, not src.
But I have no idea what name could be better.
I guess thechnically it's preprocessed src? :)
To me as is name is fine.

[vitalybuka]

"unit:" ?

[MaskRay]

mainsrc => mainfile

Clang names the file "main file". CC1 has options like -main-file-name.
Using "mainfile" instead of "mainsrc" avoids introducing new terminology.

clang/docs/SanitizerSpecialCaseList.rst

 tool-specific docs.
 
 .. code-block:: bash
 
     # Lines starting with # are ignored.
     # Turn off checks for the source file (use absolute path or path relative
     # to the current working directory):
     src:/path/to/source/file.c
+    # Turn off checks for this main file, including files included by it.
+    # Useful when the main file instead of an included file should be ignored.
+    mainfile:file.c
     # Turn off checks for a particular functions (use mangled names):
     fun:MyFooBar
     fun:_Z8MyFooBarv
     # Extended regular expressions are supported:
     fun:bad_(foo|bar)
     src:bad_source[1-9].c
     # Shell like usage of * is supported (* is treated as .*):
     src:bad/sources/*
     fun:*BadFunction*
     # Specific sanitizer tools may introduce categories.
     src:/special/path/*=special_sources
     # Sections can be used to limit ignorelist entries to specific sanitizers
     [address]
     fun:*BadASanFunc*
     # Section names are regular expressions
     [cfi-vcall|cfi-icall]
     fun:*BadCfiCall
     # Entries without sections are placed into [*] and apply to all sanitizers
+
+``mainfile`` is similar to applying ``-fno-sanitize=`` to a set of files but
+does not need plumbing into the build system. This works well for internal
+linkage functions but has a caveat for C++ vague linkage functions.
+
+C++ vague linkage functions (e.g. inline functions, template instantiations) are
+deduplicated at link time. A function (in an included file) ignored by a
+specific ``mainfile`` pattern may not be the prevailing copy picked by the
+linker. Therefore, using ``mainfile`` requires caution. It may still be useful,
+e.g. when patterns are picked in a way to ensure the prevailing one is ignored.
+(There is action-at-a-distance risk.)
+
+``mainfile`` can be useful enabling a ubsan check for a large code base when
+finding the direct stack frame triggering the failure for every failure is
+difficult.

clang/include/clang/Basic/NoSanitizeList.h

public:
   ~NoSanitizeList();
   bool containsGlobal(SanitizerMask Mask, StringRef GlobalName,
                       StringRef Category = StringRef()) const;
   bool containsType(SanitizerMask Mask, StringRef MangledTypeName,
                     StringRef Category = StringRef()) const;
   bool containsFunction(SanitizerMask Mask, StringRef FunctionName) const;
   bool containsFile(SanitizerMask Mask, StringRef FileName,
                     StringRef Category = StringRef()) const;
+  bool containsMainFile(SanitizerMask Mask, StringRef FileName,
+                        StringRef Category = StringRef()) const;
   bool containsLocation(SanitizerMask Mask, SourceLocation Loc,
                         StringRef Category = StringRef()) const;
 };
 
 } // end namespace clang
 
 #endif

clang/lib/Basic/NoSanitizeList.cpp

bool NoSanitizeList::containsFunction(SanitizerMask Mask,
   return SSCL->inSection(Mask, "fun", FunctionName);
 }
 
 bool NoSanitizeList::containsFile(SanitizerMask Mask, StringRef FileName,
                                   StringRef Category) const {
   return SSCL->inSection(Mask, "src", FileName, Category);
 }
 
+bool NoSanitizeList::containsMainFile(SanitizerMask Mask, StringRef FileName,
+                                      StringRef Category) const {
+  return SSCL->inSection(Mask, "mainfile", FileName, Category);
+}
+
 bool NoSanitizeList::containsLocation(SanitizerMask Mask, SourceLocation Loc,
                                       StringRef Category) const {
   return Loc.isValid() &&
          containsFile(Mask, SM.getFilename(SM.getFileLoc(Loc)), Category);
 }

clang/lib/CodeGen/CodeGenModule.cpp

 }
 
 bool CodeGenModule::isInNoSanitizeList(SanitizerMask Kind, llvm::Function *Fn,
                                        SourceLocation Loc) const {
   const auto &NoSanitizeL = getContext().getNoSanitizeList();
   // NoSanitize by function name.
   if (NoSanitizeL.containsFunction(Kind, Fn->getName()))
     return true;
-  // NoSanitize by location.
+  // NoSanitize by location. Check "mainfile" prefix.
+  auto &SM = Context.getSourceManager();
+  const FileEntry &MainFile = *SM.getFileEntryForID(SM.getMainFileID());
+  if (NoSanitizeL.containsMainFile(Kind, MainFile.getName()))
+    return true;
+
+  // Check "src" prefix.
   if (Loc.isValid())
     return NoSanitizeL.containsLocation(Kind, Loc);
   // If location is unknown, this may be a compiler-generated function. Assume
   // it's located in the main file.
-  auto &SM = Context.getSourceManager();
-  if (const auto *MainFile = SM.getFileEntryForID(SM.getMainFileID())) {
-    return NoSanitizeL.containsFile(Kind, MainFile->getName());
-  }
-  return false;
+  return NoSanitizeL.containsFile(Kind, MainFile.getName());
 }
 
 bool CodeGenModule::isInNoSanitizeList(SanitizerMask Kind,
                                        llvm::GlobalVariable *GV,
                                        SourceLocation Loc, QualType Ty,
                                        StringRef Category) const {
   const auto &NoSanitizeL = getContext().getNoSanitizeList();
   if (NoSanitizeL.containsGlobal(Kind, GV->getName(), Category))
     return true;
+  auto &SM = Context.getSourceManager();
+  if (NoSanitizeL.containsMainFile(
+          Kind, SM.getFileEntryForID(SM.getMainFileID())->getName(), Category))
+    return true;
   if (NoSanitizeL.containsLocation(Kind, Loc, Category))
     return true;
+
   // Check global type.
   if (!Ty.isNull()) {
     // Drill down the array types: if global variable of a fixed type is
     // not sanitized, we also don't instrument arrays of them.
     while (auto AT = dyn_cast<ArrayType>(Ty.getTypePtr()))
       Ty = AT->getElementType();
     Ty = Ty.getCanonicalType().getUnqualifiedType();
     // Only record types (classes, structs etc.) are ignored.

clang/test/CodeGen/sanitize-ignorelist-mainfile.c

+/// Test mainfile in a sanitizer special case list.
+// RUN: rm -rf %t && split-file %s %t && cd %t
+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fsanitize=address,alignment a.c -o - | FileCheck %s --check-prefixes=CHECK,DEFAULT
+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fsanitize=address,alignment -fsanitize-ignorelist=a.list a.c -o - | FileCheck %s --check-prefixes=CHECK,IGNORE
+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fsanitize=address,alignment -fsanitize-ignorelist=b.list a.c -o - | FileCheck %s --check-prefixes=CHECK,IGNORE
+
+//--- a.list
+mainfile:*a.c
+
+//--- b.list
+[address]
+mainfile:*a.c
+
+[alignment]
+mainfile:*.c
+
+//--- a.h
+int global_h;
+
+static inline int load(int *x) {
+  return *x;
+}
+
+//--- a.c
+#include "a.h"
+
+int global_c;
+
+int foo(void *x) {
+  return load(x);
+}
+
+// DEFAULT:     @___asan_gen_{{.*}} = {{.*}} c"global_h\00"
+// DEFAULT:     @___asan_gen_{{.*}} = {{.*}} c"global_c\00"
+// IGNORE-NOT:  @___asan_gen_
+
+// CHECK-LABEL: define {{.*}}@load(
+// DEFAULT:       call void @__ubsan_handle_type_mismatch_v1_abort(
+// DEFAULT:       call void @__asan_report_load4(
+// IGNORE-NOT:    call void @__ubsan_handle_type_mismatch_v1_abort(
+// IGNORE-NOT:    call void @__asan_report_load4(

llvm/include/llvm/Support/SpecialCaseList.h

 // using a '[section_name]' header and can be used to specify sanitizers the
 // entries below it apply to. Section names are regular expressions, and
 // entries without a section header match all sections (e.g. an '[*]' header
 // is assumed.)
 // The remaining lines should have the form:
 //   prefix:wildcard_expression[=category]
 // If category is not specified, it is assumed to be empty string.
 // Definitions of "prefix" and "category" are sanitizer-specific. For example,
-// sanitizer exclusion support prefixes "src", "fun" and "global".
-// Wildcard expressions define, respectively, source files, functions or
-// globals which shouldn't be instrumented.
+// sanitizer exclusion support prefixes "src", "mainfile", "fun" and "global".
+// Wildcard expressions define, respectively, source files, main files,
+// functions or globals which shouldn't be instrumented.
 // Examples of categories:
 //   "functional": used in DFSan to list functions with pure functional
 //                 semantics.
 //   "init": used in ASan exclusion list to disable initialization-order bugs
 //           detection for certain globals or source files.
 // Full special case list file example:
 // ---
 // [address]
 // # Excluded items:
 // fun:*_ZN4base6subtle*
 // global:*global_with_bad_access_or_initialization*
 // global:*global_with_initialization_issues*=init
 // type:*Namespace::ClassName*=init
 // src:file_with_tricky_code.cc
 // src:ignore-global-initializers-issues.cc=init
+// mainfile:main_file.cc
 //
 // [dataflow]
 // # Functions with pure functional semantics:
 // fun:cos=functional
 // fun:sin=functional
 // ---
 // Note that the wild card is in fact an llvm::Regex, but * is automatically
 // replaced with .*