[modules] Simplify module macro handling in non-local-submodule-visibility mode.

When reaching the end of a module, we used to convert its macros to
ModuleMacros but also leave them in the MacroDirective chain for the
identifier. This meant that every lookup of such a macro would find two
(identical) definitions. It also made it difficult to determine the correct
owner for a macro when reaching the end of a module: the most recent
MacroDirective in the chain could be from an #included submodule rather than
the current module.

Simplify this: whenever we convert a MacroDirective to a ModuleMacro when
leaving a module, clear out the MacroDirective chain for that identifier, and
just rely on the ModuleMacro to provide the macro definition information.

(We don't want to do this for local submodule visibility mode, because in that
mode we maintain a distinct MacroDirective chain for each submodule, and we
need to keep around the prior MacroDirective in case we re-enter the submodule
-- for instance, if its header is #included more than once in a module build,
we need the include guard directive to stick around. But the problem doesn't
arise in this case for the same reason: each submodule has its own
MacroDirective chain, so the macros don't leak out of submodules in the first
place.)


This reinstates r302932, reverted in r302947, with a fix for a bug that
resulted in us sometimes losing macro definitions due to failing to clear out
the overridden module macro list when promoting a directive to a module macro.

llvm-svn: 303468

2 files changed, 35 insertions, 12 deletions

diff --git a/clang/lib/Lex/PPLexerChange.cpp b/clang/lib/Lex/PPLexerChange.cpp
index 1938328..5a589d6 100644
--- a/clang/lib/Lex/PPLexerChange.cpp
+++ b/clang/lib/Lex/PPLexerChange.cpp
@@ -731,7 +731,7 @@ Module *Preprocessor::LeaveSubmodule(bool ForPragma) {
   Module *LeavingMod = Info.M;
   SourceLocation ImportLoc = Info.ImportLoc;
 
-  if (!needModuleMacros() || 
+  if (!needModuleMacros() ||
       (!getLangOpts().ModulesLocalVisibility &&
        LeavingMod->getTopLevelModuleName() != getLangOpts().CurrentModule)) {
     // If we don't need module macros, or this is not a module for which we
@@ -777,17 +777,6 @@ Module *Preprocessor::LeaveSubmodule(bool ForPragma) {
     for (auto *MD = Macro.getLatest(); MD != OldMD; MD = MD->getPrevious()) {
       assert(MD && "broken macro directive chain");
 
-      // Stop on macros defined in other submodules of this module that we
-      // #included along the way. There's no point doing this if we're
-      // tracking local submodule visibility, since there can be no such
-      // directives in our list.
-      if (!getLangOpts().ModulesLocalVisibility) {
-        Module *Mod = getModuleContainingLocation(MD->getLocation());
-        if (Mod != LeavingMod &&
-            Mod->getTopLevelModule() == LeavingMod->getTopLevelModule())
-          break;
-      }
-
       if (auto *VisMD = dyn_cast<VisibilityMacroDirective>(MD)) {
         // The latest visibility directive for a name in a submodule affects
         // all the directives that come before it.
@@ -809,6 +798,13 @@ Module *Preprocessor::LeaveSubmodule(bool ForPragma) {
         if (Def || !Macro.getOverriddenMacros().empty())
           addModuleMacro(LeavingMod, II, Def,
                          Macro.getOverriddenMacros(), IsNew);
+
+        if (!getLangOpts().ModulesLocalVisibility) {
+          // This macro is exposed to the rest of this compilation as a
+          // ModuleMacro; we don't need to track its MacroDirective any more.
+          Macro.setLatest(nullptr);
+          Macro.setOverriddenMacros(*this, {});
+        }
         break;
       }
     }
diff --git a/clang/test/Modules/macro-redefinition.cpp b/clang/test/Modules/macro-redefinition.cpp
new file mode 100644
index 0000000..7b1c7b6
--- /dev/null
+++ b/clang/test/Modules/macro-redefinition.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fmodules -x c++-module-map %s -fmodule-name=__usr_include -verify
+// RUN: %clang_cc1 -fmodules -x c++-module-map %s -fmodule-name=__usr_include -verify -DIMPORT
+
+module __usr_include {
+  module stddef {}
+  module stdlib {}
+}
+
+#pragma clang module contents
+
+// expected-no-diagnostics
+
+#pragma clang module begin __usr_include.stddef
+  #define NULL 0
+#pragma clang module end
+
+#pragma clang module begin __usr_include.stdlib
+  #ifdef IMPORT
+    #pragma clang module import __usr_include.stddef
+  #else
+    #pragma clang module begin __usr_include.stddef
+      #define NULL 0
+    #pragma clang module end
+  #endif
+
+  void *f() { return NULL; } // ok, NULL is visible here
+#pragma clang module end