[C++20][Modules][HU 3/5] Emit module macros for header units.

[iains]

Summary

For header units we build the top level module directly from the header
that it represents and macros defined in this TU need to be emitted (when
such a definition is live at the end of the TU).

[iains]

updated to remove a single-use variable.

Having some difficulty in reproducing the CI fails outside the harness, however the
code setting the removed variable could have faulted if there was no macro data for
the identifier. Set the IsHeaderUnit variable explicitly.

[iains]

rebased.

[ChuanqiXu]

Maybe it's worth to add a test from: http://eel.is/c++draft/cpp.import#8

clang/include/clang/Serialization/ASTWriter.h

127–128

I think the member is redundant. I thought we could use WritingModule->isHeaderUnit() to replace it.

clang/lib/Serialization/ASTWriter.cpp

2287–2288

We could replace the use of Mod with WritingModule too. (If I don't misread the code)

2358–2378

Is it possible to merge the implementation with the following for PCH? It looks like there are some redundancies.

2360–2362

How about to add comments to show the different logic of emitting macros for header unit and PCH? I guess other readers might be confusing too.

[iains]

address review comments, match windows path separators in tests.

[iains]

clang/include/clang/Serialization/ASTWriter.h

127–128

yeah we can do this - I added a "isHeaderUnit()" method to Module.h to do this.

clang/lib/Serialization/ASTWriter.cpp

2358–2378

Well, that was what I had originally, I actually split it out as it is now because the difference in the logic around which macros are written out was making the code pretty confusing to read. If there's a strong feeling about this, perhaps we can see if there's some way to factor it (perhaps with some place-holder vars).

[ChuanqiXu]

I feel good if we could add the test from: http://eel.is/c++draft/cpp.import#8.

clang/include/clang/Serialization/ASTWriter.h

21

This is redundant too.

clang/lib/Serialization/ASTWriter.cpp

2358–2378

I agree with you. I tried to refactor it myself and find things didn't get better.

[iains]

In D121097#3391236, @ChuanqiXu wrote:

I feel good if we could add the test from: http://eel.is/c++draft/cpp.import#8.

I agree we should have tests based on all the relevant examples in the standard.

However, that specific example is not connected with building header units, but instead is connected with consuming various preprocessor directives etc. when building a regular module. We actually already have some of the tests in diagnosing bad imports.

So I think that (after this series is in) we should be in a position to add some more of the examples in the standard - as a follow-on patch, does that make sense?

[ChuanqiXu]

In D121097#3391698, @iains wrote:

In D121097#3391236, @ChuanqiXu wrote:

I feel good if we could add the test from: http://eel.is/c++draft/cpp.import#8.

I agree we should have tests based on all the relevant examples in the standard.

However, that specific example is not connected with building header units, but instead is connected with consuming various preprocessor directives etc. when building a regular module. We actually already have some of the tests in diagnosing bad imports.

So I think that (after this series is in) we should be in a position to add some more of the examples in the standard - as a follow-on patch, does that make sense?

Yeah, it makes sense. This one LGTM.

[iains]

rebased.

[iains]

rebased

[hvdijk]

clang/test/Modules/cxx20-hu-04.cpp

23

On Windows, when the path starts with C:\Users\..., I am seeing

error: 'warning' diagnostics seen but not expected: 

  Line 1: \U used with no following hex digits; treating as '\' followed by identifier

1 error generated.

Looking at this test, TDIR is only used as a FileCheck variable, can it just be removed from this line?

[iains]

clang/test/Modules/cxx20-hu-04.cpp

23

Sorry about that, Yes, I think that change should be fine - I will take care of it,

[iains]

applied [C++20][Modules] Fix a testcase warning on Windows [NFC].
https://github.com/llvm/llvm-project/commit/1f0b8ba47ab0f1dc678099d4830d0cc0d10850b6
should be fixed now.

clang/include/clang/Serialization/ASTWriter.h

 
 #ifndef LLVM_CLANG_SERIALIZATION_ASTWRITER_H
 #define LLVM_CLANG_SERIALIZATION_ASTWRITER_H
 
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaConsumer.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTDeserializationListener.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
private:
   Preprocessor *PP = nullptr;
 
   /// The reader of existing AST files, if we're chaining.
   ASTReader *Chain = nullptr;
 
   /// The module we're currently writing, if any.
   Module *WritingModule = nullptr;
 
+  /// The module is a header unit.
+  bool IsHeaderUnit = false;
+
   /// The offset of the first bit inside the AST_BLOCK.
   uint64_t ASTBlockStartOffset = 0;
 
   /// The range representing all the AST_BLOCK.
   std::pair<uint64_t, uint64_t> ASTBlockRange;
 
   /// The base directory for any relative paths we emit.
   std::string BaseDirectory;
private:
   static std::pair<ASTFileSignature, ASTFileSignature>
   createSignature(StringRef AllBytes, StringRef ASTBlockBytes);
 
   void WriteInputFiles(SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
                        std::set<const FileEntry *> &AffectingModuleMaps);
   void WriteSourceManagerBlock(SourceManager &SourceMgr,
                                const Preprocessor &PP);
   void writeIncludedFiles(raw_ostream &Out, const Preprocessor &PP);
-  void WritePreprocessor(const Preprocessor &PP, bool IsModule);
+  void WritePreprocessor(const Preprocessor &PP, bool IsModule, Module *Mod);
   void WriteHeaderSearch(const HeaderSearch &HS);
   void WritePreprocessorDetail(PreprocessingRecord &PPRec,
                                uint64_t MacroOffsetsBase);
   void WriteSubmodules(Module *WritingModule);
 
   void WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag,
                                      bool isModule);
 

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::writeIncludedFiles(raw_ostream &Out, const Preprocessor &PP) {
   endian::Writer LE(Out, little);
   LE.write<uint32_t>(IncludedInputFileIDs.size());
   for (uint32_t ID : IncludedInputFileIDs)
     LE.write<uint32_t>(ID);
 }
 
 /// Writes the block containing the serialized form of the
 /// preprocessor.
-void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
+void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule,
+                                  Module *Mod) {
   uint64_t MacroOffsetsBase = Stream.GetCurrentBitNo();
 
   PreprocessingRecord *PPRec = PP.getPreprocessingRecord();
   if (PPRec)
     WritePreprocessorDetail(*PPRec, MacroOffsetsBase);
 
   RecordData Record;
   RecordData ModuleMacroRecord;
void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule,
 
   // Emit the macro directives as a list and associate the offset with the
   // identifier they belong to.
   for (const IdentifierInfo *Name : MacroIdentifiers) {
     MacroDirective *MD = PP.getLocalMacroDirectiveHistory(Name);
     uint64_t StartOffset = Stream.GetCurrentBitNo() - MacroOffsetsBase;
     assert((StartOffset >> 32) == 0 && "Macro identifiers offset too large");
 
+    // Write out any exported module macros.
+    bool EmittedModuleMacros = false;
+    if (IsHeaderUnit) {
+      // This is for the main TU when it is a C++20 header unit.
+      // We preserve the final state of defined macros, and we do not emit ones
+      // that are undefined.
+      if (!MD || shouldIgnoreMacro(MD, IsModule, PP) ||
+          MD->getKind() == MacroDirective::MD_Undefine)
+        continue;
+      AddSourceLocation(MD->getLocation(), Record);
+      Record.push_back(MD->getKind());
+      if (auto *DefMD = dyn_cast<DefMacroDirective>(MD)) {
+        Record.push_back(getMacroRef(DefMD->getInfo(), Name));
+      } else if (auto *VisMD = dyn_cast<VisibilityMacroDirective>(MD)) {
+        Record.push_back(VisMD->isPublic());
+      }
+      ModuleMacroRecord.push_back(getSubmoduleID(Mod));
+      ModuleMacroRecord.push_back(getMacroRef(MD->getMacroInfo(), Name));
+      Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
+      ModuleMacroRecord.clear();
+      EmittedModuleMacros = true;
+    } else {
       // Emit the macro directives in reverse source order.
       for (; MD; MD = MD->getPrevious()) {
         // Once we hit an ignored macro, we're done: the rest of the chain
         // will all be ignored macros.
         if (shouldIgnoreMacro(MD, IsModule, PP))
           break;
-
         AddSourceLocation(MD->getLocation(), Record);
         Record.push_back(MD->getKind());
         if (auto *DefMD = dyn_cast<DefMacroDirective>(MD)) {
           Record.push_back(getMacroRef(DefMD->getInfo(), Name));
         } else if (auto *VisMD = dyn_cast<VisibilityMacroDirective>(MD)) {
           Record.push_back(VisMD->isPublic());
         }
       }
 
-    // Write out any exported module macros.
-    bool EmittedModuleMacros = false;
       // We write out exported module macros for PCH as well.
       auto Leafs = PP.getLeafModuleMacros(Name);
-    SmallVector<ModuleMacro*, 8> Worklist(Leafs.begin(), Leafs.end());
-    llvm::DenseMap<ModuleMacro*, unsigned> Visits;
+      SmallVector<ModuleMacro *, 8> Worklist(Leafs.begin(), Leafs.end());
+      llvm::DenseMap<ModuleMacro *, unsigned> Visits;
       while (!Worklist.empty()) {
         auto *Macro = Worklist.pop_back_val();
 
         // Emit a record indicating this submodule exports this macro.
-      ModuleMacroRecord.push_back(
-          getSubmoduleID(Macro->getOwningModule()));
+        ModuleMacroRecord.push_back(getSubmoduleID(Macro->getOwningModule()));
         ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name));
         for (auto *M : Macro->overrides())
           ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule()));
 
         Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
         ModuleMacroRecord.clear();
 
         // Enqueue overridden macros once we've visited all their ancestors.
         for (auto *M : Macro->overrides())
           if (++Visits[M] == M->getNumOverridingMacros())
             Worklist.push_back(M);
 
         EmittedModuleMacros = true;
       }
-
+    }
     if (Record.empty() && !EmittedModuleMacros)
       continue;
 
     IdentMacroDirectivesOffsetMap[Name] = StartOffset;
     Stream.EmitRecord(PP_MACRO_DIRECTIVE_HISTORY, Record);
     Record.clear();
   }
 

 }
 
 ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
                                          const std::string &OutputFile,
                                          Module *WritingModule) {
   using namespace llvm;
 
   bool isModule = WritingModule != nullptr;
+  IsHeaderUnit = isModule && WritingModule->Kind == Module::ModuleHeaderUnit;
 
   // Make sure that the AST reader knows to finalize itself.
   if (Chain)
     Chain->finalizeForWriting();
 
   ASTContext &Context = SemaRef.Context;
   Preprocessor &PP = SemaRef.PP;
 
ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
 
   // These things can only be done once we've written out decls and types.
   WriteTypeDeclOffsets();
   if (!DeclUpdatesOffsetsRecord.empty())
     Stream.EmitRecord(DECL_UPDATE_OFFSETS, DeclUpdatesOffsetsRecord);
   WriteFileDeclIDsMap();
   WriteSourceManagerBlock(Context.getSourceManager(), PP);
   WriteComments();
-  WritePreprocessor(PP, isModule);
+  WritePreprocessor(PP, isModule, WritingModule);
   WriteHeaderSearch(PP.getHeaderSearchInfo());
   WriteSelectors(SemaRef);
   WriteReferencedSelectorsPool(SemaRef);
   WriteLateParsedTemplates(SemaRef);
   WriteIdentifierTable(PP, SemaRef.IdResolver, isModule);
   WriteFPPragmaOptions(SemaRef.CurFPFeatureOverrides());
   WriteOpenCLExtensions(SemaRef);
   WriteCUDAPragmas(SemaRef);

clang/test/Modules/cxx20-hu-04.cpp

+// Test macro preservation in C++20 Header Units.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header hu-01.h \
+// RUN: -o hu-01.pcm
+
+// RUN: %clang_cc1 -std=c++20 -module-file-info hu-01.pcm | \
+// RUN: FileCheck --check-prefix=CHECK-HU %s -DTDIR=%t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header hu-02.h \
+// RUN: -DFOO -fmodule-file=hu-01.pcm -o hu-02.pcm  -Rmodule-import 2>&1 | \
+// RUN: FileCheck --check-prefix=CHECK-IMP %s -DTDIR=%t
+
+// RUN: %clang_cc1 -std=c++20 -module-file-info hu-02.pcm | \
+// RUN: FileCheck --check-prefix=CHECK-HU2 %s -DTDIR=%t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface importer-01.cpp \
+// RUN:  -fmodule-file=hu-02.pcm -o B.pcm -DTDIR=%t -verify
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface importer-02.cpp \
+// RUN:  -fmodule-file=hu-02.pcm -o C.pcm -DTDIR=%t -Rmodule-import 2>&1 | \
+// RUN:  FileCheck --check-prefix=CHECK-IMP-HU2 %s -DTDIR=%t
+
+//--- hu-01.h
+#ifndef __GUARD
+#define __GUARD
+
+int baz(int);
+#define FORTYTWO 42
+
+#define SHOULD_NOT_BE_DEFINED -1
+#undef SHOULD_NOT_BE_DEFINED
+
+#endif // __GUARD
+// expected-no-diagnostics
+
+// CHECK-HU:  ====== C++20 Module structure ======
+// CHECK-HU-NEXT:  Header Unit './hu-01.h' is the Primary Module at index #1
+
+//--- hu-02.h
+export import "hu-01.h";
+#if !defined(FORTYTWO) || FORTYTWO != 42
+#error FORTYTWO missing in hu-02
+#endif
+
+#ifndef __GUARD
+#error __GUARD missing in hu-02
+#endif
+
+#ifdef SHOULD_NOT_BE_DEFINED
+#error SHOULD_NOT_BE_DEFINED is visible
+#endif
+
+#define KAP 6174
+
+#ifdef FOO
+#define FOO_BRANCH(X) (X) + 1
+inline int foo(int x) {
+  if (x == FORTYTWO)
+    return FOO_BRANCH(x);
+  return FORTYTWO;
+}
+#else
+#define BAR_BRANCH(X) (X) + 2
+inline int bar(int x) {
+  if (x == FORTYTWO)
+    return BAR_BRANCH(x);
+  return FORTYTWO;
+}
+#endif
+
+// CHECK-IMP: remark: importing module './hu-01.h' from 'hu-01.pcm'
+// CHECK-HU2:  ====== C++20 Module structure ======
+// CHECK-HU2-NEXT:  Header Unit './hu-02.h' is the Primary Module at index #2
+// CHECK-HU2-NEXT:   Exports:
+// CHECK-HU2-NEXT:    Header Unit './hu-01.h' is at index #1
+// expected-no-diagnostics
+
+//--- importer-01.cpp
+export module B;
+import "hu-02.h";
+
+int success(int x) {
+  return foo(FORTYTWO + x + KAP);
+}
+
+int fail(int x) {
+  return bar(FORTYTWO + x + KAP); // expected-error {{use of undeclared identifier 'bar'}}
+  // expected-note@* {{'baz' declared here}}
+}
+
+//--- importer-02.cpp
+export module C;
+import "hu-02.h";
+
+int success(int x) {
+  return foo(FORTYTWO + x + KAP);
+}
+
+// CHECK-IMP-HU2: remark: importing module './hu-02.h' from 'hu-02.pcm'
+// CHECK-IMP-HU2: remark: importing module './hu-01.h' into './hu-02.h' from '[[TDIR]]/hu-01.pcm'