[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#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, so I don't think it's the case that the external lookup hasn't been set yet at the point where we create the identifier. And `ASTReader::ReadAST` invalidates all existing identifiers when we load a new AST file (search for the calls to `setOutOfDate(true)`), so if the lookup results for `Interface` change we should invalidate the identifier properly. Given the kinds of entities affected by this, I wonder if there is something incorrect in the way that `ObjCInterfaceDecl`s are handled or some Objective-C-specific name lookup issue? (ObjC lookups certainly have some modules-specific bugs due to be implemented as raw `DeclContext::lookup` calls instead of using `Sema`'s name lookup mechanism, but I think that only applies to qualified lookups.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111543