Add keydecls to modules instead of just expanding them.

[Sterling-Augustine]

pr/171769 added a nice optimization for namespace lookups, but has a bug where if a key decl does not also appear in the chain, it will be omitted from the module. Fix that.

This may not be exactly the correct fix, but it does resolve the issue described in pr/171769. Another possible fix is to ensure that the key decl always appears in the chain. But I suspect what is happening is that it isn't written out the first time the ast file is written.

I still don't have a good reduced test case for this--I actually know almost nothing at all about Clang modules and KeyDecls. Perhaps someone more familiar with how it all works can propose one.

The symptom this fixes is below. The description isn't enough to reproduce the problem, unfortunately, and I haven't been able to reduce it beyond several hundred files.

header_b.h:

#include "header_a.h" // Declares namespace_a, and various things inside it. 
namespace namespace_b = namespace_a;

and the error is:

<header_b.h>:13:34: error: declaration of 'namespace_a' must be imported from module 'module_c' before it is required
   13 | namespace namespace_b = ::<namespace_a>;
      |                                  ^
1 error generated.

Module_c declares more things in namespace_a, as do many other modules.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: None (Sterling-Augustine)

Changes

pr/171769 added a nice optimization for namespace lookups, but has a bug where if a key decl does not also appear in the chain, it will be omitted from the module. Fix that.

This may not be exactly the correct fix, but it does resolve the issue described in pr/171769. Another possible fix is to ensure that the key decl always appears in the chain. But I suspect what is happening is that it isn't written out the first time the ast file is written.

I still don't have a good reduced test case for this--I actually know almost nothing at all about Clang modules and KeyDecls. Perhaps someone more familiar with how it all works can propose one.

The symptom this fixes is below. The description isn't enough to reproduce the problem, unfortunately, and I haven't been able to reduce it beyond several hundred files.

header_b.h:

#include "header_a.h" // Declares namespace_a, and various things inside it. 
namespace namespace_b = namespace_a;

and the error is:

<header_b.h>:13:34: error: declaration of 'namespace_a' must be imported from module 'module_c' before it is required
   13 | namespace namespace_b = ::<namespace_a>;
      |                                  ^
1 error generated.

Module_c declares more things in namespace_a, as do many other modules.

---

Full diff: https://github.com/llvm/llvm-project/pull/172898.diff

1 Files Affected:

• (modified) clang/lib/Serialization/ASTWriter.cpp (+1-2)

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 234615522e773..4154584cc30e2 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4444,6 +4444,7 @@ class ASTDeclContextNameLookupTrait
     };
     ASTReader *Chain = Writer.getChain();
     for (NamedDecl *D : Decls) {
+      AddDecl(D);
       if (Chain && isa<NamespaceDecl>(D) && D->isFromASTFile() &&
           D == Chain->getKeyDeclaration(D)) {
         // In ASTReader, we stored only the key declaration of a namespace decl
@@ -4456,8 +4457,6 @@ class ASTDeclContextNameLookupTrait
         Chain->forEachImportedKeyDecl(D, [&AddDecl](const Decl *D) {
           AddDecl(cast<NamedDecl>(const_cast<Decl *>(D)));
         });
-      } else {
-        AddDecl(D);
       }
     }
     return std::make_pair(Start, DeclIDs.size());

[mpark]

Hi @Sterling-Augustine, thanks for this! Curious, are you able to share the several hundred line repro you do have? Maybe with some obfuscating?

[Sterling-Augustine]

Hi @Sterling-Augustine, thanks for this! Curious, are you able to share the several hundred line repro you do have? Maybe with some obfuscating?

Alas, it is hundreds of files still, not hundreds of lines. And many are confidential.

[mpark]

Hi @Sterling-Augustine, thanks for this! Curious, are you able to share the several hundred line repro you do have? Maybe with some obfuscating?

Alas, it is hundreds of files still, not hundreds of lines. And many are confidential.

Ohh I see. I misread that. Okay, thanks!

[ChuanqiXu9]

Except what the patch is, I really hope we can have a test. It is really important for the maintainability.

[ChuanqiXu9]

The fix looks odd to me. Since I think the following

Chain->forEachImportedKeyDecl(D, [&AddDecl](const Decl *D) {
          AddDecl(cast<NamedDecl>(const_cast<Decl *>(D)));
        });

will always add decl for D itself.

[mpark]

Yeah, this is what I'm confused about as well 🤔

[mpark]

@Sterling-Augustine I understand you cannot share the repro you have, but would you be able to insert some code here like:

for (NamedDecl *D : Decls) {
  if (...) {
    bool Found = false;
    Chain->forEachImportedKeyDecl(D, [&AddDecl, &Found](const Decl *D2) {
      AddDecl(cast<NamedDecl>(const_cast<Decl *>(D2)));
      if(D == D2) Found = true;
    });
    if (!Found) {
      D->dump();
      D->getCanonicalDecl()->dump();
    }
  } else {
    AddDecl(D);
  }
}

and let us know what that prints?

[Sterling-Augustine]

After more testing, I don't think this fixes it, so closing as not useful while I work on further reductions.

[Sterling-Augustine]

The original PR now has a testcase created by my colleague that demonstrates the problem.

[mpark]

The original PR now has a testcase created by my colleague that demonstrates the problem.

This is great. Thank you!