[UBSan] Support src:*=sanitize for multiple ignorelists. #141640

qinkunbao · 2025-05-27T17:32:32Z

See: #139128 and #140529 for the background.

The introduction of these new tests (ubsan-src-ignorelist-category.test) -fsanitize-ignorelist=%t/src.ignorelist -fsanitize-ignorelist=%t/src.ignorelist.contradict9 in this PR will not lead to failures in the previous implementation (without this PR). This is because the existing logic distinguishes between Sections in different ignorelists, even if their names are identical. The order of these Sections is preserved using a vector.

Created using spr 1.3.6 [skip ci]

Created using spr 1.3.6

llvmbot · 2025-05-27T17:33:07Z

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Qinkun Bao (qinkunbao)

Changes

See: #139128 for the background.

Full diff: https://github.com/llvm/llvm-project/pull/141640.diff

4 Files Affected:

(modified) clang/include/clang/Basic/SanitizerSpecialCaseList.h (+9-5)
(modified) clang/lib/Basic/NoSanitizeList.cpp (+6-3)
(modified) clang/lib/Basic/SanitizerSpecialCaseList.cpp (+9-8)
(modified) clang/test/CodeGen/ubsan-src-ignorelist-category.test (+11)

diff --git a/clang/include/clang/Basic/SanitizerSpecialCaseList.h b/clang/include/clang/Basic/SanitizerSpecialCaseList.h
index 25d518e7128cf..a19392399bfd7 100644
--- a/clang/include/clang/Basic/SanitizerSpecialCaseList.h
+++ b/clang/include/clang/Basic/SanitizerSpecialCaseList.h
@@ -19,6 +19,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SpecialCaseList.h"
 #include <memory>
+#include <utility>
 #include <vector>
 
 namespace llvm {
@@ -44,20 +45,23 @@ class SanitizerSpecialCaseList : public llvm::SpecialCaseList {
                  StringRef Category = StringRef()) const;
 
   // Query ignorelisted entries if any bit in Mask matches the entry's section.
-  // Return 0 if not found. If found, return the line number (starts with 1).
-  unsigned inSectionBlame(SanitizerMask Mask, StringRef Prefix, StringRef Query,
-                          StringRef Category = StringRef()) const;
+  // Return (0,0 if not found. If found, return the file index number and the
+  // line number (FileIdx, LineNo) (all start with 1).
+  std::pair<unsigned, unsigned>
+  inSectionBlame(SanitizerMask Mask, StringRef Prefix, StringRef Query,
+                 StringRef Category = StringRef()) const;
 
 protected:
   // Initialize SanitizerSections.
   void createSanitizerSections();
 
   struct SanitizerSection {
-    SanitizerSection(SanitizerMask SM, SectionEntries &E)
-        : Mask(SM), Entries(E) {};
+    SanitizerSection(SanitizerMask SM, SectionEntries &E, unsigned idx)
+        : Mask(SM), Entries(E), FileIdx(idx) {};
 
     SanitizerMask Mask;
     SectionEntries &Entries;
+    unsigned FileIdx;
   };
 
   std::vector<SanitizerSection> SanitizerSections;
diff --git a/clang/lib/Basic/NoSanitizeList.cpp b/clang/lib/Basic/NoSanitizeList.cpp
index 4feef5c6ea052..c58a67971dfb6 100644
--- a/clang/lib/Basic/NoSanitizeList.cpp
+++ b/clang/lib/Basic/NoSanitizeList.cpp
@@ -44,14 +44,17 @@ bool NoSanitizeList::containsFunction(SanitizerMask Mask,
 
 bool NoSanitizeList::containsFile(SanitizerMask Mask, StringRef FileName,
                                   StringRef Category) const {
-  unsigned NoSanLine = SSCL->inSectionBlame(Mask, "src", FileName, Category);
+  auto [NoSanFileIdx, NoSanLine] =
+      SSCL->inSectionBlame(Mask, "src", FileName, Category);
   if (NoSanLine == 0)
     return false;
-  unsigned SanLine = SSCL->inSectionBlame(Mask, "src", FileName, "sanitize");
+  auto [SanFileIdx, SanLine] =
+      SSCL->inSectionBlame(Mask, "src", FileName, "sanitize");
   // If we have two cases such as `src:a.cpp=sanitize` and `src:a.cpp`, the
   // current entry override the previous entry.
   if (SanLine > 0)
-    return NoSanLine > SanLine;
+    return std::make_pair(NoSanFileIdx, NoSanLine) >
+           std::make_pair(SanFileIdx, SanLine);
   return true;
 }
 
diff --git a/clang/lib/Basic/SanitizerSpecialCaseList.cpp b/clang/lib/Basic/SanitizerSpecialCaseList.cpp
index 4508d705eb43a..0b2c0f9c4478a 100644
--- a/clang/lib/Basic/SanitizerSpecialCaseList.cpp
+++ b/clang/lib/Basic/SanitizerSpecialCaseList.cpp
@@ -49,28 +49,29 @@ void SanitizerSpecialCaseList::createSanitizerSections() {
 #undef SANITIZER
 #undef SANITIZER_GROUP
 
-    SanitizerSections.emplace_back(Mask, S.Entries);
+    SanitizerSections.emplace_back(Mask, S.Entries, S.FileIdx);
   }
 }
 
 bool SanitizerSpecialCaseList::inSection(SanitizerMask Mask, StringRef Prefix,
                                          StringRef Query,
                                          StringRef Category) const {
-  return inSectionBlame(Mask, Prefix, Query, Category);
+  auto [FileIdx, LineNo] = inSectionBlame(Mask, Prefix, Query, Category);
+  return FileIdx;
 }
 
-unsigned SanitizerSpecialCaseList::inSectionBlame(SanitizerMask Mask,
-                                                  StringRef Prefix,
-                                                  StringRef Query,
-                                                  StringRef Category) const {
+std::pair<unsigned, unsigned>
+SanitizerSpecialCaseList::inSectionBlame(SanitizerMask Mask, StringRef Prefix,
+                                         StringRef Query,
+                                         StringRef Category) const {
   for (auto it = SanitizerSections.crbegin(); it != SanitizerSections.crend();
        ++it) {
     if (it->Mask & Mask) {
       unsigned lineNum =
           SpecialCaseList::inSectionBlame(it->Entries, Prefix, Query, Category);
       if (lineNum > 0)
-        return lineNum;
+        return {it->FileIdx, lineNum};
     }
   }
-  return 0;
+  return {0, 0};
 }
diff --git a/clang/test/CodeGen/ubsan-src-ignorelist-category.test b/clang/test/CodeGen/ubsan-src-ignorelist-category.test
index 55967ec77c836..0ff2ea5c456a1 100644
--- a/clang/test/CodeGen/ubsan-src-ignorelist-category.test
+++ b/clang/test/CodeGen/ubsan-src-ignorelist-category.test
@@ -12,6 +12,9 @@
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=signed-integer-overflow -fsanitize-ignorelist=%t/src.ignorelist.contradict6 -emit-llvm %t/test1.c -o - | FileCheck %s --check-prefixes=CHECK1,SANITIZE
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=signed-integer-overflow -fsanitize-ignorelist=%t/src.ignorelist.contradict7 -emit-llvm %t/test1.c -o - | FileCheck %s --check-prefixes=CHECK1,IGNORE
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=signed-integer-overflow -fsanitize-ignorelist=%t/src.ignorelist.contradict8 -emit-llvm %t/test1.c -o - | FileCheck %s --check-prefixes=CHECK1,SANITIZE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=signed-integer-overflow -fsanitize-ignorelist=%t/src.ignorelist.contradict8 -fsanitize-ignorelist=%t/src.ignorelist.contradict9 -emit-llvm %t/test1.c -o - | FileCheck %s --check-prefixes=CHECK1,IGNORE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=signed-integer-overflow -fsanitize-ignorelist=%t/src.ignorelist.contradict9 -fsanitize-ignorelist=%t/src.ignorelist.contradict8 -emit-llvm %t/test1.c -o - | FileCheck %s --check-prefixes=CHECK1,SANITIZE
+
 
 // Verify ubsan only emits checks for files in the allowlist
 
@@ -77,6 +80,14 @@ src:*/tes*1.c=sanitize
 src:*/te*t1.c
 src:*/t*st1.c=sanitize
 
+//--- src.ignorelist.contradict9
+src:*
+src:*/test1.c=sanitize
+src:*/test1.c
+src:*/test1.c=sanitize
+src:*/te*t1.c
+src:*/test*.c
+
 
 //--- test1.c
 // CHECK1-LABEL: define dso_local i32 @add

Created using spr 1.3.6

clang/lib/Basic/NoSanitizeList.cpp

clang/lib/Basic/SanitizerSpecialCaseList.cpp

vitalybuka

LGTM with comments

…e-for-multiple-ignorelists

clang/lib/Basic/SanitizerSpecialCaseList.cpp

github-actions · 2025-05-28T04:29:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/include/llvm/Support/SpecialCaseList.h

qinkunbao · 2025-05-28T04:33:16Z

Looks like conflicts resolving got wrong. I will fix them and address the comments tomorrow

Created using spr 1.3.6

…e-for-multiple-ignorelists

clang/lib/Basic/NoSanitizeList.cpp

shafik · 2025-05-30T05:28:08Z

clang/include/clang/Basic/SanitizerSpecialCaseList.h

+  // Return NotFound (0,0) if not found. If found, return the file index number
+  // and the line number (FileIdx, LineNo) (FileIdx starts with 1 and LineNo
+  // starts with 0).
+  std::pair<unsigned, unsigned>


I feel like this is almost an anti-pattern, using std::pair like this. We have two opaque values that we now have to go back to the function to understand what they mean.

If you had a type (class) with two members (FileIdx and LineNo) then it would make sense at each use as opposed to first and second which is not informative.

I actually don't see the values explicitly used but I am assuming they will be in a later change.

I feel like this is almost an anti-pattern

LLVM code is full of them, and here they should not be exposed beyond NoSanitizeList, so I ignore on review.
But I will not argue if you are asking to improve.

It's used for comparison "return NoSan > San" in NoSanitizeList::containsFile.

I won't insist, but if we won't actually (maybe I am misunderstanding) use the value stored in the pair then a flag makes way more sense. Otherwise, having to use first and second is just prone to error.

I do get that this is local and that we (llvm) is not free of anti-patterns but we should strive to do better in new code. One because future new users might use this to learn and outside developers (even LLMs) will learn this is a good pattern and propagate its use.

See: llvm#139128 and llvm#140529 for the background. The introduction of these new tests (ubsan-src-ignorelist-category.test) `-fsanitize-ignorelist=%t/src.ignorelist -fsanitize-ignorelist=%t/src.ignorelist.contradict9` in this PR will not lead to failures in the previous implementation (without this PR). This is because the existing logic distinguishes between Sections in different ignorelists, even if their names are identical. The order of these Sections is preserved using a `vector`.

qinkunbao added 2 commits May 27, 2025 17:32

[𝘀𝗽𝗿] changes to main this commit is based on

d858fbe

Created using spr 1.3.6 [skip ci]

[𝘀𝗽𝗿] initial version

5fe858d

Created using spr 1.3.6

llvmbot added clang clang:frontend labels May 27, 2025

Add some comments.

cddba02

Created using spr 1.3.6

qinkunbao requested a review from vitalybuka May 27, 2025 17:43

Update tests.

468bc10

Created using spr 1.3.6

vitalybuka mentioned this pull request May 28, 2025

[SpecialCaseList] Iterate sections and matchers in reverse order #141697

Closed

vitalybuka reviewed May 28, 2025

View reviewed changes

clang/lib/Basic/NoSanitizeList.cpp Outdated Show resolved Hide resolved

vitalybuka reviewed May 28, 2025

View reviewed changes

clang/lib/Basic/SanitizerSpecialCaseList.cpp Outdated Show resolved Hide resolved

vitalybuka approved these changes May 28, 2025

View reviewed changes

qinkunbao changed the base branch from users/qinkunbao/spr/main.ubsan-support-srcsanitize-for-multiple-ignorelists to main May 28, 2025 04:19

Merge branch 'main' into users/qinkunbao/spr/ubsan-support-srcsanitiz…

fe466eb

…e-for-multiple-ignorelists

llvmbot added the llvm:support label May 28, 2025

vitalybuka reviewed May 28, 2025

View reviewed changes

clang/lib/Basic/SanitizerSpecialCaseList.cpp Outdated Show resolved Hide resolved

vitalybuka reviewed May 28, 2025

View reviewed changes

llvm/include/llvm/Support/SpecialCaseList.h Outdated Show resolved Hide resolved

qinkunbao added 6 commits May 28, 2025 04:43

Fix some errors.

b454bb1

Created using spr 1.3.6

fix a typo

260bf5d

Created using spr 1.3.6

Fix inSectionBlame

8732a7a

Created using spr 1.3.6

Fix the build

5b6ba2c

Created using spr 1.3.6

Fix comments

12b023f

Created using spr 1.3.6

Merge branch 'main' into users/qinkunbao/spr/ubsan-support-srcsanitiz…

1fe3de7

…e-for-multiple-ignorelists

qinkunbao merged commit 45b874b into main May 28, 2025
8 of 9 checks passed

qinkunbao deleted the users/qinkunbao/spr/ubsan-support-srcsanitize-for-multiple-ignorelists branch May 28, 2025 15:12

vitalybuka reviewed May 28, 2025

View reviewed changes

clang/lib/Basic/NoSanitizeList.cpp Show resolved Hide resolved

shafik reviewed May 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[UBSan] Support src:*=sanitize for multiple ignorelists. #141640

[UBSan] Support src:*=sanitize for multiple ignorelists. #141640

Uh oh!

qinkunbao commented May 27, 2025 •

edited

Loading

Uh oh!

llvmbot commented May 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

vitalybuka left a comment

Uh oh!

Uh oh!

github-actions bot commented May 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

qinkunbao commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

shafik May 30, 2025

Uh oh!

vitalybuka May 30, 2025

Uh oh!

shafik Jun 3, 2025

Uh oh!

Uh oh!

[UBSan] Support src:*=sanitize for multiple ignorelists. #141640

[UBSan] Support src:*=sanitize for multiple ignorelists. #141640

Uh oh!

Conversation

qinkunbao commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitalybuka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qinkunbao commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

shafik May 30, 2025

Choose a reason for hiding this comment

Uh oh!

vitalybuka May 30, 2025

Choose a reason for hiding this comment

Uh oh!

shafik Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qinkunbao commented May 27, 2025 •

edited

Loading

llvmbot commented May 27, 2025 •

edited

Loading

github-actions bot commented May 28, 2025 •

edited

Loading