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 <justinst...@google.com>
Date: Tue, 23 Jul 2024 20:21:49 +0000
Subject: [PATCH 1/5] 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 <justinst...@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 <justinst...@google.com>
Date: Tue, 23 Jul 2024 22:44:47 +0000
Subject: [PATCH 2/5] add docs
Signed-off-by: Justin Stitt <justinst...@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 <justinst...@google.com>
Date: Tue, 23 Jul 2024 23:08:23 +0000
Subject: [PATCH 3/5] format
Signed-off-by: Justin Stitt <justinst...@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"
>From 3ef8d3cc55c5414a55280088aed4193496597bc0 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinst...@google.com>
Date: Fri, 26 Jul 2024 00:06:03 +0000
Subject: [PATCH 4/5] add support for comma-separated pattern specification
Signed-off-by: Justin Stitt <justinst...@google.com>
---
clang/include/clang/AST/Expr.h | 4 +-
clang/include/clang/Basic/LangOptions.h | 28 ++++++++++++++
clang/include/clang/Driver/Options.td | 10 ++---
clang/include/clang/Driver/SanitizerArgs.h | 1 +
clang/lib/AST/Expr.cpp | 5 ++-
clang/lib/CodeGen/CGExprScalar.cpp | 11 ++++--
clang/lib/Driver/SanitizerArgs.cpp | 37 +++++++++++++++++++
clang/lib/Driver/ToolChains/Clang.cpp | 10 ++---
clang/lib/Frontend/CompilerInvocation.cpp | 16 ++++++++
.../CodeGen/overflow-idiom-exclusion-fp.c | 4 +-
clang/test/CodeGen/overflow-idiom-exclusion.c | 30 ++++++++++-----
11 files changed, 124 insertions(+), 32 deletions(-)
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index e8dc22c51a03d..b25a4368aa152 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -3860,7 +3860,7 @@ class CStyleCastExpr final
class BinaryOperator : public Expr {
enum { LHS, RHS, END_EXPR };
Stmt *SubExprs[END_EXPR];
- bool isOverflowIdiom;
+ bool ExcludedOverflowPattern;
public:
typedef BinaryOperatorKind Opcode;
@@ -4019,7 +4019,7 @@ class BinaryOperator : public Expr {
return isShiftAssignOp(getOpcode());
}
- bool ignoreOverflowSanitizers() const { return isOverflowIdiom; }
+ bool ignoreOverflowSanitizers() const { return ExcludedOverflowPattern; }
/// 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/include/clang/Basic/LangOptions.h
b/clang/include/clang/Basic/LangOptions.h
index 91f1c2f2e6239..d57a9d7f31c73 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -367,6 +367,21 @@ class LangOptionsBase {
PerThread,
};
+ /// Exclude certain code patterns from being instrumented by arithmetic
+ /// overflow sanitizers
+ enum OverflowPatternExclusionKind {
+ /// Don't exclude any overflow patterns from sanitizers
+ None = 1 << 0,
+ /// Exclude all overflow patterns (below)
+ All = 1 << 1,
+ /// if (a + b < a)
+ AddOverflowTest = 1 << 2,
+ /// -1UL
+ NegUnsignedConst = 1 << 3,
+ /// while (count--)
+ PostDecrInWhile = 1 << 4,
+ };
+
enum class DefaultVisiblityExportMapping {
None,
/// map only explicit default visibilities to exported
@@ -555,6 +570,11 @@ class LangOptions : public LangOptionsBase {
/// The default stream kind used for HIP kernel launching.
GPUDefaultStreamKind GPUDefaultStream;
+ /// Which overflow patterns should be excluded from sanitizer instrumentation
+ int OverflowPatternExclusionMask = 0;
+
+ std::vector<std::string> OverflowPatternExclusionValues;
+
/// The seed used by the randomize structure layout feature.
std::string RandstructSeed;
@@ -630,6 +650,14 @@ class LangOptions : public LangOptionsBase {
return MSCompatibilityVersion >= MajorVersion * 100000U;
}
+ bool isOverflowPatternExcluded(OverflowPatternExclusionKind Kind) const {
+ if (OverflowPatternExclusionMask & OverflowPatternExclusionKind::None)
+ return false;
+ if (OverflowPatternExclusionMask & OverflowPatternExclusionKind::All)
+ return true;
+ return OverflowPatternExclusionMask & Kind;
+ }
+
/// Reset all of the options that are not considered when building a
/// module.
void resetNonModularOptions();
diff --git a/clang/include/clang/Driver/Options.td
b/clang/include/clang/Driver/Options.td
index 81a6baa1a2eae..18ddfd27b3585 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2564,11 +2564,11 @@ defm sanitize_overflow_idioms :
BoolFOption<"sanitize-overflow-idioms",
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_overflow_pattern_exclusion_EQ : CommaJoined<["-"],
"fsanitize-overflow-pattern-exclusion=">,
+ 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">,
+ MarshallingInfoStringVector<LangOpts<"OverflowPatternExclusionValues">>;
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 291739e1221d9..1e23a6a6f85f1 100644
--- a/clang/include/clang/Driver/SanitizerArgs.h
+++ b/clang/include/clang/Driver/SanitizerArgs.h
@@ -33,6 +33,7 @@ class SanitizerArgs {
std::vector<std::string> BinaryMetadataIgnorelistFiles;
int CoverageFeatures = 0;
int BinaryMetadataFeatures = 0;
+ int OverflowPatternExclusions = 0;
int MsanTrackOrigins = 0;
bool MsanUseAfterDtor = true;
bool MsanParamRetval = true;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index d963d473442d8..8069cf80ea505 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4819,10 +4819,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx,
Expr *lhs, Expr *rhs,
BinaryOperatorBits.OpLoc = opLoc;
SubExprs[LHS] = lhs;
SubExprs[RHS] = rhs;
- if (!Ctx.getLangOpts().SanitizeOverflowIdioms) {
+ if (Ctx.getLangOpts().isOverflowPatternExcluded(
+ LangOptions::OverflowPatternExclusionKind::AddOverflowTest)) {
std::optional<BinaryOperator *> Result = getOverflowIdiomBinOp(this);
if (Result.has_value())
- Result.value()->isOverflowIdiom = true;
+ Result.value()->ExcludedOverflowPattern = true;
}
BinaryOperatorBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
if (hasStoredFPFeatures())
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 43a3a333fa76f..7148bf73ce2e2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -198,8 +198,11 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx,
const BinOpInfo &Op) {
const UnaryOperator *UO = dyn_cast<UnaryOperator>(Op.E);
+ // TODO: rename or rework NegUnsignedConst because it doesn't only work on
+ // constants.
if (UO && UO->getOpcode() == UO_Minus &&
- Ctx.getLangOpts().IgnoreNegationOverflow)
+ Ctx.getLangOpts().isOverflowPatternExcluded(
+ LangOptions::OverflowPatternExclusionKind::NegUnsignedConst))
return true;
// If a unary op has a widened operand, the op cannot overflow.
@@ -2887,11 +2890,11 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const
UnaryOperator *E, LValue LV,
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.
+ // Is the pattern "while (i--)" and overflow exclusion?
bool disableSanitizer =
(!isInc && !isPre &&
- !CGF.getContext().getLangOpts().SanitizeOverflowIdioms &&
+ CGF.getContext().getLangOpts().isOverflowPatternExcluded(
+ LangOptions::OverflowPatternExclusionKind::PostDecrInWhile) &&
llvm::all_of(CGF.getContext().getParentMapContext().getParents(*E),
[&](const DynTypedNode &Parent) -> bool {
return Parent.get<WhileStmt>();
diff --git a/clang/lib/Driver/SanitizerArgs.cpp
b/clang/lib/Driver/SanitizerArgs.cpp
index 567840bcfbbdb..3b1c0b645ffbe 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -119,6 +119,10 @@ static SanitizerMask parseArgValues(const Driver &D, const
llvm::opt::Arg *A,
static int parseCoverageFeatures(const Driver &D, const llvm::opt::Arg *A,
bool DiagnoseErrors);
+static int parseOverflowPatternExclusionValues(const Driver &D,
+ const llvm::opt::Arg *A,
+ bool DiagnoseErrors);
+
/// Parse -f(no-)?sanitize-metadata= flag values, diagnosing any invalid
/// components. Returns OR of members of \c BinaryMetadataFeature enumeration.
static int parseBinaryMetadataFeatures(const Driver &D, const llvm::opt::Arg
*A,
@@ -788,6 +792,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
<< "fsanitize-trap=cfi";
}
+ for (const auto *Arg :
+ Args.filtered(options::OPT_fsanitize_overflow_pattern_exclusion_EQ)) {
+ Arg->claim();
+ OverflowPatternExclusions |=
+ parseOverflowPatternExclusionValues(D, Arg, DiagnoseErrors);
+ }
+
// Parse -f(no-)?sanitize-coverage flags if coverage is supported by the
// enabled sanitizers.
for (const auto *Arg : Args) {
@@ -1245,6 +1256,10 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const
llvm::opt::ArgList &Args,
addSpecialCaseListOpt(Args, CmdArgs,
"-fsanitize-system-ignorelist=",
SystemIgnorelistFiles);
+ if (OverflowPatternExclusions)
+ Args.AddAllArgs(CmdArgs,
+ options::OPT_fsanitize_overflow_pattern_exclusion_EQ);
+
if (MsanTrackOrigins)
CmdArgs.push_back(Args.MakeArgString("-fsanitize-memory-track-origins=" +
Twine(MsanTrackOrigins)));
@@ -1433,6 +1448,28 @@ SanitizerMask parseArgValues(const Driver &D, const
llvm::opt::Arg *A,
return Kinds;
}
+static int parseOverflowPatternExclusionValues(const Driver &D,
+ const llvm::opt::Arg *A,
+ bool DiagnoseErrors) {
+ int Exclusions = 0;
+ for (int i = 0, n = A->getNumValues(); i != n; ++i) {
+ const char *Value = A->getValue(i);
+ int E =
+ llvm::StringSwitch<int>(Value)
+ .Case("none", LangOptionsBase::None)
+ .Case("all", LangOptionsBase::All)
+ .Case("add-overflow-test", LangOptionsBase::AddOverflowTest)
+ .Case("negated-unsigned-const", LangOptionsBase::NegUnsignedConst)
+ .Case("post-decr-while", LangOptionsBase::PostDecrInWhile)
+ .Default(0);
+ if (E == 0)
+ D.Diag(clang::diag::err_drv_unsupported_option_argument)
+ << A->getSpelling() << Value;
+ Exclusions |= E;
+ }
+ return Exclusions;
+}
+
int parseCoverageFeatures(const Driver &D, const llvm::opt::Arg *A,
bool DiagnoseErrors) {
assert(A->getOption().matches(options::OPT_fsanitize_coverage) ||
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp
b/clang/lib/Driver/ToolChains/Clang.cpp
index 198947c2431c3..77836fa9c9719 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6800,10 +6800,6 @@ 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");
@@ -6857,9 +6853,6 @@ 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);
@@ -7709,6 +7702,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction
&JA,
Args.AddLastArg(CmdArgs, options::OPT_fgpu_default_stream_EQ);
}
+ Args.AddAllArgs(CmdArgs,
+ options::OPT_fsanitize_overflow_pattern_exclusion_EQ);
+
Args.AddLastArg(CmdArgs, options::OPT_foffload_uniform_block,
options::OPT_fno_offload_uniform_block);
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp
b/clang/lib/Frontend/CompilerInvocation.cpp
index f6b6c44a4cab6..0d257a40859fd 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -4248,6 +4248,22 @@ bool CompilerInvocation::ParseLangArgs(LangOptions
&Opts, ArgList &Args,
Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
}
+ if (auto *A = Args.getLastArg(OPT_fsanitize_overflow_pattern_exclusion_EQ)) {
+ for (int i = 0, n = A->getNumValues(); i != n; ++i) {
+ StringRef Value = A->getValue(i);
+ if (Value == "none")
+ Opts.OverflowPatternExclusionMask |= LangOptionsBase::None;
+ else if (Value == "all")
+ Opts.OverflowPatternExclusionMask |= LangOptionsBase::All;
+ else if (Value == "add-overflow-test")
+ Opts.OverflowPatternExclusionMask |= LangOptionsBase::AddOverflowTest;
+ else if (Value == "negated-unsigned-const")
+ Opts.OverflowPatternExclusionMask |= LangOptionsBase::NegUnsignedConst;
+ else if (Value == "post-decr-while")
+ Opts.OverflowPatternExclusionMask |= LangOptionsBase::PostDecrInWhile;
+ }
+ }
+
// Parse -fsanitize= arguments.
parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
Diags, Opts.Sanitize);
diff --git a/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
index 0fa847f733a7f..7bd7f94a3f039 100644
--- a/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
+++ b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
@@ -1,6 +1,6 @@
// 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
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -fwrapv -S -emit-llvm -o - |
FileCheck %s
extern unsigned a, b, c;
extern int u, v, w;
diff --git a/clang/test/CodeGen/overflow-idiom-exclusion.c
b/clang/test/CodeGen/overflow-idiom-exclusion.c
index df138fb8f5db8..d09e8c364963c 100644
--- a/clang/test/CodeGen/overflow-idiom-exclusion.c
+++ b/clang/test/CodeGen/overflow-idiom-exclusion.c
@@ -16,12 +16,25 @@
// 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
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -fwrapv -S -emit-llvm -o - |
FileCheck %s
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=add-overflow-test -S -emit-llvm -o - |
FileCheck %s --check-prefix=ADD
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=negated-unsigned-const -S -emit-llvm -o -
| FileCheck %s --check-prefix=NEGATE
+// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=post-decr-while -S -emit-llvm -o - |
FileCheck %s --check-prefix=WHILE
+// CHECK-NOT: handle{{.*}}overflow
+
+// ADD: usub.with.overflow
+// ADD: negate_overflow
+// ADD-NOT: handler.add_overflow
+
+// NEGATE: handler.add_overflow
+// NEGATE: usub.with.overflow
+// NEGATE-NOT: negate_overflow
+
+// WHILE: handler.add_overflow
+// WHILE: negate_overflow
+// WHILE-NOT: usub.with.overflow
extern unsigned a, b, c;
extern unsigned some(void);
@@ -110,16 +123,13 @@ void common_while(unsigned i) {
}
}
-// 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) {
+void negation(void) {
#define SOME -1UL
unsigned long A = -1UL;
unsigned long B = -2UL;
unsigned long C = -3UL;
- unsigned long D = -1337UL;
+ unsigned long D = -SOME;
(void)A;(void)B;(void)C;(void)D;
}
>From 53a31f4b0f98e5cff2e0d3b58803f41f89a2a171 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinst...@google.com>
Date: Mon, 29 Jul 2024 20:19:30 +0000
Subject: [PATCH 5/5] remove more instances of 'idiom'
Signed-off-by: Justin Stitt <justinst...@google.com>
---
clang/docs/ReleaseNotes.rst | 28 ++++----
clang/docs/UndefinedBehaviorSanitizer.rst | 64 ++++++++++---------
clang/include/clang/Driver/Options.td | 5 --
clang/include/clang/Driver/SanitizerArgs.h | 1 -
clang/lib/AST/Expr.cpp | 6 +-
clang/lib/CodeGen/CGExprScalar.cpp | 4 +-
clang/lib/Driver/SanitizerArgs.cpp | 7 --
.../CodeGen/overflow-idiom-exclusion-fp.c | 20 ++++--
clang/test/CodeGen/overflow-idiom-exclusion.c | 11 ++--
9 files changed, 71 insertions(+), 75 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 443ab1456cfcb..83dac9cd6275c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -230,31 +230,35 @@ 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.
+- Added the ``-fsanitize-overflow-pattern-exclusion=`` 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``.
.. code-block:: c++
+ /// specified with
``-fsanitize-overflow-pattern-exclusion=add-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-overflow-pattern-exclusion=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-overflow-pattern-exclusion=post-decr-while``
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``.
+ Many existing projects have a large amount of these code patterns present.
+ This new flag should allow those projects to enable integer sanitizers with
+ less noise.
Python Binding Changes
----------------------
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst
b/clang/docs/UndefinedBehaviorSanitizer.rst
index b856f601aec81..2b15d14ff449b 100644
--- a/clang/docs/UndefinedBehaviorSanitizer.rst
+++ b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -293,56 +293,58 @@ To silence reports from unsigned integer overflow, you
can set
``-fsanitize-recover=unsigned-integer-overflow``, is particularly useful for
providing fuzzing signal without blowing up logs.
-Issue Suppression
-=================
-
-UndefinedBehaviorSanitizer is not expected to produce false positives.
-If you see one, look again; most likely it is a true positive!
-
-Disabling Instrumentation with ``__attribute__((no_sanitize("undefined")))``
-----------------------------------------------------------------------------
-
-You disable UBSan checks for particular functions with
-``__attribute__((no_sanitize("undefined")))``. You can use all values of
-``-fsanitize=`` flag in this attribute, e.g. if your function deliberately
-contains possible signed integer overflow, you can use
-``__attribute__((no_sanitize("signed-integer-overflow")))``.
-
-This attribute may not be
-supported by other compilers, so consider using it together with
-``#if defined(__clang__)``.
-
-Disabling instrumentation of common overflow idioms
-=====================================================
+Disabling instrumentation for common overflow patterns
+------------------------------------------------------
-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``.
+There are certain overflow-dependent or overflow-prone code patterns which
+produce a lot of noise for integer overflow/truncation sanitizers. To disable
+instrumentation for these common patterns one should use
+``-fsanitize-overflow-pattern-exclusion=``.
-Currently, this option handles three pervasive overflow-dependent code idioms:
+Currently, this option supports three pervasive overflow-dependent code idioms:
.. code-block:: c++
+ /// -fsanitize-overflow-pattern-exclusion=negated-unsigned-const
unsigned long foo = -1UL; // No longer causes a negation overflow warning
unsigned long bar = -2UL; // and so on...
.. code-block:: c++
+ /// -fsanitize-overflow-pattern-exclusion=post-decr-while
unsigned char count = 16;
while (count--) { /* ... */ } // No longer causes
unsigned-integer-overflow sanitizer to trip
.. code-block:: c++
+ /// -fsanitize-overflow-pattern-exclusion=add-overflow-test
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.
+simple overflow checks are accepted and pervasive code patterns. You can enable
+all exclusions with ``-fsanitize-overflow-pattern-exclusion=all`` or disable
+all exclusions with ``-fsanitize-overflow-pattern-exclusion=none``. Specifying
+``none`` has precedence over other values.
+
+Issue Suppression
+=================
+
+UndefinedBehaviorSanitizer is not expected to produce false positives.
+If you see one, look again; most likely it is a true positive!
+
+Disabling Instrumentation with ``__attribute__((no_sanitize("undefined")))``
+----------------------------------------------------------------------------
+
+You disable UBSan checks for particular functions with
+``__attribute__((no_sanitize("undefined")))``. You can use all values of
+``-fsanitize=`` flag in this attribute, e.g. if your function deliberately
+contains possible signed integer overflow, you can use
+``__attribute__((no_sanitize("signed-integer-overflow")))``.
+
+This attribute may not be
+supported by other compilers, so consider using it together with
+``#if defined(__clang__)``.
Suppressing Errors in Recompiled Code (Ignorelist)
--------------------------------------------------
diff --git a/clang/include/clang/Driver/Options.td
b/clang/include/clang/Driver/Options.td
index 18ddfd27b3585..bad36b404fc9a 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2559,11 +2559,6 @@ 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">>;
def fsanitize_overflow_pattern_exclusion_EQ : CommaJoined<["-"],
"fsanitize-overflow-pattern-exclusion=">,
HelpText<"Specify the overflow patterns to exclude from artihmetic sanitizer
instrumentation">,
Visibility<[ClangOption, CC1Option]>,
diff --git a/clang/include/clang/Driver/SanitizerArgs.h
b/clang/include/clang/Driver/SanitizerArgs.h
index 1e23a6a6f85f1..e64ec463ca890 100644
--- a/clang/include/clang/Driver/SanitizerArgs.h
+++ b/clang/include/clang/Driver/SanitizerArgs.h
@@ -65,7 +65,6 @@ 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 8069cf80ea505..ffe10ecb24173 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4760,12 +4760,12 @@ ParenListExpr *ParenListExpr::CreateEmpty(const
ASTContext &Ctx,
}
namespace {
-/// Certain overflow-dependent code idioms can have their integer overflow
+/// Certain overflow-dependent code patterns 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) {
+getOverflowPatternBinOp(const BinaryOperator *E) {
Expr *Addition, *ComparedTo;
if (E->getOpcode() == BO_LT) {
Addition = E->getLHS();
@@ -4821,7 +4821,7 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx,
Expr *lhs, Expr *rhs,
SubExprs[RHS] = rhs;
if (Ctx.getLangOpts().isOverflowPatternExcluded(
LangOptions::OverflowPatternExclusionKind::AddOverflowTest)) {
- std::optional<BinaryOperator *> Result = getOverflowIdiomBinOp(this);
+ std::optional<BinaryOperator *> Result = getOverflowPatternBinOp(this);
if (Result.has_value())
Result.value()->ExcludedOverflowPattern = true;
}
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 7148bf73ce2e2..6d43b1b772f8f 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -198,9 +198,7 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx,
const BinOpInfo &Op) {
const UnaryOperator *UO = dyn_cast<UnaryOperator>(Op.E);
- // TODO: rename or rework NegUnsignedConst because it doesn't only work on
- // constants.
- if (UO && UO->getOpcode() == UO_Minus &&
+ if (UO && UO->getOpcode() == UO_Minus && UO->isIntegerConstantExpr(Ctx) &&
Ctx.getLangOpts().isOverflowPatternExcluded(
LangOptions::OverflowPatternExclusionKind::NegUnsignedConst))
return true;
diff --git a/clang/lib/Driver/SanitizerArgs.cpp
b/clang/lib/Driver/SanitizerArgs.cpp
index 3b1c0b645ffbe..a63ee944fd1bb 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -1080,10 +1080,6 @@ 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;
@@ -1375,9 +1371,6 @@ 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/test/CodeGen/overflow-idiom-exclusion-fp.c
b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
index 7bd7f94a3f039..621d8f43babd8 100644
--- a/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
+++ b/clang/test/CodeGen/overflow-idiom-exclusion-fp.c
@@ -1,4 +1,4 @@
-// Check for potential false positives from patterns that _almost_ match
classic overflow idioms
+// Check for potential false positives from patterns that _almost_ match
classic overflow-dependent or overflow-prone code patterns
// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -S -emit-llvm -o - | FileCheck %s
// RUN: %clang %s -O2
-fsanitize=signed-integer-overflow,unsigned-integer-overflow
-fsanitize-overflow-pattern-exclusion=all -fwrapv -S -emit-llvm -o - |
FileCheck %s
@@ -8,7 +8,8 @@ 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.
+// instrumentation of any "near" patterns.
+// CHECK-LABEL: close_but_not_quite
void close_but_not_quite(void) {
// CHECK: br i1{{.*}}handler.
if (a + b > a)
@@ -26,6 +27,7 @@ void close_but_not_quite(void) {
if (a + b + 1 < a)
c = 9;
+ // CHECK: br i1{{.*}}handler.
// CHECK: br i1{{.*}}handler.
if (a + b < a + 1)
c = 9;
@@ -42,10 +44,6 @@ void close_but_not_quite(void) {
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.
@@ -54,7 +52,6 @@ void close_but_not_quite(void) {
}
// 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;
@@ -62,6 +59,8 @@ FSE_repeat ZSTD_selectEncodingType_repeatMode;
ZSTD_defaultPolicy_e ZSTD_selectEncodingType_isDefaultAllowed;
size_t ZSTD_NCountCost(void);
+// CHECK-LABEL: ZSTD_selectEncodingType
+// CHECK: br i1{{.*}}handler
void ZSTD_selectEncodingType(void) {
size_t basicCost =
ZSTD_selectEncodingType_isDefaultAllowed ? ZSTD_NCountCost() : 0,
@@ -70,8 +69,15 @@ void ZSTD_selectEncodingType(void) {
ZSTD_selectEncodingType_repeatMode = FSE_repeat_none;
}
+// CHECK-LABEL: function_calls
void function_calls(void) {
// CHECK: br i1{{.*}}handler
if (some() + b < some())
c = 9;
}
+
+// CHECK-LABEL: not_quite_a_negated_unsigned_const
+void not_quite_a_negated_unsigned_const(void) {
+ // CHECK: br i1{{.*}}handler
+ a = -b;
+}
diff --git a/clang/test/CodeGen/overflow-idiom-exclusion.c
b/clang/test/CodeGen/overflow-idiom-exclusion.c
index d09e8c364963c..e753343542a06 100644
--- a/clang/test/CodeGen/overflow-idiom-exclusion.c
+++ b/clang/test/CodeGen/overflow-idiom-exclusion.c
@@ -1,6 +1,6 @@
-// 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
+// Ensure some common overflow-dependent or overflow-prone code patterns don't
+// trigger the overflow sanitizers. In many cases, overflow warnings caused by
+// these patterns 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
@@ -9,8 +9,7 @@
// 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.
+// wrap-around.
// Another common pattern that, in some cases, is found to be too noisy is
// unsigned negation, for example:
@@ -107,7 +106,7 @@ void nestedstructs(void) {
}
// Normally, this would be folded into a simple call to the overflow handler
-// and a store. Excluding this idiom results in just a store.
+// and a store. Excluding this pattern results in just a store.
void constants(void) {
unsigned base = 4294967295;
unsigned offset = 1;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
