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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
