NoQ updated this revision to Diff 507912.
NoQ added a comment.
Don't recommend enabling suggestions when suggestions are impossible (eg. due
to lack of C++20).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146669/new/
https://reviews.llvm.org/D146669
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Basic/DiagnosticOptions.def
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Driver/Options.td
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fblocks -include %s -verify %s
// RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
// RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN: -fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fno-safe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fno-safe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fno-safe-buffer-usage-suggestions \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -Xclang -fsafe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -Xclang -fsafe-buffer-usage-suggestions \
+// RUN: -fno-safe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fno-safe-buffer-usage-suggestions \
+// RUN: -Xclang -fsafe-buffer-usage-suggestions \
+// RUN: -Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+ // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+ // FIXME: Better "OFF" warning?
+ x[5] = 10; // \
+ // ON-note {{used in buffer access here}} \
+ // OFF-warning{{unsafe buffer access}} \
+ // OFF-note {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+ x += 5; // \
+ // ON-note {{used in pointer arithmetic here}} \
+ // OFF-warning{{unsafe pointer arithmetic}} \
+ // OFF-note {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+ bar(x); // \
+ // ON-warning{{function introduces unsafe buffer manipulation}} \
+ // OFF-warning{{function introduces unsafe buffer manipulation}} \
+ // OFF-note {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -Wno-unused-value -verify %s
void basic(int * x) { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
int *p1 = new int[10]; // not to warn
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
void basic(int * x) {
int tmp;
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions -verify %s
[[clang::unsafe_buffer_usage]]
void deprecatedFunction3();
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
typedef int * Int_ptr_t;
typedef int Int_t;
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits -fsyntax-only \
+// RUN: %s 2>&1 | FileCheck %s
// TODO test we don't mess up vertical whitespace
// TODO test different whitespaces
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// TODO cases where we don't want fixits
Index: clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
===================================================================
--- clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
+++ clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions -verify %s
namespace localVar {
void testRefersPtrLocalVarDecl(int i) {
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2156,9 +2156,11 @@
namespace {
class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
Sema &S;
+ bool SuggestSuggestions; // Recommend -fsafe-buffer-usage-suggestions?
public:
- UnsafeBufferUsageReporter(Sema &S) : S(S) {}
+ UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions)
+ : S(S), SuggestSuggestions(SuggestSuggestions) {}
void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl) override {
@@ -2192,20 +2194,30 @@
}
} else {
if (isa<CallExpr>(Operation)) {
+ // note_unsafe_buffer_operation doesn't have this mode yet.
+ assert(!IsRelatedToDecl && "Not implemented yet!");
MsgParam = 3;
}
Loc = Operation->getBeginLoc();
Range = Operation->getSourceRange();
}
- if (IsRelatedToDecl)
+ if (IsRelatedToDecl) {
+ assert(!SuggestSuggestions &&
+ "Variables blamed for unsafe buffer usage without suggestions!");
S.Diag(Loc, diag::note_unsafe_buffer_operation) << MsgParam << Range;
- else
+ } else {
S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam << Range;
+ if (SuggestSuggestions) {
+ S.Diag(Loc, diag::note_safe_buffer_usage_suggestions_disabled);
+ }
+ }
}
// FIXME: rename to handleUnsafeVariable
void handleFixableVariable(const VarDecl *Variable,
FixItList &&Fixes) override {
+ assert(!SuggestSuggestions &&
+ "Unsafe buffer usage fixits displayed without suggestions!");
S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
<< Variable << (Variable->getType()->isPointerType() ? 0 : 1)
<< Variable->getSourceRange();
@@ -2521,11 +2533,19 @@
// Emit unsafe buffer usage warnings and fixits.
if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, D->getBeginLoc()) ||
!Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
- UnsafeBufferUsageReporter R(S);
- checkUnsafeBufferUsage(
- D, R,
- /*EmitFixits=*/S.getDiagnostics().getDiagnosticOptions().ShowFixits &&
- S.getLangOpts().CPlusPlus20);
+ DiagnosticOptions &DiagOpts = S.getDiagnostics().getDiagnosticOptions();
+
+ bool CanEmitSuggestions = S.getLangOpts().CPlusPlus20;
+ // Doesn't mean you should, just because you can?
+ bool ShouldEmitSuggestions = CanEmitSuggestions &&
+ DiagOpts.ShowSafeBufferUsageSuggestions;
+ bool ShouldEmitFixits = ShouldEmitSuggestions && DiagOpts.ShowFixits;
+ // If suggestions are off, recommend enabling them when applicable.
+ bool ShouldSuggestSuggestions = CanEmitSuggestions &&
+ !DiagOpts.ShowSafeBufferUsageSuggestions;
+
+ UnsafeBufferUsageReporter R(S, ShouldSuggestSuggestions);
+ checkUnsafeBufferUsage(D, R, ShouldEmitSuggestions, ShouldEmitFixits);
}
// If none of the previous checks caused a CFG build, trigger one here
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7021,6 +7021,9 @@
A->claim();
}
+ Args.addOptInFlag(CmdArgs, options::OPT_fsafe_buffer_usage_suggestions,
+ options::OPT_fno_safe_buffer_usage_suggestions);
+
// Setup statistics file output.
SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
if (!StatsFile.empty()) {
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -611,7 +611,8 @@
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
+findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -665,33 +666,37 @@
// clang-format off
M.addMatcher(
stmt(forEveryDescendant(
- eachOf(
- // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable
- // each other (they could if they were put in the same `anyOf` group).
- // We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers
- // match for the same node, so that we can group them
- // in one `anyOf` group (for better performance via short-circuiting).
stmt(anyOf(
-#define FIXABLE_GADGET(x) \
- x ## Gadget::matcher().bind(#x),
-#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
- // Also match DeclStmts because we'll need them when fixing
- // their underlying VarDecls that otherwise don't have
- // any backreferences to DeclStmts.
- declStmt().bind("any_ds")
- )),
- stmt(anyOf(
- // Add Gadget::matcher() for every gadget in the registry.
#define WARNING_GADGET(x) \
- allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&Handler)),
+ allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&Handler)),
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
- // In parallel, match all DeclRefExprs so that to find out
- // whether there are any uncovered by gadgets.
- declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
- )))
- )),
- &CB
+ unless(stmt()) // Avoid a hanging comma.
+ ))
+ )), &CB
);
+
+ if (EmitSuggestions) {
+ M.addMatcher(
+ stmt(forEveryDescendant(
+ stmt(anyOf(
+ // It's ok for a FixableGadget to match a node that was previously
+ // matched by a WarningGadget.
+#define FIXABLE_GADGET(x) \
+ x ## Gadget::matcher().bind(#x),
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+ // Add Gadget::matcher() for every gadget in the registry.
+ // In parallel, match all DeclRefExprs so that to find out
+ // whether there are any uncovered by gadgets.
+ declRefExpr(anyOf(hasPointerType(), hasArrayType()),
+ to(varDecl())).bind("any_dre"),
+ // Also match DeclStmts because we'll need them when fixing
+ // their underlying VarDecls that otherwise don't have
+ // any backreferences to DeclStmts.
+ declStmt().bind("any_ds")
+ ))
+ )), &CB
+ );
+ }
// clang-format on
M.match(*D->getBody(), D->getASTContext());
@@ -1131,17 +1136,30 @@
void clang::checkUnsafeBufferUsage(const Decl *D,
UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions,
bool EmitFixits) {
assert(D && D->getBody());
+ assert(!(EmitFixits && !EmitSuggestions) &&
+ "Unsafe buffer usage fixits requested without suggestions!");
WarningGadgetSets UnsafeOps;
FixableGadgetSets FixablesForUnsafeVars;
DeclUseTracker Tracker;
{
- auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D, Handler);
- UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
- FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
+ auto [FixableGadgets, WarningGadgets, TrackerRes] =
+ findGadgets(D, Handler, EmitSuggestions);
+ if (EmitSuggestions) {
+ UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
+ FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
+ } else {
+ // Don't group anything by variable if we aren't suggesting fixes.
+ UnsafeOps.noVar.reserve(WarningGadgets.size());
+ for (std::unique_ptr<WarningGadget> &G : WarningGadgets) {
+ UnsafeOps.noVar.push_back(std::move(G));
+ }
+ WarningGadgets.clear();
+ }
Tracker = std::move(TrackerRes);
}
Index: clang/include/clang/Driver/Options.td
===================================================================
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1560,6 +1560,11 @@
Group<f_Group>, Flags<[CC1Option]>,
HelpText<"Print a template comparison tree for differing templates">,
MarshallingInfoFlag<DiagnosticOpts<"ShowTemplateTree">>;
+defm safe_buffer_usage_suggestions : BoolFOption<"safe-buffer-usage-suggestions",
+ DiagnosticOpts<"ShowSafeBufferUsageSuggestions">, DefaultFalse,
+ PosFlag<SetTrue, [CC1Option],
+ "Display suggestions to update code associated with -Wunsafe-buffer-usage warnings">,
+ NegFlag<SetFalse>>;
def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">, Group<f_clang_Group>,
HelpText<"Discard value names in LLVM IR">, Flags<[NoXarchOption]>;
def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">, Group<f_clang_Group>,
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11789,6 +11789,8 @@
"used%select{| in pointer arithmetic| in buffer access}0 here">;
def note_unsafe_buffer_variable_fixit : Note<
"change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information">;
+def note_safe_buffer_usage_suggestions_disabled : Note<
+ "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
def err_loongarch_builtin_requires_la32 : Error<
"this builtin requires target: loongarch32">;
Index: clang/include/clang/Basic/DiagnosticOptions.def
===================================================================
--- clang/include/clang/Basic/DiagnosticOptions.def
+++ clang/include/clang/Basic/DiagnosticOptions.def
@@ -95,6 +95,8 @@
/// Column limit for formatting message diagnostics, or 0 if unused.
VALUE_DIAGOPT(MessageLength, 32, 0)
+DIAGOPT(ShowSafeBufferUsageSuggestions, 1, 0)
+
#undef DIAGOPT
#undef ENUM_DIAGOPT
#undef VALUE_DIAGOPT
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -54,7 +54,7 @@
// This function invokes the analysis and allows the caller to react to it
// through the handler class.
void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler,
- bool EmitFixits);
+ bool EmitSuggestions, bool EmitFixits);
namespace internal {
// Tests if any two `FixItHint`s in `FixIts` conflict. Two `FixItHint`s
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
