Revert "[FlowSensitive] [StatusOr] [2/N] Add minimal model" #164040
Merged
+0 −3,122
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This reverts commit fe02993.
llvmbot added clang 
| @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Florian Mayer (fmayer) ChangesReverts llvm/llvm-project#162932 Patch is 88.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/164040.diff 9 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h deleted file mode 100644 index cb619fb0cb5bb..0000000000000 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h +++ /dev/null @@ -1,112 +0,0 @@ -//===- UncheckedStatusOrAccessModel.h -------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H -#define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H - -#include "clang/AST/Type.h" -#include "clang/ASTMatchers/ASTMatchers.h" -#include "clang/Analysis/CFG.h" -#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" -#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" -#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" -#include "clang/Analysis/FlowSensitive/MatchSwitch.h" -#include "clang/Analysis/FlowSensitive/NoopLattice.h" -#include "clang/Analysis/FlowSensitive/StorageLocation.h" -#include "clang/Analysis/FlowSensitive/Value.h" -#include "clang/Basic/SourceLocation.h" -#include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringMap.h" -#include "llvm/ADT/StringRef.h" - -namespace clang::dataflow::statusor_model { - -// The helper functions exported here are for use of downstream vendor -// extensions of this model. - -// Match declaration of `absl::StatusOr<T>` and bind `T` to "T". -clang::ast_matchers::DeclarationMatcher statusOrClass(); -// Match declaration of `absl::Status`. -clang::ast_matchers::DeclarationMatcher statusClass(); -// Match declaration of `absl::internal_statusor::OperatorBase`. -clang::ast_matchers::DeclarationMatcher statusOrOperatorBaseClass(); -clang::ast_matchers::TypeMatcher statusOrType(); - -// Get RecordStorageLocation for the `Status` contained in the `StatusOr` -RecordStorageLocation &locForStatus(RecordStorageLocation &StatusOrLoc); -// Get the StorageLocation for the OK boolean in the `Status` -StorageLocation &locForOk(RecordStorageLocation &StatusLoc); -// Get the OK boolean in the `Status`, and initialize it if necessary. -BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env); -// Get synthetic fields for the types modelled by -// `UncheckedStatusOrAccessModel`. -llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType, - const CXXRecordDecl &RD); - -// Initialize the synthetic fields of the `StatusOr`. -// N.B. if it is already initialized, the value gets reset. -BoolValue &initializeStatusOr(RecordStorageLocation &StatusOrLoc, - Environment &Env); -// Initialize the synthetic fields of the `Status`. -// N.B. if it is already initialized, the value gets reset. -BoolValue &initializeStatus(RecordStorageLocation &StatusLoc, Environment &Env); - -bool isRecordTypeWithName(QualType Type, llvm::StringRef TypeName); -// Return true if `Type` is instantiation of `absl::StatusOr<T>` -bool isStatusOrType(QualType Type); -// Return true if `Type` is `absl::Status` -bool isStatusType(QualType Type); - -// Get `QualType` for `absl::Status`, or default-constructed -// QualType if it does not exist. -QualType findStatusType(const ASTContext &Ctx); - -struct UncheckedStatusOrAccessModelOptions {}; - -// Dataflow analysis that discovers unsafe uses of StatusOr values. -class UncheckedStatusOrAccessModel - : public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> { -public: - explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env); - - static Lattice initialElement() { return {}; } - - void transfer(const CFGElement &Elt, Lattice &L, Environment &Env); - -private: - CFGMatchSwitch<TransferState<Lattice>> TransferMatchSwitch; -}; - -using LatticeTransferState = - TransferState<UncheckedStatusOrAccessModel::Lattice>; - -// Extend the Builder with the transfer functions for -// `UncheckedStatusOrAccessModel`. This is useful to write downstream models -// that extend the model. -CFGMatchSwitch<LatticeTransferState> -buildTransferMatchSwitch(ASTContext &Ctx, - CFGMatchSwitchBuilder<LatticeTransferState> Builder); - -class UncheckedStatusOrAccessDiagnoser { -public: - explicit UncheckedStatusOrAccessDiagnoser( - UncheckedStatusOrAccessModelOptions Options = {}); - - llvm::SmallVector<SourceLocation> operator()( - const CFGElement &Elt, ASTContext &Ctx, - const TransferStateForDiagnostics<UncheckedStatusOrAccessModel::Lattice> - &State); - -private: - CFGMatchSwitch<const Environment, llvm::SmallVector<SourceLocation>> - DiagnoseMatchSwitch; -}; - -} // namespace clang::dataflow::statusor_model - -#endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H diff --git a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt index d1236f5714881..89bbe8791eb2c 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt +++ b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt @@ -1,7 +1,6 @@ add_clang_library(clangAnalysisFlowSensitiveModels ChromiumCheckModel.cpp UncheckedOptionalAccessModel.cpp - UncheckedStatusOrAccessModel.cpp LINK_LIBS clangAnalysis diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp deleted file mode 100644 index 75b0959491c22..0000000000000 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ /dev/null @@ -1,295 +0,0 @@ -//===- UncheckedStatusOrAccessModel.cpp -----------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include "clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h" - -#include <cassert> -#include <utility> - -#include "clang/AST/DeclCXX.h" -#include "clang/AST/DeclTemplate.h" -#include "clang/AST/Expr.h" -#include "clang/AST/ExprCXX.h" -#include "clang/AST/TypeBase.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/ASTMatchers/ASTMatchers.h" -#include "clang/ASTMatchers/ASTMatchersInternal.h" -#include "clang/Analysis/CFG.h" -#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" -#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" -#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" -#include "clang/Analysis/FlowSensitive/MatchSwitch.h" -#include "clang/Analysis/FlowSensitive/StorageLocation.h" -#include "clang/Analysis/FlowSensitive/Value.h" -#include "clang/Basic/LLVM.h" -#include "clang/Basic/SourceLocation.h" -#include "llvm/ADT/StringMap.h" - -namespace clang::dataflow::statusor_model { -namespace { - -using ::clang::ast_matchers::MatchFinder; -using ::clang::ast_matchers::StatementMatcher; - -} // namespace - -static bool namespaceEquals(const NamespaceDecl *NS, - clang::ArrayRef<clang::StringRef> NamespaceNames) { - while (!NamespaceNames.empty() && NS) { - if (NS->getName() != NamespaceNames.consume_back()) - return false; - NS = dyn_cast_or_null<NamespaceDecl>(NS->getParent()); - } - return NamespaceNames.empty() && !NS; -} - -// TODO: move this to a proper place to share with the rest of clang -static bool isTypeNamed(QualType Type, clang::ArrayRef<clang::StringRef> NS, - StringRef Name) { - if (Type.isNull()) - return false; - if (auto *RD = Type->getAsRecordDecl()) - if (RD->getName() == Name) - if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD->getDeclContext())) - return namespaceEquals(N, NS); - return false; -} - -static bool isStatusOrOperatorBaseType(QualType Type) { - return isTypeNamed(Type, {"absl", "internal_statusor"}, "OperatorBase"); -} - -static bool isSafeUnwrap(RecordStorageLocation *StatusOrLoc, - const Environment &Env) { - if (!StatusOrLoc) - return false; - auto &StatusLoc = locForStatus(*StatusOrLoc); - auto *OkVal = Env.get<BoolValue>(locForOk(StatusLoc)); - return OkVal != nullptr && Env.proves(OkVal->formula()); -} - -static ClassTemplateSpecializationDecl * -getStatusOrBaseClass(const QualType &Ty) { - auto *RD = Ty->getAsCXXRecordDecl(); - if (RD == nullptr) - return nullptr; - if (isStatusOrType(Ty) || - // In case we are analyzing code under OperatorBase itself that uses - // operator* (e.g. to implement operator->). - isStatusOrOperatorBaseType(Ty)) - return cast<ClassTemplateSpecializationDecl>(RD); - if (!RD->hasDefinition()) - return nullptr; - for (const auto &Base : RD->bases()) - if (auto *QT = getStatusOrBaseClass(Base.getType())) - return QT; - return nullptr; -} - -static QualType getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) { - return TRD->getTemplateArgs().get(0).getAsType(); -} - -static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxMemberCallExpr( - on(expr(unless(cxxThisExpr()))), - callee(cxxMethodDecl( - hasName(member_name), - ofClass(anyOf(statusOrClass(), statusOrOperatorBaseClass()))))); -} - -static auto isStatusOrOperatorCallWithName(llvm::StringRef operator_name) { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxOperatorCallExpr( - hasOverloadedOperatorName(operator_name), - callee(cxxMethodDecl( - ofClass(anyOf(statusOrClass(), statusOrOperatorBaseClass()))))); -} - -static auto valueCall() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return anyOf(isStatusOrMemberCallWithName("value"), - isStatusOrMemberCallWithName("ValueOrDie")); -} - -static auto valueOperatorCall() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return expr(anyOf(isStatusOrOperatorCallWithName("*"), - isStatusOrOperatorCallWithName("->"))); -} - -static auto -buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) { - return CFGMatchSwitchBuilder<const Environment, - llvm::SmallVector<SourceLocation>>() - // StatusOr::value, StatusOr::ValueOrDie - .CaseOfCFGStmt<CXXMemberCallExpr>( - valueCall(), - [](const CXXMemberCallExpr *E, - const ast_matchers::MatchFinder::MatchResult &, - const Environment &Env) { - if (!isSafeUnwrap(getImplicitObjectLocation(*E, Env), Env)) - return llvm::SmallVector<SourceLocation>({E->getExprLoc()}); - return llvm::SmallVector<SourceLocation>(); - }) - - // StatusOr::operator*, StatusOr::operator-> - .CaseOfCFGStmt<CXXOperatorCallExpr>( - valueOperatorCall(), - [](const CXXOperatorCallExpr *E, - const ast_matchers::MatchFinder::MatchResult &, - const Environment &Env) { - RecordStorageLocation *StatusOrLoc = - Env.get<RecordStorageLocation>(*E->getArg(0)); - if (!isSafeUnwrap(StatusOrLoc, Env)) - return llvm::SmallVector<SourceLocation>({E->getOperatorLoc()}); - return llvm::SmallVector<SourceLocation>(); - }) - .Build(); -} - -UncheckedStatusOrAccessDiagnoser::UncheckedStatusOrAccessDiagnoser( - UncheckedStatusOrAccessModelOptions Options) - : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {} - -llvm::SmallVector<SourceLocation> UncheckedStatusOrAccessDiagnoser::operator()( - const CFGElement &Elt, ASTContext &Ctx, - const TransferStateForDiagnostics<UncheckedStatusOrAccessModel::Lattice> - &State) { - return DiagnoseMatchSwitch(Elt, Ctx, State.Env); -} - -BoolValue &initializeStatus(RecordStorageLocation &StatusLoc, - Environment &Env) { - auto &OkVal = Env.makeAtomicBoolValue(); - Env.setValue(locForOk(StatusLoc), OkVal); - return OkVal; -} - -BoolValue &initializeStatusOr(RecordStorageLocation &StatusOrLoc, - Environment &Env) { - return initializeStatus(locForStatus(StatusOrLoc), Env); -} - -clang::ast_matchers::DeclarationMatcher statusOrClass() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return classTemplateSpecializationDecl( - hasName("absl::StatusOr"), - hasTemplateArgument(0, refersToType(type().bind("T")))); -} - -clang::ast_matchers::DeclarationMatcher statusClass() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxRecordDecl(hasName("absl::Status")); -} - -clang::ast_matchers::DeclarationMatcher statusOrOperatorBaseClass() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return classTemplateSpecializationDecl( - hasName("absl::internal_statusor::OperatorBase")); -} - -clang::ast_matchers::TypeMatcher statusOrType() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return hasCanonicalType(qualType(hasDeclaration(statusOrClass()))); -} - -bool isRecordTypeWithName(QualType Type, llvm::StringRef TypeName) { - return Type->isRecordType() && - Type->getAsCXXRecordDecl()->getQualifiedNameAsString() == TypeName; -} - -bool isStatusOrType(QualType Type) { - return isTypeNamed(Type, {"absl"}, "StatusOr"); -} - -bool isStatusType(QualType Type) { - return isTypeNamed(Type, {"absl"}, "Status"); -} - -llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType, - const CXXRecordDecl &RD) { - if (auto *TRD = getStatusOrBaseClass(Ty)) - return {{"status", StatusType}, {"value", getStatusOrValueType(TRD)}}; - if (isStatusType(Ty) || (RD.hasDefinition() && - RD.isDerivedFrom(StatusType->getAsCXXRecordDecl()))) - return {{"ok", RD.getASTContext().BoolTy}}; - return {}; -} - -RecordStorageLocation &locForStatus(RecordStorageLocation &StatusOrLoc) { - return cast<RecordStorageLocation>(StatusOrLoc.getSyntheticField("status")); -} - -StorageLocation &locForOk(RecordStorageLocation &StatusLoc) { - return StatusLoc.getSyntheticField("ok"); -} - -BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env) { - if (auto *Val = Env.get<BoolValue>(locForOk(StatusLoc))) - return *Val; - return initializeStatus(StatusLoc, Env); -} - -static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr, - const MatchFinder::MatchResult &, - LatticeTransferState &State) { - RecordStorageLocation *StatusOrLoc = - getImplicitObjectLocation(*Expr, State.Env); - if (StatusOrLoc == nullptr) - return; - - auto &OkVal = valForOk(locForStatus(*StatusOrLoc), State.Env); - State.Env.setValue(*Expr, OkVal); -} - -CFGMatchSwitch<LatticeTransferState> -buildTransferMatchSwitch(ASTContext &Ctx, - CFGMatchSwitchBuilder<LatticeTransferState> Builder) { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return std::move(Builder) - .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"), - transferStatusOrOkCall) - .Build(); -} - -QualType findStatusType(const ASTContext &Ctx) { - for (Type *Ty : Ctx.getTypes()) - if (isStatusType(QualType(Ty, 0))) - return QualType(Ty, 0); - - return QualType(); -} - -UncheckedStatusOrAccessModel::UncheckedStatusOrAccessModel(ASTContext &Ctx, - Environment &Env) - : DataflowAnalysis<UncheckedStatusOrAccessModel, - UncheckedStatusOrAccessModel::Lattice>(Ctx), - TransferMatchSwitch(buildTransferMatchSwitch(Ctx, {})) { - QualType StatusType = findStatusType(Ctx); - Env.getDataflowAnalysisContext().setSyntheticFieldCallback( - [StatusType](QualType Ty) -> llvm::StringMap<QualType> { - CXXRecordDecl *RD = Ty->getAsCXXRecordDecl(); - if (RD == nullptr) - return {}; - - if (auto Fields = getSyntheticFields(Ty, StatusType, *RD); - !Fields.empty()) - return Fields; - return {}; - }); -} - -void UncheckedStatusOrAccessModel::transfer(const CFGElement &Elt, Lattice &L, - Environment &Env) { - LatticeTransferState State(L, Env); - TransferMatchSwitch(Elt, getASTContext(), State); -} - -} // namespace clang::dataflow::statusor_model diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt index 1d932ec6e8a96..35082387b46e9 100644 --- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt +++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt @@ -25,8 +25,6 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests TransferTest.cpp TypeErasedDataflowAnalysisTest.cpp UncheckedOptionalAccessModelTest.cpp - UncheckedStatusOrAccessModelTest.cpp - UncheckedStatusOrAccessModelTestFixture.cpp ValueTest.cpp WatchedLiteralsSolverTest.cpp CLANG_LIBS diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp deleted file mode 100644 index 07d3f2412e842..0000000000000 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp +++ /dev/null @@ -1,34 +0,0 @@ -//===- UncheckedStatusOrAccessModelTest.cpp -------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include <utility> - -#include "UncheckedStatusOrAccessModelTestFixture.h" -#include "clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h" -#include "gtest/gtest.h" - -namespace clang::dataflow::statusor_model { -namespace { - -INSTANTIATE_TEST_SUITE_P( - UncheckedStatusOrAccessModelTest, UncheckedStatusOrAccessModelTest, - testing::Values( - std::make_pair(new UncheckedStatusOrAccessModelTestExecutor< - UncheckedStatusOrAccessModel>(), - UncheckedStatusOrAccessModelTestAliasKind::kUnaliased), - std::make_pair( - new UncheckedStatusOrAccessModelTestExecutor< - UncheckedStatusOrAccessModel>(), - UncheckedStatusOrAccessModelTestAliasKind::kPartiallyAliased), - std::make_pair( - new UncheckedStatusOrAccessModelTestExecutor< - UncheckedStatusOrAccessModel>(), - UncheckedStatusOrAccessModelTestAliasKind::kFullyAliased))); -} // namespace - -} // namespace clang::dataflow::statusor_model diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp deleted file mode 100644 index 4827cc1d0a7e9..0000000000000 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ /dev/null @@ -1,2518 +0,0 @@ -//===- UncheckedStatusOrAccessModelTestFixture.cpp ------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include "UncheckedStatusOrAccessModelTestFixture.h" -#include "MockHeaders.h" -#include "llvm/Support/ErrorHandling.h" - -#include <string> -#include <utility> -#include <vector> - -#include "gtest/gtest.h" - -namespace clang::dataflow::statusor_model { -namespace { - -TE... [truncated] |
fmayer deleted the revert-162932-users/fmayer/spr/flowsensitive-statusor-1n-add-minimal-model branch 
| LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/166/builds/3107 Here is the relevant piece of the build log for the reference |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Reverts #162932