-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[-Wunsafe-buffer-usage] Fix debug notes for unclaimed DREs #80787
[-Wunsafe-buffer-usage] Fix debug notes for unclaimed DREs #80787
Conversation
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (jkorous-apple) ChangesDebug notes for unclaimed DeclRefExpr should report any DRE of an unsafe variable that is not covered by a Fixable (i. e. fixit for the particular AST pattern isn't implemented for whatever reason). Currently not all unclaimed DeclRefExpr-s are reported which is a bug. The debug notes report only those DREs where the referred VarDecl has at least one other DeclRefExpr which is claimed (covered by a fixit). If there is an unsafe VarDecl that has exactly one DRE and the DRE isn't claimed then the debug note about missing fixit won't be emitted. That is because the debug note is emitted from within a loop over set of successfully matched FixableGadgets which by-definition is missing those DRE that are not matched at all. The new code simply iterates over all unsafe VarDecls and all of their unclaimed DREs. Full diff: https://github.com/llvm/llvm-project/pull/80787.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b9969..6feff1d508e67 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2870,19 +2870,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
#endif
it = FixablesForAllVars.byVar.erase(it);
} else if (Tracker.hasUnclaimedUses(it->first)) {
-#ifndef NDEBUG
- auto AllUnclaimed = Tracker.getUnclaimedUses(it->first);
- for (auto UnclaimedDRE : AllUnclaimed) {
- std::string UnclaimedUseTrace =
- getDREAncestorString(UnclaimedDRE, D->getASTContext());
-
- Handler.addDebugNoteForVar(
- it->first, UnclaimedDRE->getBeginLoc(),
- ("failed to produce fixit for '" + it->first->getNameAsString() +
- "' : has an unclaimed use\nThe unclaimed DRE trace: " +
- UnclaimedUseTrace));
- }
-#endif
it = FixablesForAllVars.byVar.erase(it);
} else if (it->first->isInitCapture()) {
#ifndef NDEBUG
@@ -2897,6 +2884,26 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
+#ifndef NDEBUG
+ for (const auto& it : UnsafeOps.byVar) {
+ const VarDecl* const UnsafeVD = it.first;
+ auto UnclaimedDREs = Tracker.getUnclaimedUses(UnsafeVD);
+ if (UnclaimedDREs.empty())
+ continue;
+ const auto UnfixedVDName = UnsafeVD->getNameAsString();
+ for (const clang::DeclRefExpr* UnclaimedDRE : UnclaimedDREs) {
+ std::string UnclaimedUseTrace =
+ getDREAncestorString(UnclaimedDRE, D->getASTContext());
+
+ Handler.addDebugNoteForVar(
+ UnsafeVD, UnclaimedDRE->getBeginLoc(),
+ ("failed to produce fixit for '" + UnfixedVDName +
+ "' : has an unclaimed use\nThe unclaimed DRE trace: " +
+ UnclaimedUseTrace));
+ }
+ }
+#endif
+
// Fixpoint iteration for pointer assignments
using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
DepMapTy DependenciesMap{};
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index e08f70d97e391..5fff0854d4546 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -98,3 +98,10 @@ void test_struct_claim_use() {
x[6] = 8; // expected-warning{{unsafe buffer access}}
x++; // expected-warning{{unsafe pointer arithmetic}}
}
+
+void use(int*);
+void array2d(int idx) {
+ int buffer[10][5]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
+ use(buffer[idx]); // expected-note{{used in buffer access here}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for 'buffer' : has an unclaimed use}}
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
e4d9594
to
1abbea3
Compare
Debug notes for unclaimed DeclRefExpr should report any DRE of an unsafe variable that is not covered by a Fixable (i. e. fixit for the particular AST pattern isn't implemented for whatever reason). Currently not all unclaimed DeclRefExpr-s are reported which is a bug. The debug notes report only those DREs where the referred VarDecl has at least one other DeclRefExpr which is claimed (covered by a fixit). If there is an unsafe VarDecl that has exactly one DRE and the DRE isn't claimed then the debug note about missing fixit won't be emitted. That is because the debug note is emitted from within a loop over set of successfully matched FixableGadgets which by-definition is missing those DRE that are not matched at all. The new code simply iterates over all unsafe VarDecls and all of their unclaimed DREs.
1abbea3
to
bda722a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think I did something similar locally when I collected data of unclaimed DREs on those big adopter projects. So I guess the data that we used to find major missing patterns for fix-its is still valid.
So, on one hand it's good that the data are representative but on the other hand if you had discovered this before me you could have saved me some time by sharing the fix :) |
Debug notes for unclaimed DeclRefExpr should report any DRE of an unsafe variable that is not covered by a Fixable (i. e. fixit for the particular AST pattern isn't implemented for whatever reason). Currently not all unclaimed DeclRefExpr-s are reported which is a bug. The debug notes report only those DREs where the referred VarDecl has at least one other DeclRefExpr which is claimed (covered by a fixit). If there is an unsafe VarDecl that has exactly one DRE and the DRE isn't claimed then the debug note about missing fixit won't be emitted. That is because the debug note is emitted from within a loop over set of successfully matched FixableGadgets which by-definition is missing those DRE that are not matched at all. The new code simply iterates over all unsafe VarDecls and all of their unclaimed DREs. (cherry picked from commit 2f49058)
…e/fix-debug-notes [-Wunsafe-buffer-usage] Fix debug notes for unclaimed DREs (llvm#80787)
Debug notes for unclaimed DeclRefExpr should report any DRE of an unsafe variable that is not covered by a Fixable (i. e. fixit for the particular AST pattern isn't implemented for whatever reason). Currently not all unclaimed DeclRefExpr-s are reported which is a bug. The debug notes report only those DREs where the referred VarDecl has at least one other DeclRefExpr which is claimed (covered by a fixit). If there is an unsafe VarDecl that has exactly one DRE and the DRE isn't claimed then the debug note about missing fixit won't be emitted. That is because the debug note is emitted from within a loop over set of successfully matched FixableGadgets which by-definition is missing those DRE that are not matched at all.
The new code simply iterates over all unsafe VarDecls and all of their unclaimed DREs.