[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D111543#3067619 <https://reviews.llvm.org/D111543#3067619>, @rsmith wrote:

> In D111543#3066686 <https://reviews.llvm.org/D111543#3066686>, @jansvoboda11 wrote:
>
>> My understanding is that `Preprocessor::getIdentifierInfo` may trigger AST deserialization. The problem here seems to be that we're calling `getIdentifierInfo` before the preprocessor's `IdentifierTable` is set up with "external identifier info lookup" (aka `ASTReader`). So the call to `IdentifierTable::get` creates new `IdentifierInfo` for `"Interface"` instead of looking it up in the AST. Further calls to `getIdentifierInfo` resolve to this new `IdentifierInfo` and the `ObjCInterfaceDecl` never gets deserialized.
>
> Interesting, that could explain what we're seeing. If that's it, then this is an invalidation bug: changing the presence or behavior of the external identifier resolver should trigger invalidation of all existing identifiers so that we will properly query the external source.
>
> But I'm missing something from that explanation: the `IdentifierTable::ExternalLookup` is set in the constructor and never changed from that point onwards,

I was wrong about that. Not sure how I missed the setter :)

Nonetheless: `ASTReader::ReadAST` calls `ASTReader::ReadASTBlock`, which sets the `ASTReader` as the external identifier lookup source. Then, once the recursive deserialization is done, `ASTReader::ReadAST` marks any pre-existing identifiers as out-of-date.

Perhaps the problem is that the identifiers attached to these attributes are somehow being loaded from within `ReadASTBlock`, before they get invalidated? That'd explain what's going wrong. If that's right, then I think this patch would not fully fix the bug: you'd see the same thing if you loaded a module that used the identifier `Interface`, then triggered deserialization of that identifier so it was marked up-to-date, then loaded the problematic module from your testcase. The fix for that would be to ensure that `ReadASTBlock` doesn't deserialize these (or any) identifiers. But I don't see where `ReadASTBlock` is triggering a load of any identifiers either...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111543