[Clang] Overflow Pattern Exclusion - rename some patterns, enhance docs (#105709)

JustinStitt · vitalybuka · web-flow · commit 76236fafda19 · 2024-08-23T23:33:23.000-07:00
From @vitalybuka's review on #104889: - [x] remove unused variable in tests - [x] rename `post-decr-while` --> `unsigned-post-decr-while` - [x] split `add-overflow-test` into `add-unsigned-overflow-test` and `add-signed-overflow-test` - [x] be more clear about defaults within docs - [x] add table to docs Here's a screenshot of the rendered table so you don't have to build the html docs yourself to inspect the layout: ![image](https://github.com/user-attachments/assets/5d3497c4-5f5a-4579-b29b-96a0fd192faa) CCs: @vitalybuka --------- Signed-off-by: Justin Stitt <justinstitt@google.com> Co-authored-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -466,28 +466,34 @@ Sanitizers
 
 - Added the ``-fsanitize-undefined-ignore-overflow-pattern`` flag which can be
   used to disable specific overflow-dependent code patterns. The supported
-  patterns are: ``add-overflow-test``, ``negated-unsigned-const``, and
-  ``post-decr-while``. The sanitizer instrumentation can be toggled off for all
-  available patterns by specifying ``all``. Conversely, you can disable all
-  exclusions with ``none``.
+  patterns are: ``add-signed-overflow-test``, ``add-unsigned-overflow-test``,
+  ``negated-unsigned-const``, and ``unsigned-post-decr-while``. The sanitizer
+  instrumentation can be toggled off for all available patterns by specifying
+  ``all``. Conversely, you may disable all exclusions with ``none`` which is
+  the default.
 
   .. code-block:: c++
 
-     /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-overflow-test``
+     /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-unsigned-overflow-test``
      int common_overflow_check_pattern(unsigned base, unsigned offset) {
        if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings, won't be instrumented
      }
 
+     /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test``
+     int common_overflow_check_pattern_signed(signed int base, signed int offset) {
+       if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings, won't be instrumented
+     }
+
      /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=negated-unsigned-const``
      void negation_overflow() {
        unsigned long foo = -1UL; // No longer causes a negation overflow warning
        unsigned long bar = -2UL; // and so on...
      }
 
-     /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=post-decr-while``
+     /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while``
      void while_post_decrement() {
        unsigned char count = 16;
-       while (count--) { /* ... */} // No longer causes unsigned-integer-overflow sanitizer to trip
+       while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip
      }
 
   Many existing projects have a large amount of these code patterns present.
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -314,26 +314,49 @@ Currently, this option supports three overflow-dependent code idioms:
     unsigned long foo = -1UL; // No longer causes a negation overflow warning
     unsigned long bar = -2UL; // and so on...
 
-``post-decr-while``
+``unsigned-post-decr-while``
 
 .. code-block:: c++
 
-    /// -fsanitize-undefined-ignore-overflow-pattern=post-decr-while
+    /// -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while
     unsigned char count = 16;
     while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip
 
-``add-overflow-test``
+``add-signed-overflow-test,add-unsigned-overflow-test``
 
 .. code-block:: c++
 
-    /// -fsanitize-undefined-ignore-overflow-pattern=add-overflow-test
+    /// -fsanitize-undefined-ignore-overflow-pattern=add-(signed|unsigned)-overflow-test
     if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings,
-                                            // won't be instrumented (same for signed types)
+                                            // won't be instrumented (signed or unsigned types)
+
+.. list-table:: Overflow Pattern Types
+   :widths: 30 50
+   :header-rows: 1
+
+   * - Pattern
+     - Sanitizer
+   * - negated-unsigned-const
+     - unsigned-integer-overflow
+   * - unsigned-post-decr-while
+     - unsigned-integer-overflow
+   * - add-unsigned-overflow-test
+     - unsigned-integer-overflow
+   * - add-signed-overflow-test
+     - signed-integer-overflow
+
+
+
+Note: ``add-signed-overflow-test`` suppresses only the check for Undefined
+Behavior. Eager Undefined Behavior optimizations are still possible. One may
+remedy this with ``-fwrapv`` or ``-fno-strict-overflow``.
 
 You can enable all exclusions with
 ``-fsanitize-undefined-ignore-overflow-pattern=all`` or disable all exclusions
-with ``-fsanitize-undefined-ignore-overflow-pattern=none``. Specifying ``none``
-has precedence over other values.
+with ``-fsanitize-undefined-ignore-overflow-pattern=none``. If
+``-fsanitize-undefined-ignore-overflow-pattern`` is not specified ``none`` is
+implied. Specifying ``none`` alongside other values also implies ``none`` as
+``none`` has precedence over other values -- including ``all``.
 
 Issue Suppression
 =================
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
@@ -375,11 +375,13 @@ class LangOptionsBase {
     /// Exclude all overflow patterns (below)
     All = 1 << 1,
     /// if (a + b < a)
-    AddOverflowTest = 1 << 2,
+    AddSignedOverflowTest = 1 << 2,
+    /// if (a + b < a)
+    AddUnsignedOverflowTest = 1 << 3,
     /// -1UL
-    NegUnsignedConst = 1 << 3,
+    NegUnsignedConst = 1 << 4,
     /// while (count--)
-    PostDecrInWhile = 1 << 4,
+    PostDecrInWhile = 1 << 5,
   };
 
   enum class DefaultVisiblityExportMapping {
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
@@ -2570,7 +2570,7 @@ defm sanitize_stats : BoolOption<"f", "sanitize-stats",
 def fsanitize_undefined_ignore_overflow_pattern_EQ : CommaJoined<["-"], "fsanitize-undefined-ignore-overflow-pattern=">,
   HelpText<"Specify the overflow patterns to exclude from artihmetic sanitizer instrumentation">,
   Visibility<[ClangOption, CC1Option]>,
-  Values<"none,all,add-overflow-test,negated-unsigned-const,post-decr-while">,
+  Values<"none,all,add-unsigned-overflow-test,add-signed-overflow-test,negated-unsigned-const,unsigned-post-decr-while">,
   MarshallingInfoStringVector<LangOpts<"OverflowPatternExclusionValues">>;
 def fsanitize_thread_memory_access : Flag<["-"], "fsanitize-thread-memory-access">,
                                      Group<f_clang_Group>,
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
@@ -4806,6 +4806,26 @@ getOverflowPatternBinOp(const BinaryOperator *E) {
   return {};
 }
 
+/// Compute and set the OverflowPatternExclusion bit based on whether the
+/// BinaryOperator expression matches an overflow pattern being ignored by
+/// -fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test or
+/// -fsanitize-undefined-ignore-overflow-pattern=add-unsigned-overflow-test
+static void computeOverflowPatternExclusion(const ASTContext &Ctx,
+                                            const BinaryOperator *E) {
+  std::optional<BinaryOperator *> Result = getOverflowPatternBinOp(E);
+  if (!Result.has_value())
+    return;
+  QualType AdditionResultType = Result.value()->getType();
+
+  if ((AdditionResultType->isSignedIntegerType() &&
+       Ctx.getLangOpts().isOverflowPatternExcluded(
+           LangOptions::OverflowPatternExclusionKind::AddSignedOverflowTest)) ||
+      (AdditionResultType->isUnsignedIntegerType() &&
+       Ctx.getLangOpts().isOverflowPatternExcluded(
+           LangOptions::OverflowPatternExclusionKind::AddUnsignedOverflowTest)))
+    Result.value()->setExcludedOverflowPattern(true);
+}
+
 BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
                                Opcode opc, QualType ResTy, ExprValueKind VK,
                                ExprObjectKind OK, SourceLocation opLoc,
@@ -4818,12 +4838,7 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
   BinaryOperatorBits.ExcludedOverflowPattern = false;
   SubExprs[LHS] = lhs;
   SubExprs[RHS] = rhs;
-  if (Ctx.getLangOpts().isOverflowPatternExcluded(
-          LangOptions::OverflowPatternExclusionKind::AddOverflowTest)) {
-    std::optional<BinaryOperator *> Result = getOverflowPatternBinOp(this);
-    if (Result.has_value())
-      Result.value()->BinaryOperatorBits.ExcludedOverflowPattern = true;
-  }
+  computeOverflowPatternExclusion(Ctx, this);
   BinaryOperatorBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
   if (hasStoredFPFeatures())
     setStoredFPFeatures(FPFeatures);
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2785,7 +2785,7 @@ static bool matchesPostDecrInWhile(const UnaryOperator *UO, bool isInc,
   if (isInc || isPre)
     return false;
 
-  // -fsanitize-undefined-ignore-overflow-pattern=post-decr-while
+  // -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while
   if (!Ctx.getLangOpts().isOverflowPatternExcluded(
           LangOptions::OverflowPatternExclusionKind::PostDecrInWhile))
     return false;
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
@@ -1457,9 +1457,12 @@ static int parseOverflowPatternExclusionValues(const Driver &D,
         llvm::StringSwitch<int>(Value)
             .Case("none", LangOptionsBase::None)
             .Case("all", LangOptionsBase::All)
-            .Case("add-overflow-test", LangOptionsBase::AddOverflowTest)
+            .Case("add-unsigned-overflow-test",
+                  LangOptionsBase::AddUnsignedOverflowTest)
+            .Case("add-signed-overflow-test",
+                  LangOptionsBase::AddSignedOverflowTest)
             .Case("negated-unsigned-const", LangOptionsBase::NegUnsignedConst)
-            .Case("post-decr-while", LangOptionsBase::PostDecrInWhile)
+            .Case("unsigned-post-decr-while", LangOptionsBase::PostDecrInWhile)
             .Default(0);
     if (E == 0)
       D.Diag(clang::diag::err_drv_unsupported_option_argument)
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -4274,9 +4274,13 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
           llvm::StringSwitch<unsigned>(A->getValue(i))
               .Case("none", LangOptionsBase::None)
               .Case("all", LangOptionsBase::All)
-              .Case("add-overflow-test", LangOptionsBase::AddOverflowTest)
+              .Case("add-unsigned-overflow-test",
+                    LangOptionsBase::AddUnsignedOverflowTest)
+              .Case("add-signed-overflow-test",
+                    LangOptionsBase::AddSignedOverflowTest)
               .Case("negated-unsigned-const", LangOptionsBase::NegUnsignedConst)
-              .Case("post-decr-while", LangOptionsBase::PostDecrInWhile)
+              .Case("unsigned-post-decr-while",
+                    LangOptionsBase::PostDecrInWhile)
               .Default(0);
     }
   }
diff --git a/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c b/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c
@@ -3,7 +3,6 @@
 
 // Check for potential false positives from patterns that _almost_ match classic overflow-dependent or overflow-prone code patterns
 extern unsigned a, b, c;
-extern int u, v, w;
 
 extern unsigned some(void);
 
diff --git a/clang/test/CodeGen/ignore-overflow-pattern.c b/clang/test/CodeGen/ignore-overflow-pattern.c
@@ -1,8 +1,8 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=all %s -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=all -fwrapv %s -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=add-overflow-test %s -emit-llvm -o - | FileCheck %s --check-prefix=ADD
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test,add-unsigned-overflow-test %s -emit-llvm -o - | FileCheck %s --check-prefix=ADD
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=negated-unsigned-const %s -emit-llvm -o - | FileCheck %s --check-prefix=NEGATE
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=post-decr-while %s -emit-llvm -o - | FileCheck %s --check-prefix=WHILE
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while %s -emit-llvm -o - | FileCheck %s --check-prefix=WHILE
 
 // Ensure some common overflow-dependent or overflow-prone code patterns don't
 // trigger the overflow sanitizers. In many cases, overflow warnings caused by
@@ -25,6 +25,7 @@
 // CHECK-NOT: handle{{.*}}overflow
 
 extern unsigned a, b, c;
+extern int u, v;
 extern unsigned some(void);
 
 // ADD-LABEL: @basic_commutativity
@@ -50,6 +51,8 @@ void basic_commutativity(void) {
     c = 9;
   if (b > b + a)
     c = 9;
+  if (u + v < u)
+    c = 9;
 }
 
 // ADD-LABEL: @arguments_and_commutativity
