Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." #82856

bazuzi · 2024-02-24T01:13:15Z

Reverts #82348, which caused crashes when analyzing empty InitListExprs for unions, e.g.

union U {
  double double_value;
  int int_value;
};

void target() {
  U value;
  value = {};
}

…pe. (llvm#82…" This reverts commit 4725993.

llvmbot · 2024-02-24T01:13:50Z

@llvm/pr-subscribers-clang

Author: Samira Bazuzi (bazuzi)

Changes

Reverts llvm/llvm-project#82348, which caused crashes when analyzing empty InitListExprs for unions.

Full diff: https://github.com/llvm/llvm-project/pull/82856.diff

5 Files Affected:

(modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+3-6)
(modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+4-14)
(modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+11-14)
(modified) clang/unittests/Analysis/FlowSensitive/TestingSupport.h (-19)
(modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+2-12)

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.
       });
 }

llvmbot · 2024-02-24T01:13:51Z

@llvm/pr-subscribers-clang-analysis

Author: Samira Bazuzi (bazuzi)

Changes

Reverts llvm/llvm-project#82348, which caused crashes when analyzing empty InitListExprs for unions.

Full diff: https://github.com/llvm/llvm-project/pull/82856.diff

5 Files Affected:

(modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+3-6)
(modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+4-14)
(modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+11-14)
(modified) clang/unittests/Analysis/FlowSensitive/TestingSupport.h (-19)
(modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+2-12)

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.
       });
 }

bazuzi · 2024-02-24T01:14:05Z

@ymand Can you merge this?

github-actions · 2024-02-24T01:14:28Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

martinboehme · 2024-02-26T09:40:06Z

Sorry about the breakage. Will re-land soon with a test for this case and a fix.

martinboehme · 2024-02-26T11:44:33Z

I see you haven't merged this revert yet.

Alternatively, here's a fix-forward, if you're prepared to go with that:

#82986

Otherwise, do of course merge the revert, and I'll reland with the fix-forward incorporated.

…ype." (llvm#82856) This reverts commit c4e9463.

Revert "[clang][dataflow] Correctly handle InitListExpr of union ty…

ae17f89

…pe. (llvm#82…" This reverts commit 4725993.

llvmbot added clang Clang issues not falling into any other category clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Feb 24, 2024

ymand approved these changes Feb 26, 2024

View reviewed changes

martinboehme merged commit c4e9463 into llvm:main Feb 26, 2024
7 of 8 checks passed

martinboehme added a commit to martinboehme/llvm-project that referenced this pull request Feb 27, 2024

Reapply "[clang][dataflow] Correctly handle InitListExpr of union t…

cd64b0e

…ype." (llvm#82856) This reverts commit c4e9463.

bazuzi deleted the revert-82348-piper_export_cl_608548818 branch April 12, 2024 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." #82856

Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." #82856

bazuzi commented Feb 24, 2024 •

edited

llvmbot commented Feb 24, 2024

llvmbot commented Feb 24, 2024

bazuzi commented Feb 24, 2024

github-actions bot commented Feb 24, 2024

martinboehme commented Feb 26, 2024

martinboehme commented Feb 26, 2024

Revert "[clang][dataflow] Correctly handle InitListExpr of union type." #82856

Revert "[clang][dataflow] Correctly handle InitListExpr of union type." #82856

Conversation

bazuzi commented Feb 24, 2024 • edited

llvmbot commented Feb 24, 2024

llvmbot commented Feb 24, 2024

bazuzi commented Feb 24, 2024

github-actions bot commented Feb 24, 2024

martinboehme commented Feb 26, 2024

martinboehme commented Feb 26, 2024

Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." #82856

Revert "[clang][dataflow] Correctly handle `InitListExpr` of union type." #82856

bazuzi commented Feb 24, 2024 •

edited