[clang] [Serialization] Fix lazy template loading (PR #133057)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > As I hinted above, that doesn't work. More specifically, I tried
> > ```diff
> > diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
> > b/clang/lib/Serialization/ASTReaderDecl.cpp
> > index 882d54f31280..d63824c4ba18 100644
> > --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> > +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> > @@ -2107,8 +2107,9 @@ void ASTDeclMerger::MergeDefinitionData(
> >      auto *Def = DD.Definition;
> >      DD = std::move(MergeDD);
> >      DD.Definition = Def;
> > -    for (auto *D : Def->redecls())
> > -      cast<CXXRecordDecl>(D)->DefinitionData = &DD;
> > +    Def = Def->getMostRecentDecl();
> > +    while ((Def = Def->getPreviousDecl()))
> > +      cast<CXXRecordDecl>(Def)->DefinitionData = &DD;
> >      return;
> >    }
> >  
> > ```
> 
> I'm stupid, the condition during the first iteration of the `while` loop will 
> skip over the most recent declaration 😵 the following works:
> 
> ```diff
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
>      auto *Def = DD.Definition;
>      DD = std::move(MergeDD);
>      DD.Definition = Def;
> -    for (auto *D : Def->redecls())
> -      cast<CXXRecordDecl>(D)->DefinitionData = &DD;
> +    Def = Def->getMostRecentDecl();
> +    do {
> +      cast<CXXRecordDecl>(Def)->DefinitionData = &DD;
> +    } while ((Def = Def->getPreviousDecl()));
>      return;
>    }
>  
> ```
> 
> > You can't use `getMostRecentDecl `, this will trigger deserializations. 
> > Then it is the same with before. You need to implement `noload_redecls()`, 
> > possibly in Redeclarable.h
> 
> If I understand correctly, we could implement this locally in 
> `ASTReaderDecl.cpp`, matching a number of patterns in 
> `ASTReader::finishPendingActions()`:
> 
> ```diff
> diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
> b/clang/lib/Serialization/ASTReaderDecl.cpp
> index 882d54f31280..ec7c2449d77e 100644
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
>      auto *Def = DD.Definition;
>      DD = std::move(MergeDD);
>      DD.Definition = Def;
> -    for (auto *D : Def->redecls())
> -      cast<CXXRecordDecl>(D)->DefinitionData = &DD;
> +    // FIXME: this duplicates logic from ASTReader::finishPendingActions()...
> +    for (auto *R = Reader.getMostRecentExistingDecl(Def); R;
> +         R = R->getPreviousDecl())
> +      cast<CXXRecordDecl>(R)->DefinitionData = &DD;
>      return;
>    }
>  
> ```
> 
> ... which then of course begs the question why the existing code in 
> `ASTReader::finishPendingActions()` doesn't already take care of it?
> 
> Another possibility is to use `merged_decls()`:
> 
> ```diff
> diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
> b/clang/lib/Serialization/ASTReaderDecl.cpp
> index 882d54f31280..1112c64eeb02 100644
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,8 @@ void ASTDeclMerger::MergeDefinitionData(
>      auto *Def = DD.Definition;
>      DD = std::move(MergeDD);
>      DD.Definition = Def;
> -    for (auto *D : Def->redecls())
> -      cast<CXXRecordDecl>(D)->DefinitionData = &DD;
> +    for (auto *D : merged_redecls(Def))
> +      D->DefinitionData = &DD;
>      return;
>    }
>  
> ```
> 
> Which of these alternatives is preferable (assuming one of them actually 
> works for @alexfh)?

I feel they are workarounds. The `noload_redecls()` new interface may be the 
cleanest solution.

https://github.com/llvm/llvm-project/pull/133057
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits