[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

7426

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

7426

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 ↗

(On Diff #545016)

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 ↗

(On Diff #545016)

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/lib/AST/ASTImporter.cpp

 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());
+
+  return UO;
 }
 
 ExpectedStmt
 
 ASTNodeImporter::VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
   auto ToOperatorLoc = importChecked(Err, E->getOperatorLoc());
   auto ToRParenLoc = importChecked(Err, E->getRParenLoc());
   if (Err)
     return std::move(Err);
 
   if (E->isArgumentType()) {
     Expected<TypeSourceInfo *> ToArgumentTypeInfoOrErr =
         import(E->getArgumentTypeInfo());
     if (!ToArgumentTypeInfoOrErr)
       return ToArgumentTypeInfoOrErr.takeError();
 
     return new (Importer.getToContext()) UnaryExprOrTypeTraitExpr(
         E->getKind(), *ToArgumentTypeInfoOrErr, ToType, ToOperatorLoc,
         ToRParenLoc);
   }
 
   ExpectedExpr ToArgumentExprOrErr = import(E->getArgumentExpr());
   if (!ToArgumentExprOrErr)
     return ToArgumentExprOrErr.takeError();
 
   return new (Importer.getToContext()) UnaryExprOrTypeTraitExpr(
       E->getKind(), *ToArgumentExprOrErr, ToType, ToOperatorLoc, ToRParenLoc);

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"

       )";
   Decl *FromTU = getTuDecl(Code, Lang_CXX11);
 
   auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
       FromTU, cxxMethodDecl(hasName("A::m")));
   Import(FromF, Lang_CXX11);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportCirularRefFieldsWithoutCorruptedRecordLayoutCacheTest) {
+  // 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];