[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

128–129

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

clang/lib/Serialization/ASTWriter.cpp

2286–2287

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

2356–2376

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

2358–2360

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

128–129

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

clang/lib/Serialization/ASTWriter.cpp

2356–2376

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

2356–2376

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

22

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

22

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/Basic/Module.h

public:
   }
 
   /// Is this a module partition.
   bool isModulePartition() const {
     return Kind == ModulePartitionInterface ||
            Kind == ModulePartitionImplementation;
   }
 
+  /// Is this module a header unit.
+  bool isHeaderUnit() const { return Kind == ModuleHeaderUnit; }
+
   /// Get the primary module interface name from a partition.
   StringRef getPrimaryModuleInterfaceName() const {
     if (isModulePartition()) {
       auto pos = Name.find(':');
       return StringRef(Name.data(), pos);
     }
     return Name;
   }

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/Module.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"
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 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;
 
   /// Indicates whether timestamps should be written to the produced

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) {
   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;
+    // C+=20 Header Units are compiled module interfaces, but they preserve
+    // macros that are live (i.e. have a defined value) at the end of the
+    // compilation.  So when writing a header unit, we preserve only the final
+    // value of each macro (and discard any that are undefined).  Header units
+    // do not have sub-modules (although they might import other header units).
+    // PCH files, conversely, retain the history of each macro's define/undef
+    // and of leaf macros in sub modules.
+    if (IsModule && WritingModule->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(WritingModule));
+      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();
   }
 

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'