https://github.com/JustinStitt created https://github.com/llvm/llvm-project/pull/105709
>From @vitalybuka's review on https://github.com/llvm/llvm-project/pull/104889: - [x] remove unused variable in tests - [x] rename `post-decr-while` --> `unsigned-post-decr-while` - [x] split `add-overflow-test` into `add-unsigned-overflow-test` and `add-signed-overflow-test` - [ ] be more clear about defaults within docs - [x] add table to docs Here's a screenshot of the rendered table so you don't have to build the html docs yourself to inspect the layout:  CCs: @vitalybuka >From c55507b3d5552d573068140736e8f904640924e9 Mon Sep 17 00:00:00 2001 From: Justin Stitt <justinst...@google.com> Date: Wed, 21 Aug 2024 19:29:03 -0700 Subject: [PATCH] rename some patterns, rewrite docs >From Vitaly's review on https://github.com/llvm/llvm-project/pull/104889: * remove unused variable in tests * rename `post-decr-while` --> `unsigned-post-decr-while` * split `add-overflow-test` into `add-unsigned-overflow-test` and `add-signed-overflow-test` * be more clear about defaults within docs * add table to docs Signed-off-by: Justin Stitt <justinst...@google.com> --- clang/docs/ReleaseNotes.rst | 20 ++++++--- clang/docs/UndefinedBehaviorSanitizer.rst | 43 ++++++++++++++++--- clang/include/clang/Basic/LangOptions.h | 8 ++-- clang/include/clang/Driver/Options.td | 2 +- clang/lib/AST/Expr.cpp | 35 ++++++++++++--- clang/lib/CodeGen/CGExprScalar.cpp | 2 +- clang/lib/Driver/SanitizerArgs.cpp | 7 ++- clang/lib/Frontend/CompilerInvocation.cpp | 8 +++- .../ignore-overflow-pattern-false-pos.c | 1 - clang/test/CodeGen/ignore-overflow-pattern.c | 7 ++- 10 files changed, 101 insertions(+), 32 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 5c156a9c073a9c..562bf95663581e 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -451,28 +451,34 @@ Sanitizers - Added the ``-fsanitize-undefined-ignore-overflow-pattern`` flag which can be used to disable specific overflow-dependent code patterns. The supported - patterns are: ``add-overflow-test``, ``negated-unsigned-const``, and - ``post-decr-while``. The sanitizer instrumentation can be toggled off for all - available patterns by specifying ``all``. Conversely, you can disable all - exclusions with ``none``. + patterns are: ``add-signed-overflow-test``, ``add-unsigned-overflow-test``, + ``negated-unsigned-const``, and ``unsigned-post-decr-while``. The sanitizer + instrumentation can be toggled off for all available patterns by specifying + ``all``. Conversely, you may disable all exclusions with ``none`` which is + the default. .. code-block:: c++ - /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-overflow-test`` + /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-unsigned-overflow-test`` int common_overflow_check_pattern(unsigned base, unsigned offset) { if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings, won't be instrumented } + /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test`` + int common_overflow_check_pattern_signed(signed int base, signed int offset) { + if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings, won't be instrumented + } + /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=negated-unsigned-const`` void negation_overflow() { unsigned long foo = -1UL; // No longer causes a negation overflow warning unsigned long bar = -2UL; // and so on... } - /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=post-decr-while`` + /// specified with ``-fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while`` void while_post_decrement() { unsigned char count = 16; - while (count--) { /* ... */} // No longer causes unsigned-integer-overflow sanitizer to trip + while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip } Many existing projects have a large amount of these code patterns present. diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst index 1c92907372f83c..973a6f39f296b3 100644 --- a/clang/docs/UndefinedBehaviorSanitizer.rst +++ b/clang/docs/UndefinedBehaviorSanitizer.rst @@ -314,26 +314,55 @@ Currently, this option supports three overflow-dependent code idioms: unsigned long foo = -1UL; // No longer causes a negation overflow warning unsigned long bar = -2UL; // and so on... -``post-decr-while`` +``unsigned-post-decr-while`` .. code-block:: c++ - /// -fsanitize-undefined-ignore-overflow-pattern=post-decr-while + /// -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while unsigned char count = 16; while (count--) { /* ... */ } // No longer causes unsigned-integer-overflow sanitizer to trip -``add-overflow-test`` +``add-signed-overflow-test,add-unsigned-overflow-test`` .. code-block:: c++ - /// -fsanitize-undefined-ignore-overflow-pattern=add-overflow-test + /// -fsanitize-undefined-ignore-overflow-pattern=add-(signed|unsigned)-overflow-test if (base + offset < base) { /* ... */ } // The pattern of `a + b < a`, and other re-orderings, - // won't be instrumented (same for signed types) + // won't be instrumented (signed or unsigned types) + +Of the two arithmetic overflow sanitizer kinds ``unsigned-integer-overflow`` +and ``signed-integer-overflow``, ignored overflow patterns exclude +instrumentation from one of them. + +.. list-table:: Overflow Pattern Types + :widths: 30 50 + :header-rows: 1 + + * - Pattern + - Sanitizer + * - negated-unsigned-const + - unsigned-integer-overflow + * - unsigned-post-decr-while + - unsigned-integer-overflow + * - add-unsigned-overflow-test + - unsigned-integer-overflow + * - add-signed-overflow-test + - signed-integer-overflow + + + +Ignoring overflow patterns has no effect on the definedness of the arithmetic +within the pattern. Since ``add-signed-overflow-test`` works with the +``signed-integer-overflow`` sanitizer instrumentation for code matching this +overflow pattern is ommitted which may cause eager UB optimizations. One may +remedy this with ``-fwrapv`` or ``-fno-strict-overflow``. You can enable all exclusions with ``-fsanitize-undefined-ignore-overflow-pattern=all`` or disable all exclusions -with ``-fsanitize-undefined-ignore-overflow-pattern=none``. Specifying ``none`` -has precedence over other values. +with ``-fsanitize-undefined-ignore-overflow-pattern=none``. If +``-fsanitize-undefined-ignore-overflow-pattern`` is not specified ``none`` is +implied. Specifying ``none`` alongside other values also implies ``none`` as +``none`` has precedence over other values -- including ``all``. Issue Suppression ================= diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h index eb4cb4b5a7e93f..1c80ee89837cb3 100644 --- a/clang/include/clang/Basic/LangOptions.h +++ b/clang/include/clang/Basic/LangOptions.h @@ -375,11 +375,13 @@ class LangOptionsBase { /// Exclude all overflow patterns (below) All = 1 << 1, /// if (a + b < a) - AddOverflowTest = 1 << 2, + AddSignedOverflowTest = 1 << 2, + /// if (a + b < a) + AddUnsignedOverflowTest = 1 << 3, /// -1UL - NegUnsignedConst = 1 << 3, + NegUnsignedConst = 1 << 4, /// while (count--) - PostDecrInWhile = 1 << 4, + PostDecrInWhile = 1 << 5, }; enum class DefaultVisiblityExportMapping { diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index c204062b4f7353..2a6bf8fbc7fbf7 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -2568,7 +2568,7 @@ defm sanitize_stats : BoolOption<"f", "sanitize-stats", def fsanitize_undefined_ignore_overflow_pattern_EQ : CommaJoined<["-"], "fsanitize-undefined-ignore-overflow-pattern=">, HelpText<"Specify the overflow patterns to exclude from artihmetic sanitizer instrumentation">, Visibility<[ClangOption, CC1Option]>, - Values<"none,all,add-overflow-test,negated-unsigned-const,post-decr-while">, + Values<"none,all,add-unsigned-overflow-test,add-signed-overflow-test,negated-unsigned-const,unsigned-post-decr-while">, MarshallingInfoStringVector<LangOpts<"OverflowPatternExclusionValues">>; def fsanitize_thread_memory_access : Flag<["-"], "fsanitize-thread-memory-access">, Group<f_clang_Group>, diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 25ab6f3b2addfb..b3ef70cf386c4e 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -4806,6 +4806,34 @@ getOverflowPatternBinOp(const BinaryOperator *E) { return {}; } +/// Compute and set the OverflowPatternExclusion bit based on whether the +/// BinaryOperator expression matches an overflow pattern being ignored by +/// -fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test or +/// -fsanitize-undefined-ignore-overflow-pattern=add-unsigned-overflow-test +static void computeOverflowPatternExclusion(const ASTContext &Ctx, + const BinaryOperator *E) { + bool AddSignedOverflowTest = Ctx.getLangOpts().isOverflowPatternExcluded( + LangOptions::OverflowPatternExclusionKind::AddSignedOverflowTest); + bool AddUnsignedOverflowTest = Ctx.getLangOpts().isOverflowPatternExcluded( + LangOptions::OverflowPatternExclusionKind::AddUnsignedOverflowTest); + + if (!AddSignedOverflowTest && !AddUnsignedOverflowTest) + return; + + std::optional<BinaryOperator *> Result = getOverflowPatternBinOp(E); + + if (!Result.has_value()) + return; + + QualType AdditionResultType = Result.value()->getType(); + + if (AddSignedOverflowTest && AdditionResultType->isSignedIntegerType()) + Result.value()->setExcludedOverflowPattern(true); + else if (AddUnsignedOverflowTest && + AdditionResultType->isUnsignedIntegerType()) + Result.value()->setExcludedOverflowPattern(true); +} + BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy, ExprValueKind VK, ExprObjectKind OK, SourceLocation opLoc, @@ -4818,12 +4846,7 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, BinaryOperatorBits.ExcludedOverflowPattern = false; SubExprs[LHS] = lhs; SubExprs[RHS] = rhs; - if (Ctx.getLangOpts().isOverflowPatternExcluded( - LangOptions::OverflowPatternExclusionKind::AddOverflowTest)) { - std::optional<BinaryOperator *> Result = getOverflowPatternBinOp(this); - if (Result.has_value()) - Result.value()->BinaryOperatorBits.ExcludedOverflowPattern = true; - } + computeOverflowPatternExclusion(Ctx, this); BinaryOperatorBits.HasFPFeatures = FPFeatures.requiresTrailingStorage(); if (hasStoredFPFeatures()) setStoredFPFeatures(FPFeatures); diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index 3bda254c86adf6..ae600a3d8489c8 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -2785,7 +2785,7 @@ static bool matchesPostDecrInWhile(const UnaryOperator *UO, bool isInc, if (isInc || isPre) return false; - // -fsanitize-undefined-ignore-overflow-pattern=post-decr-while + // -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while if (!Ctx.getLangOpts().isOverflowPatternExcluded( LangOptions::OverflowPatternExclusionKind::PostDecrInWhile)) return false; diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 9d9ad79d51d7f8..4d05375348da54 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -1453,9 +1453,12 @@ static int parseOverflowPatternExclusionValues(const Driver &D, llvm::StringSwitch<int>(Value) .Case("none", LangOptionsBase::None) .Case("all", LangOptionsBase::All) - .Case("add-overflow-test", LangOptionsBase::AddOverflowTest) + .Case("add-unsigned-overflow-test", + LangOptionsBase::AddUnsignedOverflowTest) + .Case("add-signed-overflow-test", + LangOptionsBase::AddSignedOverflowTest) .Case("negated-unsigned-const", LangOptionsBase::NegUnsignedConst) - .Case("post-decr-while", LangOptionsBase::PostDecrInWhile) + .Case("unsigned-post-decr-while", LangOptionsBase::PostDecrInWhile) .Default(0); if (E == 0) D.Diag(clang::diag::err_drv_unsupported_option_argument) diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index f510d3067d4d58..0bb4175dd021ee 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -4274,9 +4274,13 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args, llvm::StringSwitch<unsigned>(A->getValue(i)) .Case("none", LangOptionsBase::None) .Case("all", LangOptionsBase::All) - .Case("add-overflow-test", LangOptionsBase::AddOverflowTest) + .Case("add-unsigned-overflow-test", + LangOptionsBase::AddUnsignedOverflowTest) + .Case("add-signed-overflow-test", + LangOptionsBase::AddSignedOverflowTest) .Case("negated-unsigned-const", LangOptionsBase::NegUnsignedConst) - .Case("post-decr-while", LangOptionsBase::PostDecrInWhile) + .Case("unsigned-post-decr-while", + LangOptionsBase::PostDecrInWhile) .Default(0); } } diff --git a/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c b/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c index 40193e0c3e2671..b4811443b95192 100644 --- a/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c +++ b/clang/test/CodeGen/ignore-overflow-pattern-false-pos.c @@ -3,7 +3,6 @@ // Check for potential false positives from patterns that _almost_ match classic overflow-dependent or overflow-prone code patterns extern unsigned a, b, c; -extern int u, v, w; extern unsigned some(void); diff --git a/clang/test/CodeGen/ignore-overflow-pattern.c b/clang/test/CodeGen/ignore-overflow-pattern.c index b7d700258f8538..c4a9d07b07aaac 100644 --- a/clang/test/CodeGen/ignore-overflow-pattern.c +++ b/clang/test/CodeGen/ignore-overflow-pattern.c @@ -1,8 +1,8 @@ // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=all %s -emit-llvm -o - | FileCheck %s // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=all -fwrapv %s -emit-llvm -o - | FileCheck %s -// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=add-overflow-test %s -emit-llvm -o - | FileCheck %s --check-prefix=ADD +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=add-signed-overflow-test,add-unsigned-overflow-test %s -emit-llvm -o - | FileCheck %s --check-prefix=ADD // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=negated-unsigned-const %s -emit-llvm -o - | FileCheck %s --check-prefix=NEGATE -// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=post-decr-while %s -emit-llvm -o - | FileCheck %s --check-prefix=WHILE +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsanitize=signed-integer-overflow,unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=unsigned-post-decr-while %s -emit-llvm -o - | FileCheck %s --check-prefix=WHILE // Ensure some common overflow-dependent or overflow-prone code patterns don't // trigger the overflow sanitizers. In many cases, overflow warnings caused by @@ -25,6 +25,7 @@ // CHECK-NOT: handle{{.*}}overflow extern unsigned a, b, c; +extern int u, v; extern unsigned some(void); // ADD-LABEL: @basic_commutativity @@ -50,6 +51,8 @@ void basic_commutativity(void) { c = 9; if (b > b + a) c = 9; + if (u + v < u) + c = 9; } // ADD-LABEL: @arguments_and_commutativity _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
