[clang][docs] Update SanitizerSpecialCaseList docs

[ellis]

Summary

The docs in SanitizerSpecialCaseList.rst seem to be outdated.

• "The meaning of * in regular expression for entity names is different - it is treated as in shell wildcarding."
  • This is wrong because */foo.c will match /path/to/foo.c instead of just something/foo.c. Instead, * is treated as .* and filepaths are treated as strings.
• I've removed fun:MyFooBar since it made it seem like we could match demangled names.

To confirm these changes, I've extended the profile-filter-new.c test and updated it to use split-file.

[MaskRay]

"The meaning of * in regular expression for entity names is different - it is treated as in shell wildcarding."

Right. See SpecialCaseList.cpp

I've removed fun:MyFooBar since it made it seem like we could match demangled names.

Yes, this is a limitation that we cannot match just C files (src:*.c matches .cc and .cpp files as well)...

clang/test/CodeGen/profile-filter-new.c

1

is more robust since if %t previously exists as a file (stale file after testing), the new test will fail...

rm -rf %t is also clear that all files in the old directory are removed, so more difficult to reference dangling files after updating tests...

// RUN: rm -rf %t && split-file %s %t && cd %t is common as well to avoid %t/ occurrences...

11

Consider dropping Inputs/ for all files. It's unneeded and misleading (not in the source directory).

[MaskRay]

For this patch, the best test is actually llvm/unittests/Support/SpecialCaseListTest.cpp. clang/test/CodeGen/profile-filter-new.c is a bit loosely connected to the gist of the patch.

[phosek]

This is unrelated to this change but related to the issue this change is addressing. The use of regular expressions for special case list with the special handling of * is error-prone, I've seen many people having issues it. Furthermore, regular expression for matching file names requires excessive escaping (see for example https://fuchsia-review.git.corp.google.com/c/fuchsia/+/763162) and most patterns used for function names only utilize *, not requiring the full strength of regular expression. I think we should consider replacing regular expressions with glob patterns. These are already implemented in https://github.com/llvm/llvm-project/blob/3391bdc255f1a75c59d71c7305959e84d8d5f468/llvm/include/llvm/Support/GlobPattern.h so the implementation should be straightforward, but it's going to be a breaking change. I don't know how widespread the use of special case lists is and whether that's going to be an issue?

[ellis]

In D152762#4421630, @phosek wrote:

This is unrelated to this change but related to the issue this change is addressing. The use of regular expressions for special case list with the special handling of * is error-prone, I've seen many people having issues it. Furthermore, regular expression for matching file names requires excessive escaping (see for example https://fuchsia-review.git.corp.google.com/c/fuchsia/+/763162) and most patterns used for function names only utilize *, not requiring the full strength of regular expression. I think we should consider replacing regular expressions with glob patterns. These are already implemented in https://github.com/llvm/llvm-project/blob/3391bdc255f1a75c59d71c7305959e84d8d5f468/llvm/include/llvm/Support/GlobPattern.h so the implementation should be straightforward, but it's going to be a breaking change. I don't know how widespread the use of special case lists is and whether that's going to be an issue?

Oh neat, I wasn't aware of GlobPattern.h. Personally, I would be happy with this change, but it would require changing some blocklists on my end (which is fine). I'm not sure what the processes is for a breaking change like this, but since clang 16 has just been released, it seems like a good time to change this. Thoughts?

[phosek]

In D152762#4421702, @ellis wrote:

In D152762#4421630, @phosek wrote:

This is unrelated to this change but related to the issue this change is addressing. The use of regular expressions for special case list with the special handling of * is error-prone, I've seen many people having issues it. Furthermore, regular expression for matching file names requires excessive escaping (see for example https://fuchsia-review.git.corp.google.com/c/fuchsia/+/763162) and most patterns used for function names only utilize *, not requiring the full strength of regular expression. I think we should consider replacing regular expressions with glob patterns. These are already implemented in https://github.com/llvm/llvm-project/blob/3391bdc255f1a75c59d71c7305959e84d8d5f468/llvm/include/llvm/Support/GlobPattern.h so the implementation should be straightforward, but it's going to be a breaking change. I don't know how widespread the use of special case lists is and whether that's going to be an issue?

Oh neat, I wasn't aware of GlobPattern.h. Personally, I would be happy with this change, but it would require changing some blocklists on my end (which is fine). I'm not sure what the processes is for a breaking change like this, but since clang 16 has just been released, it seems like a good time to change this. Thoughts?

I think that making this change now and highlighting it as a breaking change for LLVM 17 is probably the right strategy. We should also consider extending the existing GlobPattern implementation to support brace expansion (i.e. {foo,bar,baz}) to better match the existing functionality.

[ellis]

Moved to https://reviews.llvm.org/D154014

clang/docs/SanitizerSpecialCaseList.rst

 Section names are regular expressions written in square brackets that denote
 which sanitizer the following entries apply to. For example, ``[address]``
 specifies AddressSanitizer while ``[cfi-vcall|cfi-icall]`` specifies Control
 Flow Integrity virtual and indirect call checking. Entries without a section
 will be placed under the ``[*]`` section applying to all enabled sanitizers.
 
 Entries contain an entity type, followed by a colon and a regular expression,
 specifying the names of the entities, optionally followed by an equals sign and
-a tool-specific category, e.g. ``fun:*ExampleFunc=example_category``.  The
-meaning of ``*`` in regular expression for entity names is different - it is
-treated as in shell wildcarding. Two generic entity types are ``src`` and
+a tool-specific category, e.g. ``fun:*ExampleFunc=example_category``.
+Two generic entity types are ``src`` and
 ``fun``, which allow users to specify source files and functions, respectively.
 Some sanitizer tools may introduce custom entity types and categories - refer to
 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 the source file
+    src:path/to/source/file.c
+    src:*/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 .*):
+    # "*" 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

clang/test/CodeGen/profile-filter-new.c

-// RUN: %clang_cc1 -fprofile-instrument=llvm -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}"
+// RUN: split-file %s %t
 
-// RUN: echo -e "[llvm]\nfunction:foo=skip" > %t0.list
-// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t0.list -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,SKIP-FOO
-
-// RUN: echo -e "[csllvm]\nfunction:bar=forbid" > %t1.list
-// RUN: %clang_cc1 -fprofile-instrument=csllvm -fprofile-list=%t1.list -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID-BAR
-// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t1.list -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}"
-
-// RUN: echo -e "[llvm]\ndefault:forbid\nfunction:foo=allow" > %t2.list
-// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t2.list -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID
-
-// RUN: echo "[llvm]" > %t2.list
-// RUN: echo "source:%s=forbid" | sed -e 's/\\/\\\\/g' >> %t2.list
-// RUN: echo "function:foo=allow" >> %t2.list
-// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t2.list -emit-llvm %s -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID
+// RUN: %clang_cc1 -fprofile-instrument=llvm -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}"
+// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t/Inputs/skip-foo.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,SKIP-FOO
+// RUN: %clang_cc1 -fprofile-instrument=csllvm -fprofile-list=%t/Inputs/forbid-bar.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID-BAR
+// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t/Inputs/forbid-bar.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}"
+// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t/Inputs/forbid-default.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID
+// RUN: %clang_cc1 -fprofile-instrument=csllvm -fprofile-list=%t/Inputs/forbid-default.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID
+// RUN: %clang_cc1 -fprofile-instrument=llvm -fprofile-list=%t/Inputs/forbid-source.txt -emit-llvm %t/Inputs/src/code.c -o - | FileCheck %s --implicit-check-not="; {{.* (noprofile|skipprofile)}}" --check-prefixes=CHECK,FORBID
 
+//--- Inputs/src/code.c
 // SKIP-FOO: skipprofile
 // CHECK-LABEL: define {{.*}} @foo
 int foo(int a) { return 4 * a + 1; }
 
 // FORBID-BAR: noprofile
 // FORBID: noprofile
 // CHECK-LABEL: define {{.*}} @bar
 int bar(int a) { return 4 * a + 2; }
 
 // FORBID: noprofile
 // CHECK-LABEL: define {{.*}} @goo
 int goo(int a) { return 4 * a + 3; }
+
+//--- Inputs/skip-foo.txt
+[llvm]
+# This is a comment
+# Use mangled names to select functions
+function:foo=skip
+
+//--- Inputs/forbid-bar.txt
+[csllvm]
+function:bar=forbid
+
+//--- Inputs/forbid-default.txt
+[llvm|csllvm]
+default:forbid
+function:foo=allow
+
+//--- Inputs/forbid-source.txt
+[llvm]
+# The "*" prefix matches the whole directory "%t/Inputs/" and "(/|\\)" matches
+# unix-like and windows path separators
+source:*(/|\\)src(/|\\)code.c=forbid
+# Function entries take precedence over source entries
+function:foo=allow