[clang] 692da62 - [-Wunsafe-buffer-usage] Filter out conflicting fix-its
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 7 16:15:47 PST 2023
Author: Ziqing Luo
Date: 2023-02-07T16:15:28-08:00
New Revision: 692da6245d719fcee9d55936a021d9f9e301c557
URL: https://github.com/llvm/llvm-project/commit/692da6245d719fcee9d55936a021d9f9e301c557
DIFF: https://github.com/llvm/llvm-project/commit/692da6245d719fcee9d55936a021d9f9e301c557.diff
LOG: [-Wunsafe-buffer-usage] Filter out conflicting fix-its
Two fix-its conflict if they have overlapping source ranges. We shall
not emit conflicting fix-its. This patch checks conflicts in fix-its
generated for one variable (including variable declaration fix-its and
variable usage fix-its). If there is any, we do NOT emit any fix-it
for that variable.
Reviewed by: NoQ
Differential revision: https://reviews.llvm.org/D141338
Added:
clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
Modified:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/unittests/Analysis/CMakeLists.txt
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 04c9fdea444f8..3ac4def0429bf 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -52,6 +52,12 @@ class UnsafeBufferUsageHandler {
// through the handler class.
void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+namespace internal {
+// Tests if any two `FixItHint`s in `FixIts` conflict. Two `FixItHint`s
+// conflict if they have overlapping source ranges.
+bool anyConflict(const llvm::SmallVectorImpl<FixItHint> &FixIts,
+ const SourceManager &SM);
+} // namespace internal
} // end namespace clang
#endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c4f0a13a5d51e..af648235beef0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -694,6 +694,37 @@ groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
return FixablesForUnsafeVars;
}
+bool clang::internal::anyConflict(
+ const SmallVectorImpl<FixItHint> &FixIts, const SourceManager &SM) {
+ // A simple interval overlap detection algorithm. Sorts all ranges by their
+ // begin location then finds the first overlap in one pass.
+ std::vector<const FixItHint *> All; // a copy of `FixIts`
+
+ for (const FixItHint &H : FixIts)
+ All.push_back(&H);
+ std::sort(All.begin(), All.end(),
+ [&SM](const FixItHint *H1, const FixItHint *H2) {
+ return SM.isBeforeInTranslationUnit(H1->RemoveRange.getBegin(),
+ H2->RemoveRange.getBegin());
+ });
+
+ const FixItHint *CurrHint = nullptr;
+
+ for (const FixItHint *Hint : All) {
+ if (!CurrHint ||
+ SM.isBeforeInTranslationUnit(CurrHint->RemoveRange.getEnd(),
+ Hint->RemoveRange.getBegin())) {
+ // Either to initialize `CurrHint` or `CurrHint` does not
+ // overlap with `Hint`:
+ CurrHint = Hint;
+ } else
+ // In case `Hint` overlaps the `CurrHint`, we found at least one
+ // conflict:
+ return true;
+ }
+ return false;
+}
+
std::optional<FixItList>
ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
@@ -948,8 +979,12 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
else
FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
FixItsForVD.begin(), FixItsForVD.end());
- // Fix-it shall not overlap with macros or/and templates:
- if (overlapWithMacro(FixItsForVariable[VD])) {
+ // We conservatively discard fix-its of a variable if
+ // a fix-it overlaps with macros; or
+ // a fix-it conflicts with another one
+ if (overlapWithMacro(FixItsForVariable[VD]) ||
+ clang::internal::anyConflict(FixItsForVariable[VD],
+ Ctx.getSourceManager())) {
FixItsForVariable.erase(VD);
}
}
diff --git a/clang/unittests/Analysis/CMakeLists.txt b/clang/unittests/Analysis/CMakeLists.txt
index 619f2fc8b8581..d418a2b0f9c34 100644
--- a/clang/unittests/Analysis/CMakeLists.txt
+++ b/clang/unittests/Analysis/CMakeLists.txt
@@ -9,6 +9,7 @@ add_clang_unittest(ClangAnalysisTests
CloneDetectionTest.cpp
ExprMutationAnalyzerTest.cpp
MacroExpansionContextTest.cpp
+ UnsafeBufferUsageTest.cpp
)
clang_target_link_libraries(ClangAnalysisTests
diff --git a/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp b/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
new file mode 100644
index 0000000000000..66af5750ef242
--- /dev/null
+++ b/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
@@ -0,0 +1,60 @@
+#include "gtest/gtest.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
+
+using namespace clang;
+
+namespace {
+// The test fixture.
+class UnsafeBufferUsageTest : public ::testing::Test {
+protected:
+ UnsafeBufferUsageTest()
+ : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+ Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+ SourceMgr(Diags, FileMgr) {}
+
+ FileSystemOptions FileMgrOpts;
+ FileManager FileMgr;
+ IntrusiveRefCntPtr<DiagnosticIDs> DiagID;
+ DiagnosticsEngine Diags;
+ SourceManager SourceMgr;
+};
+} // namespace
+
+TEST_F(UnsafeBufferUsageTest, FixItHintsConflict) {
+ const FileEntry *DummyFile = FileMgr.getVirtualFile("<virtual>", 100, 0);
+ FileID DummyFileID = SourceMgr.getOrCreateFileID(DummyFile, SrcMgr::C_User);
+ SourceLocation StartLoc = SourceMgr.getLocForStartOfFile(DummyFileID);
+
+#define MkDummyHint(Begin, End) \
+ FixItHint::CreateReplacement(SourceRange(StartLoc.getLocWithOffset((Begin)), \
+ StartLoc.getLocWithOffset((End))), \
+ "dummy")
+
+ FixItHint H1 = MkDummyHint(0, 5);
+ FixItHint H2 = MkDummyHint(6, 10);
+ FixItHint H3 = MkDummyHint(20, 25);
+ llvm::SmallVector<FixItHint> Fixes;
+
+ // Test non-overlapping fix-its:
+ Fixes = {H1, H2, H3};
+ EXPECT_FALSE(internal::anyConflict(Fixes, SourceMgr));
+ Fixes = {H3, H2, H1}; // re-order
+ EXPECT_FALSE(internal::anyConflict(Fixes, SourceMgr));
+
+ // Test overlapping fix-its:
+ Fixes = {H1, H2, H3, MkDummyHint(0, 4) /* included in H1 */};
+ EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+ Fixes = {H1, H2, H3, MkDummyHint(10, 15) /* overlaps H2 */};
+ EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+ Fixes = {H1, H2, H3, MkDummyHint(7, 23) /* overlaps H2, H3 */};
+ EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+ Fixes = {H1, H2, H3, MkDummyHint(2, 23) /* overlaps H1, H2, and H3 */};
+ EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+}
\ No newline at end of file
More information about the cfe-commits
mailing list