Author: Samira Bazuzi Date: 2024-02-26T14:23:46+01:00 New Revision: c4e94633e8a48ee33115d5d3161ee142fc1c9700
URL: https://github.com/llvm/llvm-project/commit/c4e94633e8a48ee33115d5d3161ee142fc1c9700 DIFF: https://github.com/llvm/llvm-project/commit/c4e94633e8a48ee33115d5d3161ee142fc1c9700.diff LOG: Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." (#82856) Reverts llvm/llvm-project#82348, which caused crashes when analyzing empty InitListExprs for unions, e.g. ```cc union U { double double_value; int int_value; }; void target() { U value; value = {}; } ``` Co-authored-by: Samira Bazuzi <baz...@users.noreply.github.com> Added: Modified: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/TestingSupport.h clang/unittests/Analysis/FlowSensitive/TransferTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index b3dc940705f870..0aecc749bf415c 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -753,12 +753,9 @@ RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE, RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME, const Environment &Env); -/// Returns the fields of a `RecordDecl` that are initialized by an -/// `InitListExpr`, in the order in which they appear in -/// `InitListExpr::inits()`. -/// `Init->getType()` must be a record type. -std::vector<const FieldDecl *> -getFieldsForInitListExpr(const InitListExpr *InitList); +/// Returns the fields of `RD` that are initialized by an `InitListExpr`, in the +/// order in which they appear in `InitListExpr::inits()`. +std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD); /// Associates a new `RecordValue` with `Loc` and returns the new value. RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 0cfc26ea952cda..d487944ce92111 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -361,8 +361,8 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields, if (const auto *FD = dyn_cast<FieldDecl>(VD)) Fields.insert(FD); } else if (auto *InitList = dyn_cast<InitListExpr>(&S)) { - if (InitList->getType()->isRecordType()) - for (const auto *FD : getFieldsForInitListExpr(InitList)) + if (RecordDecl *RD = InitList->getType()->getAsRecordDecl()) + for (const auto *FD : getFieldsForInitListExpr(RD)) Fields.insert(FD); } } @@ -1104,22 +1104,12 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME, return Env.get<RecordStorageLocation>(*Base); } -std::vector<const FieldDecl *> -getFieldsForInitListExpr(const InitListExpr *InitList) { - const RecordDecl *RD = InitList->getType()->getAsRecordDecl(); - assert(RD != nullptr); - - std::vector<const FieldDecl *> Fields; - - if (InitList->getType()->isUnionType()) { - Fields.push_back(InitList->getInitializedFieldInUnion()); - return Fields; - } - +std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) { // Unnamed bitfields are only used for padding and do not appear in // `InitListExpr`'s inits. However, those fields do appear in `RecordDecl`'s // field list, and we thus need to remove them before mapping inits to // fields to avoid mapping inits to the wrongs fields. + std::vector<FieldDecl *> Fields; llvm::copy_if( RD->fields(), std::back_inserter(Fields), [](const FieldDecl *Field) { return !Field->isUnnamedBitfield(); }); diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index cd1f04e53cff68..fe13e919bddcd8 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -663,7 +663,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { void VisitInitListExpr(const InitListExpr *S) { QualType Type = S->getType(); - if (!Type->isRecordType()) { + if (Type->isUnionType()) { + // FIXME: Initialize unions properly. + if (auto *Val = Env.createValue(Type)) + Env.setValue(*S, *Val); + return; + } + + if (!Type->isStructureOrClassType()) { // Until array initialization is implemented, we don't need to care about // cases where `getNumInits() > 1`. if (S->getNumInits() == 1) @@ -681,9 +688,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs; // This only contains the direct fields for the given type. - std::vector<const FieldDecl *> FieldsForInit = getFieldsForInitListExpr(S); + std::vector<FieldDecl *> FieldsForInit = + getFieldsForInitListExpr(Type->getAsRecordDecl()); - // `S->inits()` contains all the initializer expressions, including the + // `S->inits()` contains all the initializer epressions, including the // ones for direct base classes. auto Inits = S->inits(); size_t InitIdx = 0; @@ -723,17 +731,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { FieldLocs.insert({Field, &Loc}); } - // In the case of a union, we don't in general have initializers for all - // of the fields. Create storage locations for the remaining fields (but - // don't associate them with values). - if (Type->isUnionType()) { - for (const FieldDecl *Field : - Env.getDataflowAnalysisContext().getModeledFields(Type)) { - if (auto [it, inserted] = FieldLocs.insert({Field, nullptr}); inserted) - it->second = &Env.createStorageLocation(Field->getType()); - } - } - // Check that we satisfy the invariant that a `RecordStorageLoation` // contains exactly the set of modeled fields for that type. // `ModeledFields` includes fields from all the bases, but only the diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h index b7cf6cc966edb0..0d36d2802897fd 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h @@ -432,8 +432,6 @@ llvm::Error checkDataflowWithNoopAnalysis( {}); /// Returns the `ValueDecl` for the given identifier. -/// The returned pointer is guaranteed to be non-null; the function asserts if -/// no `ValueDecl` with the given name is found. /// /// Requirements: /// @@ -477,15 +475,6 @@ ValueT &getValueForDecl(ASTContext &ASTCtx, const Environment &Env, return *cast<ValueT>(Env.getValue(*VD)); } -/// Returns the storage location for the field called `Name` of `Loc`. -/// Optionally casts the field storage location to `T`. -template <typename T = StorageLocation> -std::enable_if_t<std::is_base_of_v<StorageLocation, T>, T &> -getFieldLoc(const RecordStorageLocation &Loc, llvm::StringRef Name, - ASTContext &ASTCtx) { - return *cast<T>(Loc.getChild(*findValueDecl(ASTCtx, Name))); -} - /// Returns the value of a `Field` on the record referenced by `Loc.` /// Returns null if `Loc` is null. inline Value *getFieldValue(const RecordStorageLocation *Loc, @@ -498,14 +487,6 @@ inline Value *getFieldValue(const RecordStorageLocation *Loc, return Env.getValue(*FieldLoc); } -/// Returns the value of a `Field` on the record referenced by `Loc.` -/// Returns null if `Loc` is null. -inline Value *getFieldValue(const RecordStorageLocation *Loc, - llvm::StringRef Name, ASTContext &ASTCtx, - const Environment &Env) { - return getFieldValue(Loc, *findValueDecl(ASTCtx, Name), Env); -} - /// Creates and owns constraints which are boolean values. class ConstraintContext { unsigned NextAtom = 0; diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index e7d74581865a32..a65b0446ac7818 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -2377,24 +2377,14 @@ TEST(TransferTest, InitListExprAsUnion) { } F; public: - constexpr target() : F{nullptr} { - int *null = nullptr; - F.b; // Make sure we reference 'b' so it is modeled. - // [[p]] - } + constexpr target() : F{nullptr} {} }; )cc"; runDataflow( Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { - const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); - - auto &FLoc = getFieldLoc<RecordStorageLocation>( - *Env.getThisPointeeStorageLocation(), "F", ASTCtx); - auto *AVal = cast<PointerValue>(getFieldValue(&FLoc, "a", ASTCtx, Env)); - ASSERT_EQ(AVal, &getValueForDecl<PointerValue>(ASTCtx, Env, "null")); - ASSERT_EQ(getFieldValue(&FLoc, "b", ASTCtx, Env), nullptr); + // Just verify that it doesn't crash. }); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
