[PATCH] D156201: [ASTImporter] Fix corrupted RecordLayout introduced by circular referenced fields

[Ding Fei via Phabricator via cfe-commits]

danix800 added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7418
+
+  return UO;
 }
----------------
balazske wrote:
> 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?
`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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156201/new/

https://reviews.llvm.org/D156201