PCH + Module: make sure we write out macros associated with builtin identifiers

[manmanren]

Summary

When we import a module that defines a builtin identifier from prefix header and precompile the prefix header, the macro information related to the identifier is lost.
If we don't precompile the prefix header, the source file can still see the macro information.
The reason is that we write out the identifier in the pch but not the macro information since the macro is not defined locally.

This seems to be related to r251565: If we read a builtin identifier from a module that wasn't "interesting" to that module, we will still write it out to a PCH that imports that module.

The fix proposed here is to write exported module macros for PCH as well. I am open to other suggestions.

[bruno]

Hi Manman,

lib/Serialization/ASTWriter.cpp

2191 ↗

(On Diff #57673)

Is this intended?

[manmanren]

Thanks Bruno

lib/Serialization/ASTWriter.cpp

2191 ↗

(On Diff #57673)

I was trying to make sure people get that it is the only thing this patch changes :) I will remove it if we are okay with this approach.

[benlangmuir]

I'd like to see Doug and/or Richard review this. It seems reasonable to me to first blush, but I assume there was a good reason we weren't doing this already...

[manmanren]

Doug and Richard,

Can you take a look at this when you have time?

Thanks,
Manman

[doug.gregor]

Yeah, this looks like the right approach. PCH follows the same rules as modules when it comes to newer information shadowing imported information.

[manmanren]

In D20383#443613, @doug.gregor wrote:

Yeah, this looks like the right approach. PCH follows the same rules as modules when it comes to newer information shadowing imported information.

Hi Doug,

Thanks for reviewing the patch! Can I take that as a "LGTM"? I will clean up the source change.

Manman

[doug.gregor]

Yes, that's a LGTM, sorry for being unclear.

[rsmith]

Looks good to me, too.

cfe/trunk/lib/Serialization/ASTWriter.cpp

for (; MD; MD = MD->getPrevious()) {
         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;
-    if (IsModule) {
-      auto Leafs = PP.getLeafModuleMacros(Name);
-      SmallVector<ModuleMacro*, 8> Worklist(Leafs.begin(), Leafs.end());
-      llvm::DenseMap<ModuleMacro*, unsigned> Visits;
-      while (!Worklist.empty()) {
-        auto *Macro = Worklist.pop_back_val();
+    // We write out exported module macros for PCH as well.
 
-        // Emit a record indicating this submodule exports this macro.
-        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();
   }

cfe/trunk/test/Modules/Inputs/MacroFabs1.h

+
+#undef fabs
+#define fabs(x) (x)
+
+#undef my_fabs
+#define my_fabs(x) (x)

cfe/trunk/test/Modules/Inputs/module.map

module MethodPoolString1 {
   header "MethodPoolString1.h"
 }
 
 module MethodPoolString2 {
   header "MethodPoolString2.h"
 }
 
 module Empty {}
+
+module MacroFabs1 {
+  header "MacroFabs1.h"
+}

cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch

+
+@import MacroFabs1;
+

cfe/trunk/test/Modules/pch-module-macro.m

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -emit-pch -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -o %t.pch -I %S/Inputs -x objective-c-header %S/Inputs/pch-import-module-with-macro.pch
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify
+// expected-no-diagnostics
+
+int test(int x) {
+  return my_fabs(x) + fabs(x);
+}
+