diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -52,6 +52,12 @@ // 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 &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 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -694,6 +694,37 @@ return FixablesForUnsafeVars; } +bool clang::internal::anyConflict( + const SmallVectorImpl &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 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 ULCArraySubscriptGadget::getFixits(const Strategy &S) const { if (const auto *DRE = dyn_cast(Node->getBase()->IgnoreImpCasts())) @@ -948,8 +979,12 @@ 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 --- a/clang/unittests/Analysis/CMakeLists.txt +++ b/clang/unittests/Analysis/CMakeLists.txt @@ -9,6 +9,7 @@ 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 --- /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 DiagID; + DiagnosticsEngine Diags; + SourceManager SourceMgr; +}; +} // namespace + +TEST_F(UnsafeBufferUsageTest, FixItHintsConflict) { + const FileEntry *DummyFile = FileMgr.getVirtualFile("", 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 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