NoQ updated this revision to Diff 507903.
NoQ added a comment.
Explain the weird variable name.
Confirm that even though `warn_unsafe_buffer_operation` and
`note_unsafe_buffer_operation` have a different number of modes, the mismatch
doesn't cause problems yet. Add an assert to catch the problem when it starts
happening. Add tests to confirm that messages make sense.
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,15 @@
// 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 EmitSuggestions = DiagOpts.ShowSafeBufferUsageSuggestions &&
+ S.getLangOpts().CPlusPlus20;
+ bool EmitFixits = EmitSuggestions && DiagOpts.ShowFixits;
+ // If suggestions are off, recommend enabling them.
+ bool SuggestSuggestions = !EmitSuggestions;
+
+ UnsafeBufferUsageReporter R(S, SuggestSuggestions);
+ checkUnsafeBufferUsage(D, R, EmitSuggestions, EmitFixits);
}
// 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
