[Modules] Teach Clang to survive ambiguous macros which come from system headers even if they arrived when merging non-system modules.

[chandlerc]

Summary

The idea of this code is that we don't want to warn the user about
macros defined multiple times by their system headers with slightly
different definitions. We should have this behavior if either the
macro comes from a system module, or the definition within the module
comes from a system header. Previously, we would warn on ambiguous
macros being merged when they came from a users modules even though they
only showed up via system headers.

By surviving this we can handle common system header macro differences
like differing 'const' qualification of pointers due to some headers
predating 'const' being valid in C code, even when those systems headers
are pre-built into a system module.

[chandlerc]

I should have mentioned, I'll simplify the test case to something more precise prior to committing now that I know what the nature of the bug is.

I'm mostly interested in whether you agree with my reasoning about the fix.

[rsmith]

OK, makes sense.

lib/Serialization/ASTReader.cpp

 /// \brief For the given macro definitions, check if they are both in system
 /// modules.
 static bool areDefinedInSystemModules(MacroInfo *PrevMI, MacroInfo *NewMI,
                                       Module *NewOwner, ASTReader &Reader) {
   assert(PrevMI && NewMI);
   Module *PrevOwner = nullptr;
   if (SubmoduleID PrevModID = PrevMI->getOwningModuleID())
     PrevOwner = Reader.getSubmodule(PrevModID);
-  SourceManager &SrcMgr = Reader.getSourceManager();
-  bool PrevInSystem
-    = PrevOwner? PrevOwner->IsSystem
-               : SrcMgr.isInSystemHeader(PrevMI->getDefinitionLoc());
-  bool NewInSystem
-    = NewOwner? NewOwner->IsSystem
-              : SrcMgr.isInSystemHeader(NewMI->getDefinitionLoc());
   if (PrevOwner && PrevOwner == NewOwner)
     return false;
+  SourceManager &SrcMgr = Reader.getSourceManager();
+  bool PrevInSystem = (PrevOwner && PrevOwner->IsSystem) ||
+                      SrcMgr.isInSystemHeader(PrevMI->getDefinitionLoc());
+  bool NewInSystem = (NewOwner && NewOwner->IsSystem) ||
+                     SrcMgr.isInSystemHeader(NewMI->getDefinitionLoc());
   return PrevInSystem && NewInSystem;
 }
 
 void ASTReader::removeOverriddenMacros(IdentifierInfo *II,
                                        SourceLocation ImportLoc,
                                        AmbiguousMacros &Ambig,
                                        ArrayRef<SubmoduleID> Overrides) {
   for (unsigned OI = 0, ON = Overrides.size(); OI != ON; ++OI) {

test/Modules/Inputs/macro-ambiguity/a/foo.h

+#ifndef A_FOO_H
+#define A_FOO_H
+
+#include_next <foo.h>
+
+#endif // FOO_H

test/Modules/Inputs/macro-ambiguity/b/foo.h

+#ifndef B_FOO_H
+#define B_FOO_H
+
+#include <bar.h>
+
+#endif // B_FOO_H

test/Modules/Inputs/macro-ambiguity/c/bar.h

+#ifndef C_BAR_H
+#define C_BAR_H
+
+#define FOO(x) x + x
+
+#define MORE_META(x) (x)
+
+#define META_FOO(x) MORE_META(FOO(x))
+
+#endif

test/Modules/Inputs/macro-ambiguity/c/d/baz.h

+#ifndef C_D_BAZ_H
+#define C_D_BAZ_H
+
+#define FOO(x) 2 * x
+
+#endif // C_D_BAZ_H

test/Modules/Inputs/macro-ambiguity/module.modulemap

+module a { header "Inputs/macro-ambiguity/a/foo.h" export * use bc }
+module bc [system] {
+  textual header "Inputs/macro-ambiguity/b/foo.h"
+  textual header "Inputs/macro-ambiguity/c/bar.h"
+  textual header "Inputs/macro-ambiguity/c/d/baz.h"
+}

test/Modules/macro-ambiguity.cpp

+// RUN: rm -rf %t
+// RUN: cd %S
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -v -iquote . \
+// RUN:   -isystem Inputs/macro-ambiguity/a \
+// RUN:   -isystem Inputs/macro-ambiguity/b \
+// RUN:   -isystem Inputs/macro-ambiguity/c \
+// RUN:   -fno-modules-implicit-maps -fmodule-map-file-home-is-cwd \
+// RUN:   -emit-module -fmodule-name=a -o %t/a.pcm \
+// RUN:   Inputs/macro-ambiguity/module.modulemap
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -v -iquote . \
+// RUN:   -isystem Inputs/macro-ambiguity/a \
+// RUN:   -isystem Inputs/macro-ambiguity/b \
+// RUN:   -isystem Inputs/macro-ambiguity/c \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/macro-ambiguity/module.modulemap \
+// RUN:   -fmodule-file=%t/a.pcm \
+// RUN:   -Wambiguous-macro -verify macro-ambiguity.cpp
+
+// FIXME: We should add tests for cases which *will* issue the diagnostic as
+// well.
+// expected-no-diagnostics
+
+#include <d/baz.h>
+#include <foo.h>
+
+int test(int x) {
+  int foo = META_FOO(x) + FOO(x);
+
+  return foo;
+}