diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 78f180447eef6..7b1c5107a7e04 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1090,16 +1090,6 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler, } M.match(*D->getBody(), D->getASTContext()); - - // Gadgets "claim" variables they're responsible for. Once this loop finishes, - // the tracker will only track DREs that weren't claimed by any gadgets, - // i.e. not understood by the analysis. - for (const auto &G : CB.FixableGadgets) { - for (const auto *DRE : G->getClaimedVarUseSites()) { - CB.Tracker.claimUse(DRE); - } - } - return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; } @@ -2250,6 +2240,33 @@ void clang::checkUnsafeBufferUsage(const Decl *D, return; } + // If no `WarningGadget`s ever matched, there is no unsafe operations in the + // function under the analysis. No need to fix any Fixables. + if (!WarningGadgets.empty()) { + // Gadgets "claim" variables they're responsible for. Once this loop + // finishes, the tracker will only track DREs that weren't claimed by any + // gadgets, i.e. not understood by the analysis. + for (const auto &G : FixableGadgets) { + for (const auto *DRE : G->getClaimedVarUseSites()) { + Tracker.claimUse(DRE); + } + } + } + + // If no `WarningGadget`s ever matched, there is no unsafe operations in the + // function under the analysis. Thus, it early returns here as there is + // nothing needs to be fixed. + // + // Note this claim is based on the assumption that there is no unsafe + // variable whose declaration is invisible from the analyzing function. + // Otherwise, we need to consider if the uses of those unsafe varuables needs + // fix. + // So far, we are not fixing any global variables or class members. And, + // lambdas will be analyzed along with the enclosing function. So this early + // return is correct for now. + if (WarningGadgets.empty()) + return; + UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets)); @@ -2356,6 +2373,34 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } + // Remove a `FixableGadget` if the associated variable is not in the graph + // computed above. We do not want to generate fix-its for such variables, + // since they are neither warned nor reachable from a warned one. + // + // Note a variable is not warned if it is not directly used in any unsafe + // operation. A variable `v` is NOT reachable from an unsafe variable, if it + // does not exist another variable `u` such that `u` is warned and fixing `u` + // (transitively) implicates fixing `v`. + // + // For example, + // ``` + // void f(int * p) { + // int * a = p; *p = 0; + // } + // ``` + // `*p = 0` is a fixable gadget associated with a variable `p` that is neither + // warned nor reachable from a warned one. If we add `a[5] = 0` to the end of + // the function above, `p` becomes reachable from a warned variable. + for (auto I = FixablesForAllVars.byVar.begin(); + I != FixablesForAllVars.byVar.end();) { + // Note `VisitedVars` contain all the variables in the graph: + if (VisitedVars.find((*I).first) == VisitedVars.end()) { + // no such var in graph: + I = FixablesForAllVars.byVar.erase(I); + } else + ++I; + } + Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); FixItsForVariableGroup = diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp index 9301e2a2f1bd6..47ef0b7972951 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp @@ -107,3 +107,128 @@ void noteGoesWithVarDeclWarning() { p[5]; // not to note since the associated warning is suppressed } + + +// Test suppressing interacts with variable grouping: + +// The implication edges are: `a` -> `b` -> `c`. +// If the unsafe operation on `a` is supressed, none of the variables +// will be fixed. +void suppressedVarInGroup() { + int * a; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * b; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * c; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + +#pragma clang unsafe_buffer_usage begin + a[5] = 5; +#pragma clang unsafe_buffer_usage end + a = b; + b = c; +} + +// To show that if `a[5]` is not suppressed in the +// `suppressedVarInGroup` function above, all variables will be fixed. +void suppressedVarInGroup_control() { + int * a; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span a" + int * b; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span b" + int * c; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span c" + + a[5] = 5; + a = b; + b = c; +} + +// The implication edges are: `a` -> `b` -> `c`. +// The unsafe operation on `b` is supressed, while the unsafe +// operation on `a` is not suppressed. Variable `b` will still be +// fixed when fixing `a`. +void suppressedVarInGroup2() { + int * a; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span a" + int * b; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span b" + int * c; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span c" + + a[5] = 5; +#pragma clang unsafe_buffer_usage begin + b[5] = 5; +#pragma clang unsafe_buffer_usage end + a = b; + b = c; +} + +// The implication edges are: `a` -> `b` -> `c`. +// The unsafe operation on `b` is supressed, while the unsafe +// operation on `c` is not suppressed. Only variable `c` will be fixed +// then. +void suppressedVarInGroup3() { + int * a; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * b; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * c; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span c" + + c[5] = 5; +#pragma clang unsafe_buffer_usage begin + b[5] = 5; +#pragma clang unsafe_buffer_usage end + a = b; + b = c; +} + +// The implication edges are: `a` -> `b` -> `c` -> `a`. +// The unsafe operation on `b` is supressed, while the unsafe +// operation on `c` is not suppressed. Since the implication graph +// forms a cycle, all variables will be fixed. +void suppressedVarInGroup4() { + int * a; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span a" + int * b; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span b" + int * c; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span c" + + c[5] = 5; +#pragma clang unsafe_buffer_usage begin + b[5] = 5; +#pragma clang unsafe_buffer_usage end + a = b; + b = c; + c = a; +} + +// There are two groups: `a` -> `b` -> `c` and `d` -> `e` -> `f`. +// Suppressing unsafe operations on variables in one group does not +// affect other groups. +void suppressedVarInGroup5() { + int * a; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * b; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * c; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * d; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span d" + int * e; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span e" + int * f; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span f" + +#pragma clang unsafe_buffer_usage begin + a[5] = 5; +#pragma clang unsafe_buffer_usage end + a = b; + b = c; + + d[5] = 5; + d = e; + e = f; +}