[clang] [Clang] Overflow Idiom Exclusions (PR #100272)

Justin Stitt via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 23 16:08:41 PDT 2024


https://github.com/JustinStitt updated https://github.com/llvm/llvm-project/pull/100272

>From 06b702cd38943314b2e6f873e64d70baed6f57f7 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt at google.com>
Date: Tue, 23 Jul 2024 20:21:49 +0000
Subject: [PATCH 1/3] implement idiom exclusions

Add flag `-fno-sanitize-overflow-idioms` which disables integer
overflow/truncation sanitizer instrumentation for common
overflow-dependent code patterns.

Signed-off-by: Justin Stitt <justinstitt at google.com>
---
 clang/include/clang/AST/Expr.h                |   5 +
 clang/include/clang/Basic/LangOptions.def     |   2 +
 clang/include/clang/Driver/Options.td         |  10 ++
 clang/include/clang/Driver/SanitizerArgs.h    |   1 +
 clang/lib/AST/Expr.cpp                        |  53 +++++++
 clang/lib/CodeGen/CGExprScalar.cpp            |  26 +++-
 clang/lib/Driver/SanitizerArgs.cpp            |   7 +
 clang/lib/Driver/ToolChains/Clang.cpp         |   7 +
 .../CodeGen/overflow-idiom-exclusion-fp.c     |  77 ++++++++++
 clang/test/CodeGen/overflow-idiom-exclusion.c | 141 ++++++++++++++++++
 10 files changed, 327 insertions(+), 2 deletions(-)
 create mode 100644 clang/test/CodeGen/overflow-idiom-exclusion-fp.c
 create mode 100644 clang/test/CodeGen/overflow-idiom-exclusion.c

diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 5b813bfc2faf9..c329ee061cf00 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -3860,6 +3860,7 @@ class CStyleCastExpr final
 class BinaryOperator : public Expr {
   enum { LHS, RHS, END_EXPR };
   Stmt *SubExprs[END_EXPR];
+  bool isOverflowIdiom;
 
 public:
   typedef BinaryOperatorKind Opcode;
@@ -4018,6 +4019,10 @@ class BinaryOperator : public Expr {
     return isShiftAssignOp(getOpcode());
   }
 
+  bool ignoreOverflowSanitizers() const {
+    return isOverflowIdiom;
+  }
+
   /// Return true if a binary operator using the specified opcode and operands
   /// would match the 'p = (i8*)nullptr + n' idiom for casting a pointer-sized
   /// integer to a pointer.
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 834a6f6cd43e3..675350a99024a 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -402,6 +402,8 @@ VALUE_LANGOPT(TrivialAutoVarInitMaxSize, 32, 0,
              "stop trivial automatic variable initialization if var size exceeds the specified size (in bytes). Must be greater than 0.")
 ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2, SOB_Undefined,
              "signed integer overflow handling")
+LANGOPT(IgnoreNegationOverflow, 1, 0, "ignore overflow caused by negation")
+LANGOPT(SanitizeOverflowIdioms, 1, 1, "enable instrumentation for common overflow idioms")
 ENUM_LANGOPT(ThreadModel  , ThreadModelKind, 2, ThreadModelKind::POSIX, "Thread Model")
 
 BENIGN_LANGOPT(ArrowDepth, 32, 256,
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 69269cf7537b0..81a6baa1a2eae 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2559,6 +2559,16 @@ defm sanitize_stats : BoolOption<"f", "sanitize-stats",
           "Disable">,
   BothFlags<[], [ClangOption], " sanitizer statistics gathering.">>,
   Group<f_clang_Group>;
+defm sanitize_overflow_idioms : BoolFOption<"sanitize-overflow-idioms",
+  LangOpts<"SanitizeOverflowIdioms">, DefaultTrue,
+  PosFlag<SetTrue, [], [], "Enable">,
+  NegFlag<SetFalse, [], [], "Disable">,
+  BothFlags<[], [ClangOption], " the instrumentation of common overflow idioms">>;
+defm sanitize_negation_overflow : BoolFOption<"sanitize-negation-overflow",
+  LangOpts<"IgnoreNegationOverflow">, DefaultFalse,
+  PosFlag<SetFalse, [], [], "Enable">,
+  NegFlag<SetTrue, [], [], "Disable">,
+  BothFlags<[], [ClangOption], " integer overflow sanitizer instrumentation for negation">>;
 def fsanitize_thread_memory_access : Flag<["-"], "fsanitize-thread-memory-access">,
                                      Group<f_clang_Group>,
                                      HelpText<"Enable memory access instrumentation in ThreadSanitizer (default)">;
diff --git a/clang/include/clang/Driver/SanitizerArgs.h b/clang/include/clang/Driver/SanitizerArgs.h
index 47ef175302679..291739e1221d9 100644
--- a/clang/include/clang/Driver/SanitizerArgs.h
+++ b/clang/include/clang/Driver/SanitizerArgs.h
@@ -64,6 +64,7 @@ class SanitizerArgs {
   // True if cross-dso CFI support if provided by the system (i.e. Android).
   bool ImplicitCfiRuntime = false;
   bool NeedsMemProfRt = false;
+  bool SanitizeOverflowIdioms = true;
   bool HwasanUseAliases = false;
   llvm::AsanDetectStackUseAfterReturnMode AsanUseAfterReturn =
       llvm::AsanDetectStackUseAfterReturnMode::Invalid;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9d5b8167d0ee6..c07560c92100d 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4759,6 +4759,54 @@ ParenListExpr *ParenListExpr::CreateEmpty(const ASTContext &Ctx,
   return new (Mem) ParenListExpr(EmptyShell(), NumExprs);
 }
 
+namespace {
+/// Certain overflow-dependent code idioms can have their integer overflow
+/// sanitization disabled. Check for the common pattern `if (a + b < a)` and
+/// return the resulting BinaryOperator responsible for the addition so we can
+/// elide overflow checks during codegen.
+static std::optional<BinaryOperator *> getOverflowIdiomBinOp(const BinaryOperator *E) {
+  Expr *Addition, *ComparedTo;
+  if (E->getOpcode() == BO_LT) {
+    Addition = E->getLHS();
+    ComparedTo = E->getRHS();
+  } else if (E->getOpcode() == BO_GT) {
+    Addition = E->getRHS();
+    ComparedTo = E->getLHS();
+  } else {
+    return {};
+  }
+
+  const Expr *AddLHS = nullptr, *AddRHS = nullptr;
+  BinaryOperator *BO = dyn_cast<BinaryOperator>(Addition);
+
+  if (BO && BO->getOpcode() == clang::BO_Add) {
+    // now store addends for lookup on other side of '>'
+    AddLHS = BO->getLHS();
+    AddRHS = BO->getRHS();
+  }
+
+  if (!AddLHS || !AddRHS)
+    return {};
+
+  const Decl *LHSDecl, *RHSDecl, *OtherDecl;
+
+  LHSDecl = AddLHS->IgnoreParenImpCasts()->getReferencedDeclOfCallee();
+  RHSDecl = AddRHS->IgnoreParenImpCasts()->getReferencedDeclOfCallee();
+  OtherDecl = ComparedTo->IgnoreParenImpCasts()->getReferencedDeclOfCallee();
+
+  if (!OtherDecl)
+    return {};
+
+  if (!LHSDecl && !RHSDecl)
+    return {};
+
+  if ((LHSDecl && LHSDecl == OtherDecl && LHSDecl != RHSDecl) ||
+      (RHSDecl && RHSDecl == OtherDecl && RHSDecl != LHSDecl))
+    return BO;
+  return {};
+}
+} // namespace
+
 BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
                                Opcode opc, QualType ResTy, ExprValueKind VK,
                                ExprObjectKind OK, SourceLocation opLoc,
@@ -4770,6 +4818,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
   BinaryOperatorBits.OpLoc = opLoc;
   SubExprs[LHS] = lhs;
   SubExprs[RHS] = rhs;
+  if (!Ctx.getLangOpts().SanitizeOverflowIdioms) {
+    std::optional<BinaryOperator*> Result = getOverflowIdiomBinOp(this);
+    if (Result.has_value())
+      Result.value()->isOverflowIdiom = true;
+  }
   BinaryOperatorBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
   if (hasStoredFPFeatures())
     setStoredFPFeatures(FPFeatures);
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index a17d68424bbce..be0307073f3b9 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -25,6 +25,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetInfo.h"
@@ -195,13 +196,22 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx, const BinOpInfo &Op) {
   if (!Op.mayHaveIntegerOverflow())
     return true;
 
+  const UnaryOperator *UO = dyn_cast<UnaryOperator>(Op.E);
+
+  if (UO && UO->getOpcode() == UO_Minus &&
+      Ctx.getLangOpts().IgnoreNegationOverflow)
+    return true;
+
   // If a unary op has a widened operand, the op cannot overflow.
-  if (const auto *UO = dyn_cast<UnaryOperator>(Op.E))
+  if (UO)
     return !UO->canOverflow();
 
   // We usually don't need overflow checks for binops with widened operands.
   // Multiplication with promoted unsigned operands is a special case.
   const auto *BO = cast<BinaryOperator>(Op.E);
+  if (BO->ignoreOverflowSanitizers())
+    return true;
+
   auto OptionalLHSTy = getUnwidenedIntegerType(Ctx, BO->getLHS());
   if (!OptionalLHSTy)
     return false;
@@ -2876,6 +2886,17 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
   } else if (type->isIntegerType()) {
     QualType promotedType;
     bool canPerformLossyDemotionCheck = false;
+
+    // Does this match the pattern of while(i--) {...}? If so, and if
+    // SanitizeOverflowIdioms is disabled, we don't want to enable sanitizer.
+    bool disableSanitizer =
+        (!isInc && !isPre &&
+         !CGF.getContext().getLangOpts().SanitizeOverflowIdioms &&
+         llvm::all_of(CGF.getContext().getParentMapContext().getParents(*E),
+                      [&](const DynTypedNode &Parent) -> bool {
+                        return Parent.get<WhileStmt>();
+                      }));
+
     if (CGF.getContext().isPromotableIntegerType(type)) {
       promotedType = CGF.getContext().getPromotedIntegerType(type);
       assert(promotedType != type && "Shouldn't promote to the same type.");
@@ -2935,7 +2956,8 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
     } else if (E->canOverflow() && type->isSignedIntegerOrEnumerationType()) {
       value = EmitIncDecConsiderOverflowBehavior(E, value, isInc);
     } else if (E->canOverflow() && type->isUnsignedIntegerType() &&
-               CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow)) {
+               CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
+               !disableSanitizer) {
       value = EmitOverflowCheckedBinOp(createBinOpInfoFromIncDec(
           E, value, isInc, E->getFPFeaturesInEffect(CGF.getLangOpts())));
     } else {
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 1fd870b72286e..567840bcfbbdb 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -1069,6 +1069,10 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
                                 options::OPT_fmemory_profile_EQ,
                                 options::OPT_fno_memory_profile, false);
 
+  SanitizeOverflowIdioms =
+      Args.hasFlag(options::OPT_fsanitize_overflow_idioms,
+                   options::OPT_fno_sanitize_overflow_idioms, true);
+
   // Finally, initialize the set of available and recoverable sanitizers.
   Sanitizers.Mask |= Kinds;
   RecoverableSanitizers.Mask |= RecoverableKinds;
@@ -1356,6 +1360,9 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
     CmdArgs.push_back("+tagged-globals");
   }
 
+  if (!SanitizeOverflowIdioms)
+    CmdArgs.push_back("-fno-sanitize-overflow-idioms");
+
   // MSan: Workaround for PR16386.
   // ASan: This is mainly to help LSan with cases such as
   // https://github.com/google/sanitizers/issues/373
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 78936fd634f33..198947c2431c3 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6800,6 +6800,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     }
   }
 
+  if (Args.hasArg(options::OPT_fno_sanitize_overflow_idioms) &&
+      !Args.hasArg(options::OPT_fsanitize_negation_overflow))
+    CmdArgs.push_back("-fno-sanitize-negation-overflow");
+
   if (Args.getLastArg(options::OPT_fapple_kext) ||
       (Args.hasArg(options::OPT_mkernel) && types::isCXX(InputType)))
     CmdArgs.push_back("-fapple-kext");
@@ -6853,6 +6857,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   Args.addOptInFlag(CmdArgs, options::OPT_mspeculative_load_hardening,
                     options::OPT_mno_speculative_load_hardening);
 
+  Args.AddLastArg(CmdArgs, options::OPT_fsanitize_negation_overflow,
+                  options::OPT_fno_sanitize_negation_overflow);
+
   RenderSSPOptions(D, TC, Args, CmdArgs, KernelOrKext);
   RenderSCPOptions(TC, Args, CmdArgs);
   RenderTrivialAutoVarInitOptions(D, TC, Args, CmdArgs);
diff --git a/clang/test/CodeGen/overflow-idiom-exclusion-fp.c b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
new file mode 100644
index 0000000000000..0fa847f733a7f
--- /dev/null
+++ b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
@@ -0,0 +1,77 @@
+// Check for potential false positives from patterns that _almost_ match classic overflow idioms
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -fwrapv -S -emit-llvm -o - | FileCheck %s
+
+extern unsigned a, b, c;
+extern int u, v, w;
+
+extern unsigned some(void);
+
+// Make sure all these still have handler paths, we shouldn't be excluding
+// instrumentation of any "near" idioms.
+void close_but_not_quite(void) {
+  // CHECK: br i1{{.*}}handler.
+  if (a + b > a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a - b < a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a + b < a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a + b + 1 < a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a + b < a + 1)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (b >= a + b)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a + a < a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler.
+  if (a + b == a)
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler
+  if (u + v < u) /* matches overflow idiom, but is signed */
+    c = 9;
+
+  // CHECK: br i1{{.*}}handler
+  // Although this can never actually overflow we are still checking that the
+  // sanitizer instruments it.
+  while (--a)
+    some();
+}
+
+// cvise'd kernel code that caused problems during development
+// CHECK: br i1{{.*}}handler
+typedef unsigned size_t;
+typedef enum { FSE_repeat_none } FSE_repeat;
+typedef enum { ZSTD_defaultAllowed } ZSTD_defaultPolicy_e;
+FSE_repeat ZSTD_selectEncodingType_repeatMode;
+ZSTD_defaultPolicy_e ZSTD_selectEncodingType_isDefaultAllowed;
+size_t ZSTD_NCountCost(void);
+
+void ZSTD_selectEncodingType(void) {
+  size_t basicCost =
+             ZSTD_selectEncodingType_isDefaultAllowed ? ZSTD_NCountCost() : 0,
+         compressedCost = 3 + ZSTD_NCountCost();
+  if (basicCost <= compressedCost)
+    ZSTD_selectEncodingType_repeatMode = FSE_repeat_none;
+}
+
+void function_calls(void) {
+  // CHECK: br i1{{.*}}handler
+  if (some() + b < some())
+    c = 9;
+}
diff --git a/clang/test/CodeGen/overflow-idiom-exclusion.c b/clang/test/CodeGen/overflow-idiom-exclusion.c
new file mode 100644
index 0000000000000..df138fb8f5db8
--- /dev/null
+++ b/clang/test/CodeGen/overflow-idiom-exclusion.c
@@ -0,0 +1,141 @@
+// Ensure some common idioms don't trigger the overflow sanitizers when
+// -fno-sanitize-overflow-idioms is enabled. In many cases, overflow warnings
+// caused by these idioms are seen as "noise" and result in users turning off
+// sanitization all together.
+
+// A pattern like "if (a + b < a)" simply checks for overflow and usually means
+// the user is trying to handle it gracefully.
+
+// Similarly, a pattern resembling "while (i--)" is extremely common and
+// warning on its inevitable overflow can be seen as superfluous. Do note that
+// using "i" in future calculations can be tricky because it will still
+// wrap-around. Using -fno-sanitize-overflow-idioms or not doesn't change this
+// fact -- we just won't warn/trap with sanitizers.
+
+// Another common pattern that, in some cases, is found to be too noisy is
+// unsigned negation, for example:
+// unsigned long A = -1UL;
+
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -fwrapv -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -S -emit-llvm -o - | FileCheck %s --check-prefix=NEGATION
+// RUN: %clang %s -O2 -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fno-sanitize-overflow-idioms -fsanitize-negation-overflow -S -emit-llvm -o - | FileCheck %s --check-prefix=NEGATIONOV
+// CHECK-NOT: br{{.*}}overflow
+
+extern unsigned a, b, c;
+extern unsigned some(void);
+
+void basic_commutativity(void) {
+  if (a + b < a)
+    c = 9;
+  if (a + b < b)
+    c = 9;
+  if (b + a < b)
+    c = 9;
+  if (b + a < a)
+    c = 9;
+  if (a > a + b)
+    c = 9;
+  if (a > b + a)
+    c = 9;
+  if (b > a + b)
+    c = 9;
+  if (b > b + a)
+    c = 9;
+}
+
+void arguments_and_commutativity(unsigned V1, unsigned V2) {
+  if (V1 + V2 < V1)
+    c = 9;
+  if (V1 + V2 < V2)
+    c = 9;
+  if (V2 + V1 < V2)
+    c = 9;
+  if (V2 + V1 < V1)
+    c = 9;
+  if (V1 > V1 + V2)
+    c = 9;
+  if (V1 > V2 + V1)
+    c = 9;
+  if (V2 > V1 + V2)
+    c = 9;
+  if (V2 > V2 + V1)
+    c = 9;
+}
+
+void pointers(unsigned *P1, unsigned *P2, unsigned V1) {
+  if (*P1 + *P2 < *P1)
+    c = 9;
+  if (*P1 + V1 < V1)
+    c = 9;
+  if (V1 + *P2 < *P2)
+    c = 9;
+}
+
+struct OtherStruct {
+  unsigned foo, bar;
+};
+
+struct MyStruct {
+  unsigned base, offset;
+  struct OtherStruct os;
+};
+
+extern struct MyStruct ms;
+
+void structs(void) {
+  if (ms.base + ms.offset < ms.base)
+    c = 9;
+}
+
+void nestedstructs(void) {
+  if (ms.os.foo + ms.os.bar < ms.os.foo)
+    c = 9;
+}
+
+// Normally, this would be folded into a simple call to the overflow handler
+// and a store. Excluding this idiom results in just a store.
+void constants(void) {
+  unsigned base = 4294967295;
+  unsigned offset = 1;
+  if (base + offset < base)
+    c = 9;
+}
+
+void common_while(unsigned i) {
+  // This post-decrement usually causes overflow sanitizers to trip on the very
+  // last operation.
+  while (i--) {
+    some();
+  }
+}
+
+// NEGATION-LABEL,NEGATIONOV-LABEL: define{{.*}}negation_overflow
+// NEGATION-NOT: negate_overflow
+// NEGATIONOV: negate_overflow
+// Normally, these assignments would trip the unsigned overflow sanitizer.
+void negation_overflow(void) {
+#define SOME -1UL
+  unsigned long A = -1UL;
+  unsigned long B = -2UL;
+  unsigned long C = -3UL;
+  unsigned long D = -1337UL;
+  (void)A;(void)B;(void)C;(void)D;
+}
+
+// cvise'd kernel code that caused problems during development due to sign
+// extension
+typedef unsigned long size_t;
+int qnbytes;
+int *key_alloc_key;
+size_t key_alloc_quotalen;
+int *key_alloc(void) {
+  if (qnbytes + key_alloc_quotalen < qnbytes)
+    return key_alloc_key;
+  return key_alloc_key + 3;;
+}
+
+void function_call(void) {
+  if (b + some() < b)
+    c = 9;
+}

>From 1dd1cbb1b13d0b038fa511620af993238a5bb260 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt at google.com>
Date: Tue, 23 Jul 2024 22:44:47 +0000
Subject: [PATCH 2/3] add docs

Signed-off-by: Justin Stitt <justinstitt at google.com>
---
 clang/docs/ReleaseNotes.rst               | 26 ++++++++++++++++++
 clang/docs/UndefinedBehaviorSanitizer.rst | 32 +++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ac1de0db9ce48..443ab1456cfcb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -230,6 +230,32 @@ Moved checkers
 Sanitizers
 ----------
 
+- Added the ``-fno-sanitize-overflow-idioms`` flag which disables integer
+  overflow and truncation sanitizer instrumentation for specific
+  overflow-dependent code patterns. The noise created by these idioms is a
+  large reason as to why large projects refuse to turn on arithmetic
+  sanitizers.
+
+  .. code-block:: c++
+
+     void negation_overflow() {
+       unsigned long foo = -1UL; // No longer causes a negation overflow warning
+       unsigned long bar = -2UL; // and so on...
+     }
+
+     void while_post_decrement() {
+       unsigned char count = 16;
+       while (count--) { /* ... */} // No longer causes unsigned-integer-overflow sanitizer to trip
+     }
+
+     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
+     }
+
+  Note that the ``-fsanitize-overflow-idioms`` flag now also exists but has
+  virtually no function other than to disable an already present
+  ``-fno-sanitize-overflow-idioms``.
+
 Python Binding Changes
 ----------------------
 
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst
index 531d56e313826..b856f601aec81 100644
--- a/clang/docs/UndefinedBehaviorSanitizer.rst
+++ b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -312,6 +312,38 @@ This attribute may not be
 supported by other compilers, so consider using it together with
 ``#if defined(__clang__)``.
 
+Disabling instrumentation of common overflow idioms
+=====================================================
+
+There are certain overflow-dependent code patterns which produce a lot of noise
+for integer overflow/truncation sanitizers. To disable instrumentation for
+these common patterns one should use ``-fno-sanitize-overflow-idioms``. Its
+inverse ``-fsanitize-overflow-idioms`` also exists but has no function other
+than to disable an already present ``-fno-sanitize-overflow-idioms``.
+
+Currently, this option handles three pervasive overflow-dependent code idioms:
+
+.. code-block:: c++
+
+    unsigned long foo = -1UL; // No longer causes a negation overflow warning
+    unsigned long bar = -2UL; // and so on...
+
+.. code-block:: c++
+
+    unsigned char count = 16;
+    while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip
+
+.. code-block:: c++
+
+    if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings,
+                                            // won't be instrumented (same for signed types)
+
+Negated unsigned constants, post-decrements in a while loop condition and
+simple overflow checks are accepted and pervasive code patterns. Existing
+projects that enable more warnings or sanitizers may find that the compiler is
+too noisy. Now, they can use ``-fno-sanitize-overflow-idioms`` with no source
+modifications.
+
 Suppressing Errors in Recompiled Code (Ignorelist)
 --------------------------------------------------
 

>From ac15444b318f38da1e5cc0e8bb7b5af53bc68971 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt at google.com>
Date: Tue, 23 Jul 2024 23:08:23 +0000
Subject: [PATCH 3/3] format

Signed-off-by: Justin Stitt <justinstitt at google.com>
---
 clang/include/clang/AST/Expr.h     | 4 +---
 clang/lib/AST/Expr.cpp             | 5 +++--
 clang/lib/CodeGen/CGExprScalar.cpp | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index c329ee061cf00..e8dc22c51a03d 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -4019,9 +4019,7 @@ class BinaryOperator : public Expr {
     return isShiftAssignOp(getOpcode());
   }
 
-  bool ignoreOverflowSanitizers() const {
-    return isOverflowIdiom;
-  }
+  bool ignoreOverflowSanitizers() const { return isOverflowIdiom; }
 
   /// Return true if a binary operator using the specified opcode and operands
   /// would match the 'p = (i8*)nullptr + n' idiom for casting a pointer-sized
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index c07560c92100d..d963d473442d8 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4764,7 +4764,8 @@ namespace {
 /// sanitization disabled. Check for the common pattern `if (a + b < a)` and
 /// return the resulting BinaryOperator responsible for the addition so we can
 /// elide overflow checks during codegen.
-static std::optional<BinaryOperator *> getOverflowIdiomBinOp(const BinaryOperator *E) {
+static std::optional<BinaryOperator *>
+getOverflowIdiomBinOp(const BinaryOperator *E) {
   Expr *Addition, *ComparedTo;
   if (E->getOpcode() == BO_LT) {
     Addition = E->getLHS();
@@ -4819,7 +4820,7 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
   SubExprs[LHS] = lhs;
   SubExprs[RHS] = rhs;
   if (!Ctx.getLangOpts().SanitizeOverflowIdioms) {
-    std::optional<BinaryOperator*> Result = getOverflowIdiomBinOp(this);
+    std::optional<BinaryOperator *> Result = getOverflowIdiomBinOp(this);
     if (Result.has_value())
       Result.value()->isOverflowIdiom = true;
   }
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index be0307073f3b9..43a3a333fa76f 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -24,8 +24,8 @@
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
-#include "clang/AST/RecordLayout.h"
 #include "clang/AST/ParentMapContext.h"
+#include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetInfo.h"



More information about the cfe-commits mailing list