llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-ssaf Author: Ziqing Luo (ziqingluo-90) <details> <summary>Changes</summary> PR #<!-- -->210457 introduces declaration rewriting. When CppBoundedBuffers rewrites a declaration or return type to a bounded_ptr/bounded_array, some of the existing uses of that entity needs retrofit. This commit creates expression rewrites for the following patterns: - Append `.data()` to call arguments when necessary parameter is not transformed but argument is transformed. - Append `.as_bounded<T>()` to call arguments when both parameter and argument are transformed but element types are not identical. - Rewrite `&e[I]` to `(e + i)` and `&*e`/`&(*e)` to `e`, if `e` is transformed. - Rewrite `(T*)e`, `static_cast<T*>(e)`, and `reinterpret_cast<T*>(e)` to `e.as_bounded<T>()`, if `e` is transformed. The 3rd step of rdar://187125348 --- Patch is 30.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/223105.diff 4 Files Affected: - (modified) clang/lib/ScalableStaticAnalysis/SourceTransformation/CMakeLists.txt (+9) - (modified) clang/lib/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.cpp (+417-7) - (modified) clang/unittests/ScalableStaticAnalysis/SourceTransformation/CppBoundedBuffersTest.cpp (+246) - (modified) llvm/utils/gn/secondary/clang/lib/ScalableStaticAnalysis/SourceTransformation/BUILD.gn (+7) ``````````diff diff --git a/clang/lib/ScalableStaticAnalysis/SourceTransformation/CMakeLists.txt b/clang/lib/ScalableStaticAnalysis/SourceTransformation/CMakeLists.txt index 96e85ec3a50a8..951a40e874398 100644 --- a/clang/lib/ScalableStaticAnalysis/SourceTransformation/CMakeLists.txt +++ b/clang/lib/ScalableStaticAnalysis/SourceTransformation/CMakeLists.txt @@ -2,6 +2,13 @@ set(LLVM_LINK_COMPONENTS Support ) +# TODO: move SSAFAnalysesCommon.h to a higher-up folder shared by both +# libraries instead of reaching into a sibling library's private header. +# +# Needed to reach clang/lib/ScalableStaticAnalysis/Analyses/SSAFAnalysesCommon.h, +# a private header shared by the two libraries. +include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../Analyses) + add_clang_library(clangScalableStaticAnalysisSourceTransformation SARIFTransformationReportFormat.cpp TransformationRegistry.cpp @@ -12,6 +19,8 @@ add_clang_library(clangScalableStaticAnalysisSourceTransformation clangAST clangBasic clangLex + clangScalableStaticAnalysisAnalyses clangScalableStaticAnalysisCore clangToolingCore + clangToolingRefactoring ) diff --git a/clang/lib/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.cpp b/clang/lib/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.cpp index c119ee2f01c5e..01c27c4dd7637 100644 --- a/clang/lib/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.cpp +++ b/clang/lib/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.cpp @@ -7,11 +7,14 @@ //===----------------------------------------------------------------------===// #include "clang/ScalableStaticAnalysis/SourceTransformation/Transformations/CppBoundedBuffers.h" +#include "../../Analyses/SSAFAnalysesCommon.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DynamicRecursiveASTVisitor.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h" #include "clang/Basic/LangOptions.h" @@ -20,6 +23,7 @@ #include "clang/Frontend/SSAFOptions.h" #include "clang/Lex/Lexer.h" #include "clang/ScalableStaticAnalysis/Analyses/EntityPointerLevel/EntityPointerLevel.h" +#include "clang/ScalableStaticAnalysis/Analyses/PointerFlow/PointerFlowPairs.h" #include "clang/ScalableStaticAnalysis/Analyses/UnsafeBufferUsage/UnsafeBufferUsageAnalysis.h" #include "clang/ScalableStaticAnalysis/Core/ASTEntityMapping.h" #include "clang/ScalableStaticAnalysis/Core/Model/EntityId.h" @@ -27,12 +31,18 @@ #include "clang/ScalableStaticAnalysis/Core/Model/EntityName.h" #include "clang/ScalableStaticAnalysis/SourceTransformation/TransformationRegistry.h" #include "clang/Tooling/Core/Replacement.h" +#include "clang/Tooling/Refactoring/AtomicChange.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/Twine.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/ErrorHandling.h" #include <cassert> #include <map> #include <optional> #include <string> +#include <vector> using namespace clang; using namespace clang::ssaf; @@ -290,6 +300,11 @@ class CollectVisitor : public DynamicRecursiveASTVisitor { /// Rewrites or reports every collected declarator and function return. class RewriteVisitor : public DynamicRecursiveASTVisitor { public: + // Decls and their ClassifyResults for all that are successfully + // transformed by `emit`: + llvm::DenseMap<const Decl *, ClassifyResult> TransformedDecls; + llvm::DenseMap<const FunctionDecl *, ClassifyResult> TransformedReturns; + RewriteVisitor(ASTContext &Ctx, DeclLevels &Decls, ReturnLevels &Returns, SourceEditEmitter &Edits, TransformationReportEmitter &Report) : Ctx(Ctx), Decls(Decls), Returns(Returns), Edits(Edits), Report(Report) { @@ -324,8 +339,12 @@ class RewriteVisitor : public DynamicRecursiveASTVisitor { if (!FunTypeLoc) return report(FD, ReportReason::EmissionFailed); - return report(FD, emit(FD->getBeginLoc(), NameLoc, - FunTypeLoc.getReturnLoc(), FD->getReturnType(), R)); + + auto Reason = emit(FD->getBeginLoc(), NameLoc, FunTypeLoc.getReturnLoc(), + FD->getReturnType(), R); + if (!Reason) + TransformedReturns[FD] = R; + return report(FD, Reason); } private: @@ -347,7 +366,12 @@ class RewriteVisitor : public DynamicRecursiveASTVisitor { if (R.Skip) return (void)report(D, *R.Skip); - report(D, emit(D->getBeginLoc(), NameLoc, TSI->getTypeLoc(), T, R)); + + auto Reason = emit(D->getBeginLoc(), NameLoc, TSI->getTypeLoc(), T, R); + + if (!Reason) + TransformedDecls[D] = R; + report(D, Reason); } /// Compute the precise source range for rewriting. The produced range is @@ -482,6 +506,108 @@ class RewriteVisitor : public DynamicRecursiveASTVisitor { TransformationReportEmitter &Report; }; +// FIXME: adding report for any unsuccessful edits +// FIXME: we need clusters to group edits atomically + +/// Traverses the whole TU and create edits for expressions in order to adapt to +/// transformed Decls. +class ExpressionRewriter { +public: + ExpressionRewriter( + ASTContext &Ctx, const SSAFOptions &Opts, SourceEditEmitter &Edits, + const llvm::DenseMap<const Decl *, ClassifyResult> &TransformedDecls, + const llvm::DenseMap<const FunctionDecl *, ClassifyResult> + &TransformedReturns) + : Ctx(Ctx), Opts(Opts), Edits(Edits), TransformedDecls(TransformedDecls), + TransformedReturns(TransformedReturns) {} + + /// Traverses the whole \c TU and create edits expressions in order to adapt + /// to transformed Decls. + void rewriteExprInTU(const TranslationUnitDecl *TU); + + /// Create edits in \c AC to update expression \c E, if it is + /// \c isPtrExprBaseTransformed, so that \c E has proper bounded type after + /// transformation. + /// + /// Note that this step is self-contained. It does not rely on the context of + /// where \c E is. If \c E's base is transformed, it needs retrofit. + /// + /// \return true iff \c E needs edits. + bool rewriteExpression(const Expr *E, tooling::AtomicChange &AC) const; + + /// Create edits for a \c PointerFlowPair that + /// 1) \c rewriteExpression for involved expressions; and + /// 2) further update expressions in order to adapt the flow to the + /// transformation. + std::optional<tooling::AtomicChange> + adaptPointerFlow(const PointerFlowPair &Pair) const; + +private: + /// Associate edit operations to \c CharSourceRange always, since they carry + /// token/char range info while single SourceLocation doesn't. This way, the + /// edit kind (replacement or insertion) cannot be inferred from the input + /// (i.e., SourceRange vs. SourceLocation). Use this enum to explicitly + /// express the kinds. + enum EditKind { Replace, InsertAtBegin, InsertAtEnd }; + + /// \param Range The source locations associated with the edit. For any \c + /// EditKind, source locations are given by a \c CharSourceRange, which + /// carries the information of whether it is a token range or a char range. + /// \param NewText The text of the edit that will replace a source range or + /// be inserted at a location. + /// \param EditKind The kind of edit: replacement or insertion. + /// \param AC IN/OUT parameter. The new edit will be added to \c AC + /// \return true if edit is successfully added to \c AC + bool addEditToAtomicChange(CharSourceRange Range, StringRef NewText, + EditKind EditKind, + tooling::AtomicChange &AC) const; + + /// \return true iff the base(s) of the pointer/array expression \c E are all + /// transformed to have bounded types. + bool isExprBaseTransformed(const Expr *E) const; + + /// \return a non-null pointer to a \c ClassifyResult, if `D` is transformed + /// to have bounded types. + const ClassifyResult *getDeclClassifyResultsIfTransformed(const Decl *D, + bool IsRet) const; + + /// \return a non-empty vector of \c ClassifyResult, the base(s) of the + /// pointer/array expression \c E are all transformed to have bounded + /// types. + std::vector<const ClassifyResult *> + getPtrExprClassifyResultsIfTransformed(const Expr *E) const; + + //==---------------- Expression rewrite rules -------------------==// + + /// - Append '.data()' to RHS, if LHS is a parameter and NOT transformed, and + /// RHS is transformed; or + /// - Append '.as_bounded<T>()' to RHS, if LHS is a parameter and both sides + /// are transformed but element types are not identical. + /// \return true iff the pattern is matched and edits were attempted to be + /// added into \c AC. + bool adaptCallArgumentInFlow(const PointerFlowPair &Pair, + tooling::AtomicChange &AC) const; + + /// Edit '&e[i]' to '(e + i)' or '&*e' to 'ptr', if 'e' is transformed. + /// \return true iff the pattern is matched and edits were attempted to be + /// added into \c AC. + bool retrofitAddrofElementAccess(const Expr *E, + tooling::AtomicChange &AC) const; + + /// Edit '(T*)e/static_cast<T>(e)/reinterpret_cast<T>(e)' to + /// 'e.as_bounded<T>()' , if 'e' is transformed. + /// \return true iff the pattern is matched and edits were attempted to be + /// added into \c AC. + bool retrofitPointerCast(const Expr *E, tooling::AtomicChange &AC) const; + + ASTContext &Ctx; + const SSAFOptions &Opts; + SourceEditEmitter &Edits; + const llvm::DenseMap<const Decl *, ClassifyResult> &TransformedDecls; + const llvm::DenseMap<const FunctionDecl *, ClassifyResult> + &TransformedReturns; +}; + } // namespace namespace clang::ssaf { @@ -607,14 +733,298 @@ void CppBoundedBuffers::HandleTranslationUnit(ASTContext &Ctx) { DeclLevels Decls; ReturnLevels Returns; - Decl *TU = Ctx.getTranslationUnitDecl(); - CollectVisitor(Reach, TUNamespace, LUNamespace, Decls, Returns) - .TraverseDecl(TU); - RewriteVisitor(Ctx, Decls, Returns, Edits, Report).TraverseDecl(TU); + auto *TU = Ctx.getTranslationUnitDecl(); + CollectVisitor(Reach, Decls, Returns).TraverseDecl(TU); + auto RV = RewriteVisitor(Ctx, Decls, Returns, Edits, Report); + + RV.TraverseDecl(TU); + + ExpressionRewriter ExprRewriter(Ctx, Opts, Edits, RV.TransformedDecls, + RV.TransformedReturns); + + ExprRewriter.rewriteExprInTU(TU); } } // namespace clang::ssaf +namespace { + +//===------------ ExpressionRewriter implementation --------------===// + +std::optional<tooling::AtomicChange> +ExpressionRewriter::adaptPointerFlow(const PointerFlowPair &Pair) const { + tooling::AtomicChange AC("", ""); + + rewriteExpression(Pair.RHS, AC); + adaptCallArgumentInFlow(Pair, AC); // FIXME: more flow patterns + return AC; +} + +bool ExpressionRewriter::rewriteExpression(const Expr *E, + tooling::AtomicChange &AC) const { + if (isExprBaseTransformed(E)) + return retrofitAddrofElementAccess(E, AC) || retrofitPointerCast(E, AC); + return false; +} + +bool ExpressionRewriter::adaptCallArgumentInFlow( + const PointerFlowPair &Pair, tooling::AtomicChange &AC) const { + const auto *PVD = + dyn_cast_or_null<ParmVarDecl>(Pair.LHS.dyn_cast<const ValueDecl *>()); + + if (!PVD) + return false; + + // '(RHS).data()' is a prvalue that can't bind to a reference. So bail. + if (PVD->getType()->isReferenceType()) + return false; + + QualType RTypeBeforeImpCast = Pair.RHS->IgnoreImpCasts()->getType(); + QualType LType = PVD->getType().getNonReferenceType(); + + // If RHS has `void*` type, it will have `char*` after transformation and + // being appened '.data()'. This type change may cause the callee to be + // silently swapped to a different overload. So bail. + if (RTypeBeforeImpCast->isVoidPointerType() && LType->isVoidPointerType()) + return false; + + auto It = TransformedDecls.find(PVD); + bool IsLHSTransformed = It != TransformedDecls.end(); + bool IsRHSTransformed = isExprBaseTransformed(Pair.RHS); + + CharSourceRange RHSRange = Lexer::getAsCharRange( + Pair.RHS->getSourceRange(), Ctx.getSourceManager(), Ctx.getLangOpts()); + + if (!IsLHSTransformed && IsRHSTransformed) { + addEditToAtomicChange(RHSRange, "(", EditKind::InsertAtBegin, AC); + addEditToAtomicChange(RHSRange, ").data()", EditKind::InsertAtEnd, AC); + return true; + } + if (IsLHSTransformed && IsRHSTransformed) { + QualType RPteTy = RTypeBeforeImpCast->getPointeeType(); + QualType LPteTy = LType->getPointeeType(); + + if (LPteTy.isNull() || RPteTy.isNull()) + return false; + + if (Ctx.hasSameType(LPteTy, RPteTy)) + return false; + + StringRef InnerSpelling = It->getSecond().InnerSpelling; + + addEditToAtomicChange(RHSRange, "(", EditKind::InsertAtBegin, AC); + addEditToAtomicChange( + RHSRange, + (").as_bounded<" + InnerSpelling + ">()").getSingleStringRef(), + EditKind::InsertAtEnd, AC); + return true; + } + return false; +} + +bool ExpressionRewriter::retrofitAddrofElementAccess( + const Expr *E, tooling::AtomicChange &AC) const { + const auto *UO = dyn_cast<UnaryOperator>(E->IgnoreParenImpCasts()); + if (!UO || UO->getOpcode() != UO_AddrOf) + return false; + + const Expr *SubExpr = UO->getSubExpr()->IgnoreParenImpCasts(); + const Expr *Ptr, *Offset = nullptr; + + if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(SubExpr)) { + Ptr = ASE->getBase(); + Offset = ASE->getIdx(); + } else if (const auto *Deref = dyn_cast<UnaryOperator>(SubExpr); + Deref && Deref->getOpcode() == UO_Deref) { + Ptr = Deref->getSubExpr(); + } else + return false; + + if (!isExprBaseTransformed(Ptr)) + return false; + + rewriteExpression(Ptr, AC); + // Ptr may have been recursively edited, so its source range should stay + // intact. + + const SourceManager &SM = Ctx.getSourceManager(); + const LangOptions &LO = Ctx.getLangOpts(); + auto PtrCR = Lexer::getAsCharRange(Ptr->getSourceRange(), SM, LO); + auto FullExprCR = Lexer::getAsCharRange(UO->getSourceRange(), SM, LO); + // Source range before `Ptr`: + auto PrePtrCR = + CharSourceRange::getCharRange(FullExprCR.getBegin(), PtrCR.getBegin()); + + if (!Offset) { + // Source range after `Ptr`: + auto PostPtrCR = + CharSourceRange::getCharRange(PtrCR.getEnd(), FullExprCR.getEnd()); + // For '&*ptr' or '&(*ptr)', drop contents in PrePtrCR and PostPtrCR: + addEditToAtomicChange(PrePtrCR, "", EditKind::Replace, AC); + addEditToAtomicChange(PostPtrCR, "", EditKind::Replace, AC); + return true; + } + + auto OffsetCR = Lexer::getAsCharRange(Offset->getSourceRange(), SM, LO); + auto PostPtrPreOffsetCR = + CharSourceRange::getCharRange(PtrCR.getEnd(), OffsetCR.getBegin()); + auto PostOffsetCR = + CharSourceRange::getCharRange(OffsetCR.getEnd(), FullExprCR.getEnd()); + + // For '&ptr[offset]' or '&(ptr[offset])', + // 1. replace contents in PrePtrCR with "(", and + // 2. replace contents in PostPtrPreOffsetCR with " + ", and + // 3. replace contents in postOffsetCR with ") ", + // results in '(ptr + offset)': + addEditToAtomicChange(PrePtrCR, "(", EditKind::Replace, AC); + addEditToAtomicChange(PostPtrPreOffsetCR, " + ", EditKind::Replace, AC); + addEditToAtomicChange(PostOffsetCR, ")", EditKind::Replace, AC); + return true; +} + +bool ExpressionRewriter::retrofitPointerCast(const Expr *E, + tooling::AtomicChange &AC) const { + const auto *CE = dyn_cast<ExplicitCastExpr>(E->IgnoreParenImpCasts()); + + if (!CE || + !isa<CStyleCastExpr, CXXStaticCastExpr, CXXReinterpretCastExpr>(CE)) + return false; + if (!isExprBaseTransformed(CE)) + return false; + + QualType DestTy = CE->getTypeAsWritten(); + QualType DestPteTy = DestTy->getPointeeType(); + + if (!DestTy->isPointerType()) + return false; + + const Expr *Ptr = CE->getSubExpr(); + rewriteExpression(Ptr, AC); + // Ptr may have been recursively edited, so its source range should stay + // intact. + + const SourceManager &SM = Ctx.getSourceManager(); + const LangOptions &LO = Ctx.getLangOpts(); + CharSourceRange PtrCR = + Lexer::getAsCharRange(CE->getSubExpr()->getSourceRange(), SM, LO); + CharSourceRange FullCastExprCR = + Lexer::getAsCharRange(CE->getSourceRange(), SM, LO); + CharSourceRange PrePtrCR = CharSourceRange::getCharRange( + FullCastExprCR.getBegin(), PtrCR.getBegin()); + CharSourceRange PostPtrCR = + CharSourceRange::getCharRange(PtrCR.getEnd(), FullCastExprCR.getEnd()); + std::string T = spell(DestPteTy, Ctx); + + // For `(T*)ptr` or *_cast<T>(ptr), + // 1. replace contents in PrePtrCR with "(", and + // 2. replace contents in PostPtrCR with ").as_bounded<T>()", + // results in '(ptr).as_bounded<T>()'. + addEditToAtomicChange(PrePtrCR, "(", EditKind::Replace, AC); + addEditToAtomicChange(PostPtrCR, ").as_bounded<" + T + ">()", + EditKind::Replace, AC); + return true; +} + +void ExpressionRewriter::rewriteExprInTU(const TranslationUnitDecl *TU) { + llvm::DenseMap<const NamedDecl *, std::vector<const NamedDecl *>> + ContributorGroups; + + findContributors(Ctx, Opts, ContributorGroups, + /*ExtractFromSystemHeaders=*/false); + + llvm::SmallVector<PointerFlowPair> Pairs; + PointerFlowPairMatcher Matcher{Ctx}; + + for (auto &[GrpCano, ContriGrp] : ContributorGroups) + for (auto *ContriDecl : ContriGrp) { + auto PairsCollector = [&Pairs, &Matcher, + &ContriDecl](const DynTypedNode &Node) { + Matcher.matches(Node, ContriDecl, Pairs); + }; + findMatchesIn(ContriDecl, PairsCollector); + } + + for (const PointerFlowPair &Pair : Pairs) { + if (auto AC = adaptPointerFlow(Pair); + AC && llvm::all_of(AC->getReplacements(), + std::mem_fn(&tooling::Replacement::isApplicable))) + for (const tooling::Replacement &R : AC->getReplacements()) + Edits.addReplacement(R); + } +} + +bool ExpressionRewriter::isExprBaseTransformed(const Expr *E) const { + return !getPtrExprClassifyResultsIfTransformed(E).empty(); +} + +const ClassifyResult * +ExpressionRewriter::getDeclClassifyResultsIfTransformed(const Decl *D, + bool IsRet) const { + auto Lookup = [](const auto &Map, const auto *Key) { + auto It = Map.find(Key); + return It == Map.end() ? nullptr : &It->second; + }; + return IsRet ? Lookup(TransformedReturns, cast<FunctionDecl>(D)) + : Lookup(TransformedDecls, D); +} + +std::vector<const ClassifyResult *> +ExpressionRewriter::getPtrExprClassifyResultsIfTransformed( + const Expr *E) const { + auto DPLs = translateDeclPointerLevel(E, Ctx); + + if (!DPLs) { + // Errors indicate no transformation for E. No further action. + llvm::consumeError(DPLs.takeError()); + return {}; + } + + std::vector<const ClassifyResult *> Result; + + for (auto &DPL : *DPLs) { + const auto *ClassifyResult = + getDeclClassifyResultsIfTransformed(DPL.Decl, DPL.IsReturn); + + if (!ClassifyResult) + return {}; + Result.push_back(ClassifyResult); + } + return Result; +} + +bool ExpressionRewriter::addEditToAtomicChange( + CharSourceRange Range, StringRef NewText, EditKind EditKind, + tooling::AtomicChange &AC) const { + assert(Range.isCharRange()); + + if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID()) + // FIXME: report... + return false; + + const SourceManager &SM = Ctx.getSourceManager(); + llvm::Error Err = [&]() -> llvm::Error { + switch (EditKind) { + case Replace: + return AC.replace(SM, Range, NewText); + case InsertAtBegin: + return AC.insert(SM, Range.getBegin(), NewText, /*InsertAfter=*/false); + case InsertAtEnd: + return AC.insert(SM, Range.getEnd(), NewText, /*InsertAfter=*/true); + } + llvm_unreachable("unhandled EditKind"); + }(); + + if (Err) { + llvm::consumeError(std::move(Err)); + // FIXME: generate a Report + // If AtomicChange has an error,the whole should be... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/223105 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
