From 295fe0bd438209831071ffbacf003c4941f31b90 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 20 Aug 2024 13:13:44 -0700 Subject: [PATCH] [Clang] Re-land Overflow Pattern Exclusions (#104889) Introduce "-fsanitize-undefined-ignore-overflow-pattern=" which can be used to disable sanitizer instrumentation for common overflow-dependent code patterns. For a wide selection of projects, proper overflow sanitization could help catch bugs and solve security vulnerabilities. Unfortunately, in some cases the integer overflow sanitizers are too noisy for their users and are often left disabled. Providing users with a method to disable sanitizer instrumentation of common patterns could mean more projects actually utilize the sanitizers in the first place. One such project that has opted to not use integer overflow (or truncation) sanitizers is the Linux Kernel. There has been some discussion[1] recently concerning mitigation strategies for unexpected arithmetic overflow. This discussion is still ongoing and a succinct article[2] accurately sums up the discussion. In summary, many Kernel developers do not want to introduce more arithmetic wrappers when most developers understand the code patterns as they are. Patterns like: if (base + offset < base) { ... } or while (i--) { ... } or #define SOME -1UL are extremely common in a code base like the Linux Kernel. It is perhaps too much to ask of kernel developers to use arithmetic wrappers in these cases. For example: while (wrapping_post_dec(i)) { ... } which wraps some builtin would not fly. This would incur too many changes to existing code; the code churn would be too much, at least too much to justify turning on overflow sanitizers. Currently, this commit tackles three pervasive idioms: 1. "if (a + b < a)" or some logically-equivalent re-ordering like "if (a > b + a)" 2. "while (i--)" (for unsigned) a post-decrement always overflows here 3. "-1UL, -2UL, etc" negation of unsigned constants will always overflow The patterns that are excluded can be chosen from the following list: - add-overflow-test - post-decr-while - negated-unsigned-const These can be enabled with a comma-separated list: -fsanitize-undefined-ignore-overflow-pattern=add-overflow-test,negated-unsigned-const "all" or "none" may also be used to specify that all patterns should be excluded or that none should be. [1] https://lore.kernel.org/all/202404291502.612E0A10@keescook/ [2] https://lwn.net/Articles/979747/ CCs: @efriedma-quic @kees @jyknight @fmayer @vitalybuka Signed-off-by: Justin Stitt Co-authored-by: Bill Wendling --- clang/docs/ReleaseNotes.rst | 30 +++ clang/docs/UndefinedBehaviorSanitizer.rst | 42 ++++ clang/include/clang/AST/Expr.h | 10 + clang/include/clang/AST/Stmt.h | 5 + clang/include/clang/Basic/LangOptions.h | 28 +++ clang/include/clang/Driver/Options.td | 5 + clang/include/clang/Driver/SanitizerArgs.h | 1 + clang/lib/AST/Expr.cpp | 55 ++++++ clang/lib/CodeGen/CGExprScalar.cpp | 41 +++- clang/lib/Driver/SanitizerArgs.cpp | 39 ++++ clang/lib/Driver/ToolChains/Clang.cpp | 3 + clang/lib/Frontend/CompilerInvocation.cpp | 14 ++ clang/lib/Serialization/ASTReaderStmt.cpp | 1 + clang/lib/Serialization/ASTWriterStmt.cpp | 1 + .../ignore-overflow-pattern-false-pos.c | 63 ++++++ clang/test/CodeGen/ignore-overflow-pattern.c | 181 ++++++++++++++++++ 16 files changed, 517 insertions(+), 2 deletions(-) create mode 100644 clang/test/CodeGen/ignore-overflow-pattern-false-pos.c create mode 100644 clang/test/CodeGen/ignore-overflow-pattern.c diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 8435ca3c4f5050..1df3f0e7e75ca3 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -438,6 +438,36 @@ Moved checkers 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``. + + .. code-block:: c++ + + /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=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-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`` + void while_post_decrement() { + unsigned char count = 16; + while (count--) { /* ... */} // No longer causes unsigned-integer-overflow sanitizer to trip + } + + 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 ---------------------- - Fixed an issue that led to crashes when calling ``Type.get_exception_specification_kind``. diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst index 531d56e313826c..1c92907372f83c 100644 --- a/clang/docs/UndefinedBehaviorSanitizer.rst +++ b/clang/docs/UndefinedBehaviorSanitizer.rst @@ -293,6 +293,48 @@ 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. +Disabling instrumentation for common overflow patterns +------------------------------------------------------ + +There are certain overflow-dependent or overflow-prone code patterns which +produce a lot of noise for integer overflow/truncation sanitizers. Negated +unsigned constants, post-decrements in a while loop condition and simple +overflow checks are accepted and pervasive code patterns. However, the signal +received from sanitizers instrumenting these code patterns may be too noisy for +some projects. To disable instrumentation for these common patterns one should +use ``-fsanitize-undefined-ignore-overflow-pattern=``. + +Currently, this option supports three overflow-dependent code idioms: + +``negated-unsigned-const`` + +.. code-block:: c++ + + /// -fsanitize-undefined-ignore-overflow-pattern=negated-unsigned-const + unsigned long foo = -1UL; // No longer causes a negation overflow warning + unsigned long bar = -2UL; // and so on... + +``post-decr-while`` + +.. code-block:: c++ + + /// -fsanitize-undefined-ignore-overflow-pattern=post-decr-while + unsigned char count = 16; + while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip + +``add-overflow-test`` + +.. code-block:: c++ + + /// -fsanitize-undefined-ignore-overflow-pattern=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) + +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. + Issue Suppression ================= diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index 5b813bfc2faf90..7bacf028192c65 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -3888,6 +3888,7 @@ class BinaryOperator : public Expr { /// Construct an empty binary operator. explicit BinaryOperator(EmptyShell Empty) : Expr(BinaryOperatorClass, Empty) { BinaryOperatorBits.Opc = BO_Comma; + BinaryOperatorBits.ExcludedOverflowPattern = false; } public: @@ -4043,6 +4044,15 @@ class BinaryOperator : public Expr { void setHasStoredFPFeatures(bool B) { BinaryOperatorBits.HasFPFeatures = B; } bool hasStoredFPFeatures() const { return BinaryOperatorBits.HasFPFeatures; } + /// Set and get the bit that informs arithmetic overflow sanitizers whether + /// or not they should exclude certain BinaryOperators from instrumentation + void setExcludedOverflowPattern(bool B) { + BinaryOperatorBits.ExcludedOverflowPattern = B; + } + bool hasExcludedOverflowPattern() const { + return BinaryOperatorBits.ExcludedOverflowPattern; + } + /// Get FPFeatures from trailing storage FPOptionsOverride getStoredFPFeatures() const { assert(hasStoredFPFeatures()); diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h index bbd7634bcc3bfb..f1a2aac0a8b2f8 100644 --- a/clang/include/clang/AST/Stmt.h +++ b/clang/include/clang/AST/Stmt.h @@ -650,6 +650,11 @@ class alignas(void *) Stmt { LLVM_PREFERRED_TYPE(bool) unsigned HasFPFeatures : 1; + /// Whether or not this BinaryOperator should be excluded from integer + /// overflow sanitization. + LLVM_PREFERRED_TYPE(bool) + unsigned ExcludedOverflowPattern : 1; + SourceLocation OpLoc; }; diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h index 91f1c2f2e6239e..eb4cb4b5a7e93f 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 + unsigned OverflowPatternExclusionMask = 0; + + std::vector 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 625a8303b9bf15..c204062b4f7353 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -2565,6 +2565,11 @@ defm sanitize_stats : BoolOption<"f", "sanitize-stats", "Disable">, BothFlags<[], [ClangOption], " sanitizer statistics gathering.">>, Group; +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">, + MarshallingInfoStringVector>; def fsanitize_thread_memory_access : Flag<["-"], "fsanitize-thread-memory-access">, 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 47ef175302679f..e64ec463ca8907 100644 --- a/clang/include/clang/Driver/SanitizerArgs.h +++ b/clang/include/clang/Driver/SanitizerArgs.h @@ -33,6 +33,7 @@ class SanitizerArgs { std::vector 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 9d5b8167d0ee62..25ab6f3b2addfb 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -4759,6 +4759,53 @@ ParenListExpr *ParenListExpr::CreateEmpty(const ASTContext &Ctx, return new (Mem) ParenListExpr(EmptyShell(), NumExprs); } +/// 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 +getOverflowPatternBinOp(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(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 {}; +} + BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy, ExprValueKind VK, ExprObjectKind OK, SourceLocation opLoc, @@ -4768,8 +4815,15 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, assert(!isCompoundAssignmentOp() && "Use CompoundAssignOperator for compound assignments"); BinaryOperatorBits.OpLoc = opLoc; + BinaryOperatorBits.ExcludedOverflowPattern = false; SubExprs[LHS] = lhs; SubExprs[RHS] = rhs; + if (Ctx.getLangOpts().isOverflowPatternExcluded( + LangOptions::OverflowPatternExclusionKind::AddOverflowTest)) { + std::optional Result = getOverflowPatternBinOp(this); + if (Result.has_value()) + Result.value()->BinaryOperatorBits.ExcludedOverflowPattern = true; + } BinaryOperatorBits.HasFPFeatures = FPFeatures.requiresTrailingStorage(); if (hasStoredFPFeatures()) setStoredFPFeatures(FPFeatures); @@ -4782,6 +4836,7 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, FPOptionsOverride FPFeatures, bool dead2) : Expr(CompoundAssignOperatorClass, ResTy, VK, OK) { BinaryOperatorBits.Opc = opc; + BinaryOperatorBits.ExcludedOverflowPattern = false; assert(isCompoundAssignmentOp() && "Use CompoundAssignOperator for compound assignments"); BinaryOperatorBits.OpLoc = opLoc; diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index 84392745ea6144..3bda254c86adf6 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -24,6 +24,7 @@ #include "clang/AST/Attr.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/Expr.h" +#include "clang/AST/ParentMapContext.h" #include "clang/AST/RecordLayout.h" #include "clang/AST/StmtVisitor.h" #include "clang/Basic/CodeGenOptions.h" @@ -195,13 +196,24 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx, const BinOpInfo &Op) { if (!Op.mayHaveIntegerOverflow()) return true; + const UnaryOperator *UO = dyn_cast(Op.E); + + if (UO && UO->getOpcode() == UO_Minus && + Ctx.getLangOpts().isOverflowPatternExcluded( + LangOptions::OverflowPatternExclusionKind::NegUnsignedConst) && + UO->isIntegerConstantExpr(Ctx)) + return true; + // If a unary op has a widened operand, the op cannot overflow. - if (const auto *UO = dyn_cast(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(Op.E); + if (BO->hasExcludedOverflowPattern()) + return true; + auto OptionalLHSTy = getUnwidenedIntegerType(Ctx, BO->getLHS()); if (!OptionalLHSTy) return false; @@ -2766,6 +2778,26 @@ llvm::Value *ScalarExprEmitter::EmitIncDecConsiderOverflowBehavior( llvm_unreachable("Unknown SignedOverflowBehaviorTy"); } +/// For the purposes of overflow pattern exclusion, does this match the +/// "while(i--)" pattern? +static bool matchesPostDecrInWhile(const UnaryOperator *UO, bool isInc, + bool isPre, ASTContext &Ctx) { + if (isInc || isPre) + return false; + + // -fsanitize-undefined-ignore-overflow-pattern=post-decr-while + if (!Ctx.getLangOpts().isOverflowPatternExcluded( + LangOptions::OverflowPatternExclusionKind::PostDecrInWhile)) + return false; + + // all Parents (usually just one) must be a WhileStmt + for (const auto &Parent : Ctx.getParentMapContext().getParents(*UO)) + if (!Parent.get()) + return false; + + return true; +} + namespace { /// Handles check and update for lastprivate conditional variables. class OMPLastprivateConditionalUpdateRAII { @@ -2877,6 +2909,10 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, } else if (type->isIntegerType()) { QualType promotedType; bool canPerformLossyDemotionCheck = false; + + bool excludeOverflowPattern = + matchesPostDecrInWhile(E, isInc, isPre, CGF.getContext()); + if (CGF.getContext().isPromotableIntegerType(type)) { promotedType = CGF.getContext().getPromotedIntegerType(type); assert(promotedType != type && "Shouldn't promote to the same type."); @@ -2936,7 +2972,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) && + !excludeOverflowPattern) { 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 1fd870b72286e5..9d9ad79d51d7f8 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -119,6 +119,12 @@ static SanitizerMask parseArgValues(const Driver &D, const llvm::opt::Arg *A, static int parseCoverageFeatures(const Driver &D, const llvm::opt::Arg *A, bool DiagnoseErrors); +/// Parse -fsanitize-undefined-ignore-overflow-pattern= flag values, diagnosing +/// any invalid values. Returns a mask of excluded overflow patterns. +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 +794,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, << "fsanitize-trap=cfi"; } + for (const auto *Arg : Args.filtered( + options::OPT_fsanitize_undefined_ignore_overflow_pattern_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) { @@ -1241,6 +1254,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_undefined_ignore_overflow_pattern_EQ); + if (MsanTrackOrigins) CmdArgs.push_back(Args.MakeArgString("-fsanitize-memory-track-origins=" + Twine(MsanTrackOrigins))); @@ -1426,6 +1443,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(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 04ef50585f2002..f7c2f485d3fc11 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -7775,6 +7775,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA, Args.AddLastArg(CmdArgs, options::OPT_fgpu_default_stream_EQ); } + Args.AddAllArgs(CmdArgs, + options::OPT_fsanitize_undefined_ignore_overflow_pattern_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 e3911c281985b7..f510d3067d4d58 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -4267,6 +4267,20 @@ 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_undefined_ignore_overflow_pattern_EQ)) { + for (int i = 0, n = A->getNumValues(); i != n; ++i) { + Opts.OverflowPatternExclusionMask |= + llvm::StringSwitch(A->getValue(i)) + .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); + } + } + // Parse -fsanitize= arguments. parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ), Diags, Opts.Sanitize); diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp index a33f2a41a65497..8ae07907a04aba 100644 --- a/clang/lib/Serialization/ASTReaderStmt.cpp +++ b/clang/lib/Serialization/ASTReaderStmt.cpp @@ -1128,6 +1128,7 @@ void ASTStmtReader::VisitBinaryOperator(BinaryOperator *E) { (BinaryOperator::Opcode)CurrentUnpackingBits->getNextBits(/*Width=*/6)); bool hasFP_Features = CurrentUnpackingBits->getNextBit(); E->setHasStoredFPFeatures(hasFP_Features); + E->setExcludedOverflowPattern(CurrentUnpackingBits->getNextBit()); E->setLHS(Record.readSubExpr()); E->setRHS(Record.readSubExpr()); E->setOperatorLoc(readSourceLocation()); diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp index 038616a675b727..c292d0a789c7cd 100644 --- a/clang/lib/Serialization/ASTWriterStmt.cpp +++ b/clang/lib/Serialization/ASTWriterStmt.cpp @@ -1063,6 +1063,7 @@ void ASTStmtWriter::VisitBinaryOperator(BinaryOperator *E) { CurrentPackingBits.addBits(E->getOpcode(), /*Width=*/6); bool HasFPFeatures = E->hasStoredFPFeatures(); CurrentPackingBits.addBit(HasFPFeatures); + CurrentPackingBits.addBit(E->hasExcludedOverflowPattern()); Record.AddStmt(E->getLHS()); Record.AddStmt(E->getRHS()); Record.AddSourceLocation(E->getOperatorLoc()); diff --git a/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c b/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c new file mode 100644 index 00000000000000..40193e0c3e2671 --- /dev/null +++ b/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c @@ -0,0 +1,63 @@ +// 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 + +// 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); + +// Make sure all these still have handler paths, we shouldn't be excluding +// 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) + 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. + // 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 + while (--a) + some(); +} + +// 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/ignore-overflow-pattern.c b/clang/test/CodeGen/ignore-overflow-pattern.c new file mode 100644 index 00000000000000..b7d700258f8538 --- /dev/null +++ b/clang/test/CodeGen/ignore-overflow-pattern.c @@ -0,0 +1,181 @@ +// 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=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 + +// 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 +// 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. + +// Another common pattern that, in some cases, is found to be too noisy is +// unsigned negation, for example: +// unsigned long A = -1UL; + + +// CHECK-NOT: handle{{.*}}overflow + +extern unsigned a, b, c; +extern unsigned some(void); + +// ADD-LABEL: @basic_commutativity +// WHILE-LABEL: @basic_commutativity +// NEGATE-LABEL: @basic_commutativity +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +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; +} + +// ADD-LABEL: @arguments_and_commutativity +// WHILE-LABEL: @arguments_and_commutativity +// NEGATE-LABEL: @arguments_and_commutativity +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +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; +} + +// ADD-LABEL: @pointers +// WHILE-LABEL: @pointers +// NEGATE-LABEL: @pointers +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +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; + +// ADD-LABEL: @structs +// WHILE-LABEL: @structs +// NEGATE-LABEL: @structs +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +void structs(void) { + if (ms.base + ms.offset < ms.base) + c = 9; +} + +// ADD-LABEL: @nestedstructs +// WHILE-LABEL: @nestedstructs +// NEGATE-LABEL: @nestedstructs +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +void nestedstructs(void) { + if (ms.os.foo + ms.os.bar < ms.os.foo) + c = 9; +} + +// ADD-LABEL: @constants +// WHILE-LABEL: @constants +// NEGATE-LABEL: @constants +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +// Normally, this would be folded into a simple call to the overflow handler +// and a store. Excluding this pattern results in just a store. +void constants(void) { + unsigned base = 4294967295; + unsigned offset = 1; + if (base + offset < base) + c = 9; +} +// ADD-LABEL: @common_while +// NEGATE-LABEL: @common_while +// WHILE-LABEL: @common_while +// ADD: usub.with.overflow +// NEGATE: usub.with.overflow +// WHILE: %dec = add i32 %0, -1 +void common_while(unsigned i) { + // This post-decrement usually causes overflow sanitizers to trip on the very + // last operation. + while (i--) { + some(); + } +} + +// ADD-LABEL: @negation +// NEGATE-LABEL: @negation +// WHILE-LABEL @negation +// ADD: negate_overflow +// NEGATE-NOT: negate_overflow +// WHILE: negate_overflow +// Normally, these assignments would trip the unsigned overflow sanitizer. +void negation(void) { +#define SOME -1UL + unsigned long A = -1UL; + unsigned long B = -2UL; + unsigned long C = -SOME; + (void)A;(void)B;(void)C; +} + + +// ADD-LABEL: @function_call +// WHILE-LABEL: @function_call +// NEGATE-LABEL: @function_call +// WHILE: handler.add_overflow +// NEGATE: handler.add_overflow +// ADD-NOT: handler.add_overflow +void function_call(void) { + if (b + some() < b) + c = 9; +}