Skip to content

[clang][dataflow] reland #96766 with fix #98896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Conversation

martinboehme
Copy link
Contributor

@llvmbot 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 Jul 15, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 15, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: None (martinboehme)

Changes
  • Reapply "[clang][dataflow] Teach AnalysisASTVisitor that typeid() can be evaluated." (#96766)
  • Turn on RTTI explicitly in checkDataflowWithNoopAnalysis().

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

3 Files Affected:

  • (modified) clang/include/clang/Analysis/FlowSensitive/ASTOps.h (+5-1)
  • (modified) clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp (+6-1)
  • (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+43)
diff --git a/clang/include/clang/Analysis/FlowSensitive/ASTOps.h b/clang/include/clang/Analysis/FlowSensitive/ASTOps.h
index 925b99af9141a..f9c923a36ad22 100644
--- a/clang/include/clang/Analysis/FlowSensitive/ASTOps.h
+++ b/clang/include/clang/Analysis/FlowSensitive/ASTOps.h
@@ -113,7 +113,11 @@ class AnalysisASTVisitor : public RecursiveASTVisitor<Derived> {
   // nevertheless it appears in the Clang CFG, so we don't exclude it here.
   bool TraverseDecltypeTypeLoc(DecltypeTypeLoc) { return true; }
   bool TraverseTypeOfExprTypeLoc(TypeOfExprTypeLoc) { return true; }
-  bool TraverseCXXTypeidExpr(CXXTypeidExpr *) { return true; }
+  bool TraverseCXXTypeidExpr(CXXTypeidExpr *TIE) {
+    if (TIE->isPotentiallyEvaluated())
+      return RecursiveASTVisitor<Derived>::TraverseCXXTypeidExpr(TIE);
+    return true;
+  }
   bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *) {
     return true;
   }
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
index 5c4d42c6ccdcf..622de6323ad33 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -176,13 +176,18 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
     DataflowAnalysisOptions Options, LangStandard::Kind Std,
     std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback) {
   llvm::SmallVector<std::string, 3> ASTBuildArgs = {
+      "-fsyntax-only",
       // -fnodelayed-template-parsing is the default everywhere but on Windows.
       // Set it explicitly so that tests behave the same on Windows as on other
       // platforms.
+      "-fno-delayed-template-parsing",
       // Set -Wno-unused-value because it's often desirable in tests to write
       // expressions with unused value, and we don't want the output to be
       // cluttered with warnings about them.
-      "-fsyntax-only", "-fno-delayed-template-parsing", "-Wno-unused-value",
+      "-Wno-unused-value",
+      // Some build environments don't have RTTI enabled by default.
+      // Enable it explicitly to make sure tests work in all environments.
+      "-frtti",
       "-std=" +
           std::string(LangStandard::getLangStandardForKind(Std).getName())};
   AnalysisInputs<NoopAnalysis> AI(
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index e743eefa5d458..39e7001393e5e 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1637,6 +1637,49 @@ TEST(TransferTest, StructModeledFieldsWithAccessor) {
       });
 }
 
+TEST(TransferTest, StructModeledFieldsInTypeid) {
+  // Test that we model fields mentioned inside a `typeid()` expression only if
+  // that expression is potentially evaluated -- i.e. if the expression inside
+  // `typeid()` is a glvalue of polymorphic type (see
+  // `CXXTypeidExpr::isPotentiallyEvaluated()` and [expr.typeid]p3).
+  std::string Code = R"(
+    // Definitions needed for `typeid`.
+    namespace std {
+      class type_info {};
+      class bad_typeid {};
+    }  // namespace std
+
+    struct NonPolymorphic {};
+
+    struct Polymorphic {
+      virtual ~Polymorphic() = default;
+    };
+
+    struct S {
+      NonPolymorphic *NonPoly;
+      Polymorphic *Poly;
+    };
+
+    void target(S &s) {
+      typeid(*s.NonPoly);
+      typeid(*s.Poly);
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        auto &SLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s");
+        std::vector<const ValueDecl *> Fields;
+        for (auto [Field, _] : SLoc.children())
+          Fields.push_back(Field);
+        EXPECT_THAT(Fields,
+                    UnorderedElementsAre(findValueDecl(ASTCtx, "Poly")));
+      });
+}
+
 TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
   std::string Code = R"(
     struct Base1 {

@martinboehme
Copy link
Contributor Author

Per discussion with @sam-mccall, the issue that was causing the buildbot failure when I originally landed #98896 is that RTTI is turned off by default on Playstation targets. To resolve this, I'm now turning on -frtti explicitly.

@martinboehme martinboehme requested review from ymand and Xazax-hun July 15, 2024 13:17
Copy link
Collaborator

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@martinboehme martinboehme changed the title reland [clang][dataflow] reland #96766 with fix Jul 16, 2024
@martinboehme martinboehme merged commit 655c223 into llvm:main Jul 16, 2024
11 checks passed
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
- **Reapply "[clang][dataflow] Teach `AnalysisASTVisitor` that
`typeid()` can be evaluated." (#96766)**
- **Turn on RTTI explicitly in `checkDataflowWithNoopAnalysis()`.**

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants