[PATCH] D82940: [ASTReader][ASTWriter][PCH][Modules] Fix (de)serialization of SwitchCases
Bruno Ricci via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 1 12:27:13 PDT 2020
riccibruno added a comment.
In D82940#2125939 <https://reviews.llvm.org/D82940#2125939>, @martong wrote:
> In D82940#2125183 <https://reviews.llvm.org/D82940#2125183>, @riccibruno wrote:
>
> > > I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?
> >
> > Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:
> >
> > void should_not_crash_with_switch_in_lambda() {
> > switch (1)
> > default:;
> > auto f = [] {
> > switch (1)
> > default:;
> > };
> > }
> >
> >
> > Before the serialization of `LambdaExpr` for the `LambdaExpr *E`, we have `E->getBody() == E->getCallOperator()->getBody()`.
> > After serialization this is not true anymore because we are writing and reading the body directly when visiting the `LambdaExpr`
> > (the captures have the same problem).
>
>
> The same is true in case of the simplest lambda example:
>
> void foo() { // line 11
> []{};
> }
>
>
> The AST that we got from the Parser has only one CompoundStmt node, but referenced in the CXXMethodDecl and added as a direct child to LambdaExpr:
>
> `-LambdaExpr 0x5643235372c8 <line:12:5, col:8>
> |-CXXRecordDecl 0x564323536c98 <col:5> col:5 implicit class definition
> | |-CXXMethodDecl 0x564323536dd8 <col:6, col:8> col:5 constexpr operator() 'auto () const -> void' inline
> | | `-CompoundStmt 0x564323536e88 <col:7, col:8>
> `-CompoundStmt 0x564323536e88 <col:7, col:8>
>
>
> After serialization and deserialization, the CompoundStmt is just copied, notice the different pointer values:
>
> `-LambdaExpr 0x55a49f35f830 <line:12:5, col:8>
> |-CXXRecordDecl 0x55a49f35f860 <col:5> col:5 imported implicit <undeserialized declarations> class definition
> | |-CXXMethodDecl 0x55a49f35fac8 <col:6, col:8> col:5 imported constexpr operator() 'auto () const -> void' inline
> | | `-CompoundStmt 0x55a49f360110 <col:7, col:8>
> `-CompoundStmt 0x55a49f35f820 <col:7, col:8>
>
>
> This suggests that we should prepare the ASTReader/ASTWriter to be able to handle references to Stmts similarly as it can already handle references to Decls (see `ASTWriter::DeclIDs`).
> Does this sound as a good direction, should I move forward this way?
Yes, I think that my patch just traded-off a bug for another bug. What about the following instead:
Initially zero the `Stmt *` for the body, then when the body is needed (either with `getBody` or with `children`)
populate it if needed from the call operator. This is what was done before with `getBody`.
This preserve the benefit of having a lazily deserialized body while being a simple fix.
Thoughts? Am I missing something?
If not I can cook-up a patch either tonight or tomorrow.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82940/new/
https://reviews.llvm.org/D82940
More information about the cfe-commits
mailing list