[ASTImporter] Fix corrupted RecordLayout introduced by circular referenced fields

[danix800]

Summary

UnaryOperator(&)'s creation might need layout of some records
whose fields importation are still on fly, the layout is incorrectly
computed and cached. Clients relying on this will not work properly
or crash direclty (e.g StaticAnalyzer's MemRegion.cpp (calculateOffset)).

Use UnaryOperator::CreateEmpty() instead of UnaryOperator::Create()
to avoid this computation.

https://github.com/llvm/llvm-project/issues/64170

[danix800]

Restore in-class initializer importing when minimal importing.

[danix800]

Remove case #1 (fixed by https://reviews.llvm.org/D155574 from @balazske which is better).

[shafik]

clang/lib/AST/ASTImporter.cpp

7403

I don't see the following values from the old code used: E->getValueKind(), E->getObjectKind() and E->getFPOptionsOverride()

[danix800]

clang/lib/AST/ASTImporter.cpp

7403

These three values are set for all Exprs in Import(Stmt *FromS):

Expected<Stmt *> ASTImporter::Import(Stmt *FromS) {
  // ...
  if (auto *ToE = dyn_cast<Expr>(*ToSOrErr)) {
    auto *FromE = cast<Expr>(FromS);
    // Copy ExprBitfields, which may not be handled in Expr subclasses
    // constructors.
    ToE->setValueKind(FromE->getValueKind());
    ToE->setObjectKind(FromE->getObjectKind());
    ToE->setDependence(FromE->getDependence());
  }
  // ...
}

[danix800]

clang/lib/AST/ASTImporter.cpp

7403

Oh only the first two are set, FPOptionsOverride is missing! I'll fix that.

[danix800]

Add missing field value setting (FPOptionsOverride).

[balazske]

clang/lib/AST/ASTImporter.cpp

7418

Why is it better to use CreateEmpty instead of the old code? Does Create do something that does not work at this situation (probably getting the layout)? If yes the same should be done later at some point, can you explain how this works?

[danix800]

clang/lib/AST/ASTImporter.cpp

7418

CreateEmpty is used to avoid unnecessarily (re-)computeDependence() of the imported UnaryOperator. See UnaryOperator's ctors for details.

While importing fields there could be deps on current record's layout. In the following testcase, importing A::b
will take this path: A::b => B => ... => B::f => ... => UnaryOperator(&).

class B;
class A {
  B* b;
  int c;
};
class B {
  A *f() { return &((B *)0)->a; }
  A a;
};

The (non-empty) UnaryOperator (&) ctor needs the whole RecordLayout (including A) to be evaluated for
computeDependence(), but none of fields of`A` is imported at this moment. The RecordLayout is incorrectly
computed and what's worse more is, the RecordLayout is cached for later use. Any clients relying on this RecordLayout
would be broken. For example EXPECT_TRUE(ToLayout.getFieldOffset(0) == 0); would simply crash with

ASTTests: /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTVector.h:116: clang::ASTVector::const_reference clang::ASTVector<unsigned long>::operator[](unsigned int) const [T = unsigned long]: Assertion `Begin + idx < End' failed.
...
#10 0x0000558b01b39942 clang::ASTVector<unsigned long>::operator[](unsigned int) const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTVector.h:0:5
#11 0x0000558b01aaeb0f clang::ASTRecordLayout::getFieldOffset(unsigned int) const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/RecordLayout.h:201:12
#12 0x0000558b01a6d115 clang::ast_matchers::ASTImporterOptionSpecificTestBase_ImportCirularRefFieldsWithoutCorruptedRecordLayoutCacheTest_Test::TestBody() /home/xxxxx/Sources/llvm-project-main/clang/unittests/AST/ASTImporterTest.cpp:8067:3
...

UnaryOperator is just one example that could introduce this kind of problem. If I understand it correctly, ASTImporter is much like ASTReader which could use CreateEmpty directly to avoid recomputing some of the
data, most of which could be copied from From context, as ASTReader reads node data when deserialization.

Maybe ASTImporter could be ideally refactored into this behaviour on node creation.

[balazske]

The fix looks good, only a person with AST competence should have a look at it because a change in Expr.h.

clang/unittests/AST/ASTImporterTest.cpp

8033

A small thing, ImportCirularRefFieldsWithoutCorruptedRecordLayoutCache (no Test ending) is the usual naming for these tests.

[aaron.ballman]

clang/include/clang/AST/Expr.h

2343–2344

Rather than make this a public API, would it instead make sense to add friend class ASTNodeImporter; at the end of the class?

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

8033

Thanks for reminding, I'll fix all testcases ending with Test.

[danix800]

clang/include/clang/AST/Expr.h

2343–2344

Tested & confirmed, it's unnecessary to expose this method, friend is much better. Thanks!

[danix800]

1. Update ReleaseNotes.rst;
2. Fix testcase names (remove Test suffix);
3. Use friend instead of exposing API.

[steakhal]

Have you considered back porting this to clang-17?

[danix800]

In D156201#4551270, @steakhal wrote:

Have you considered back porting this to clang-17?

How to backport? I'm not familiar with this. Is it plain cherry-pick to specific branch?

[aaron.ballman]

LGTM!

In D156201#4551332, @danix800 wrote:

In D156201#4551270, @steakhal wrote:

Have you considered back porting this to clang-17?

How to backport? I'm not familiar with this. Is it plain cherry-pick to specific branch?

We have some documented instructions here: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches -- I don't oppose adding this to the release branch (it seems simple and straightforward enough), but if it's not fixing a regression or other critical issue, it might make sense to leave it out of 17.x so there's less moving parts during the release.

[danix800]

In D156201#4551464, @aaron.ballman wrote:

LGTM!

In D156201#4551332, @danix800 wrote:

In D156201#4551270, @steakhal wrote:

Have you considered back porting this to clang-17?

How to backport? I'm not familiar with this. Is it plain cherry-pick to specific branch?

We have some documented instructions here: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches -- I don't oppose adding this to the release branch (it seems simple and straightforward enough), but if it's not fixing a regression or other critical issue, it might make sense to leave it out of 17.x so there's less moving parts during the release.

I'll leave it out of 17.x.

clang/docs/ReleaseNotes.rst

- Fix cases where we ignore ambiguous name lookup when looking up memebers.
   (`#29942 <https://github.com/llvm/llvm-project/issues/29942>_`),
   (`#35574 <https://github.com/llvm/llvm-project/issues/35574>_`) and
   (`#27224 <https://github.com/llvm/llvm-project/issues/27224>_`).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
   `Issue 64169 <https://github.com/llvm/llvm-project/issues/64169>`_
+- Remove unnecessary RecordLayout computation when importing UnaryOperator. The
+  computed RecordLayout is incorrect if fields are not completely imported and
+  should not be cached.
+  `Issue 64170 <https://github.com/llvm/llvm-project/issues/64170>`_
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Target Specific Changes

clang/include/clang/AST/Expr.h

public:
   /// Is FPFeatures in Trailing Storage?
   bool hasStoredFPFeatures() const { return UnaryOperatorBits.HasFPFeatures; }
 
   /// Get FPFeatures from trailing storage.
   FPOptionsOverride getStoredFPFeatures() const {
     return getTrailingFPFeatures();
   }
 
 protected:
-  /// Set FPFeatures in trailing storage, used only by Serialization
+  /// Set FPFeatures in trailing storage, used by Serialization & ASTImporter.
   void setStoredFPFeatures(FPOptionsOverride F) { getTrailingFPFeatures() = F; }
 
 public:
   /// Get the FP features status of this operator. Only meaningful for
   /// operations on floating point types.
   FPOptions getFPFeaturesInEffect(const LangOptions &LO) const {
     if (UnaryOperatorBits.HasFPFeatures)
       return getStoredFPFeatures().applyOverrides(LO);
     return FPOptions::defaultWithoutTrailingStorage(LO);
   }
   FPOptionsOverride getFPOptionsOverride() const {
     if (UnaryOperatorBits.HasFPFeatures)
       return getStoredFPFeatures();
     return FPOptionsOverride();
   }
 
   friend TrailingObjects;
+  friend class ASTNodeImporter;
   friend class ASTReader;
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 };
 
 /// Helper class for OffsetOfExpr.
 
 // __builtin_offsetof(type, identifier(.identifier|[expr])*)

clang/lib/AST/ASTImporter.cpp

 ExpectedStmt ASTNodeImporter::VisitStmtExpr(StmtExpr *E) {
   Error Err = Error::success();
   auto ToSubStmt = importChecked(Err, E->getSubStmt());
   auto ToType = importChecked(Err, E->getType());
   auto ToLParenLoc = importChecked(Err, E->getLParenLoc());
   auto ToRParenLoc = importChecked(Err, E->getRParenLoc());
   if (Err)
     return std::move(Err);
 
   return new (Importer.getToContext())
       StmtExpr(ToSubStmt, ToType, ToLParenLoc, ToRParenLoc,
                E->getTemplateDepth());
 }
 
 ExpectedStmt ASTNodeImporter::VisitUnaryOperator(UnaryOperator *E) {
   Error Err = Error::success();
   auto ToSubExpr = importChecked(Err, E->getSubExpr());
   auto ToType = importChecked(Err, E->getType());
   auto ToOperatorLoc = importChecked(Err, E->getOperatorLoc());
   if (Err)
     return std::move(Err);
 
-  return UnaryOperator::Create(
-      Importer.getToContext(), ToSubExpr, E->getOpcode(), ToType,
-      E->getValueKind(), E->getObjectKind(), ToOperatorLoc, E->canOverflow(),
-      E->getFPOptionsOverride());
+  auto *UO = UnaryOperator::CreateEmpty(Importer.getToContext(),
+                                        E->hasStoredFPFeatures());
+  UO->setType(ToType);
+  UO->setSubExpr(ToSubExpr);
+  UO->setOpcode(E->getOpcode());
+  UO->setOperatorLoc(ToOperatorLoc);
+  UO->setCanOverflow(E->canOverflow());
+  if (E->hasStoredFPFeatures())
+    UO->setStoredFPFeatures(E->getStoredFPFeatures());
+
+  return UO;
 }
 
 ExpectedStmt
 
 ASTNodeImporter::VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
   auto ToOperatorLoc = importChecked(Err, E->getOperatorLoc());

clang/unittests/AST/ASTImporterTest.cpp

 //===- unittest/AST/ASTImporterTest.cpp - AST node import test ------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Tests for the correct import of AST nodes from one AST context to another.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/RecordLayout.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SmallVectorMemoryBuffer.h"
 
 #include "clang/AST/DeclContextInternals.h"
 #include "gtest/gtest.h"
 
 #include "ASTImporterFixtures.h"
TEST_P(ImportFunctions,
   EXPECT_EQ(ToDFOutOfClass->getLexicalDeclContext(), ToTU);
 
   // Check that the redecl chain is intact.
   EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
   EXPECT_EQ(ToDFOutOfClass->getPreviousDecl(), ToDFInClass);
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
-       ImportVirtualOverriddenMethodOnALoopTest) {
+       ImportVirtualOverriddenMethodOnALoop) {
   // B::f() calls => f1() ==> C ==> C::f()
   //     \
   //      \---- A::f()
   //
   // C::f()'s ImportOverriddenMethods() asserts B::isVirtual(), so B::f()'s
   // ImportOverriddenMethods() should be completed before B::f()'s body
   const char *Code =
       R"(
EXPECT_EQ(
       FromDGCopyCtor->getTemplatedDecl());
   EXPECT_EQ(FromDGOther->getDescribedTemplate()
                 ->getTemplateParameters()
                 ->getParam(0)
                 ->getDeclContext(),
             FromDGOther);
 
   // Here the import of 'ToDGCopyCtor' and 'ToDGCtor' is reversed relative to
   // the previous test.
   auto *ToDGCopyCtor = Import(FromDGCopyCtor, Lang_CXX17);
   auto *ToDGCtor = Import(FromDGCtor, Lang_CXX17);
   auto *ToDGOther = Import(FromDGOther, Lang_CXX17);
   ASSERT_TRUE(ToDGCtor);
   ASSERT_TRUE(ToDGCopyCtor);
   ASSERT_TRUE(ToDGOther);
 
   TemplateParameterList *ToDGCtorTP = ToDGCtor->getTemplateParameters();
TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuideDifferentOrder) {
   EXPECT_EQ(ToDGOther->getDescribedTemplate()
                 ->getTemplateParameters()
                 ->getParam(0)
                 ->getDeclContext(),
             ToDGOther);
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
-       ImportFieldsFirstForCorrectRecordLayoutTest) {
+       ImportFieldsFirstForCorrectRecordLayout) {
   // UnaryOperator(&) triggers RecordLayout computation, which relies on
   // correctly imported fields.
   auto Code =
       R"(
       class A {
         int m() {
           return &((A *)0)->f1 - &((A *)0)->f2;
         }
         int f1;
         int f2;
       };
       )";
   Decl *FromTU = getTuDecl(Code, Lang_CXX11);
 
   auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
       FromTU, cxxMethodDecl(hasName("A::m")));
   Import(FromF, Lang_CXX11);
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportCirularRefFieldsWithoutCorruptedRecordLayoutCache) {
+  // Import sequence: A => A.b => B => B.f() => ... => UnaryOperator(&) => ...
+  //
+  // UnaryOperator(&) should not introduce invalid RecordLayout since 'A' is
+  // still not completely imported.
+  auto Code =
+      R"(
+      class B;
+      class A {
+        B* b;
+        int c;
+      };
+      class B {
+        A *f() { return &((B *)0)->a; }
+        A a;
+      };
+      )";
+
+  auto *FromR = FirstDeclMatcher<CXXRecordDecl>().match(
+      getTuDecl(Code, Lang_CXX11), cxxRecordDecl(hasName("A")));
+  FromR = FromR->getDefinition();
+  auto &FromAST = FromR->getASTContext();
+  auto *ToR = Import(FromR, Lang_CXX11);
+  auto &ToAST = ToR->getASTContext();
+
+  uint64_t SecondFieldOffset = FromAST.getTypeSize(FromAST.VoidPtrTy);
+
+  EXPECT_TRUE(FromR->isCompleteDefinition());
+  const auto &FromLayout = FromAST.getASTRecordLayout(FromR);
+  EXPECT_TRUE(FromLayout.getFieldOffset(0) == 0);
+  EXPECT_TRUE(FromLayout.getFieldOffset(1) == SecondFieldOffset);
+
+  EXPECT_TRUE(ToR->isCompleteDefinition());
+  const auto &ToLayout = ToAST.getASTRecordLayout(ToR);
+  EXPECT_TRUE(ToLayout.getFieldOffset(0) == 0);
+  EXPECT_TRUE(ToLayout.getFieldOffset(1) == SecondFieldOffset);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
        ImportRecordWithLayoutRequestingExpr) {
   TranslationUnitDecl *FromTU = getTuDecl(
       R"(
       struct A {
         int idx;
         static void foo(A x) {
           (void)&"text"[x.idx];
         }