Improve redefinition errors pointing to the same header.

[bruno]

Summary

Diagnostics related to redefinition errors that point to the same header file do not provide much information that helps fixing the issue. In modules context it usually happens because of a non modular include, in non-module context it might happen because of the lack of header guardas. This patch tries to improve this situation by enhancing diagnostics in this particular scenario. If the approach seems reasonable, I can extend it to other relevant redefinition_error diagnostic call sites.

Modules

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;

^

In module 'X' imported from x.c:1:
Inputs4/C.h:3:5: note: previous definition is here
int c = 1;

^

1 warning and 1 error generated.

After this patch

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;

^

In module 'X' imported from x.c:1:
Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional (likely non-modular) include site in module 'X.B'
#include "C.h"

^

1 error generated.
Inputs4/module.modulemap:6:10: note: consider adding 'Inputs4/C.h' as part of 'X.B' definition in

module B {
       ^

1 error generated.

Without Modules

In file included from x.c:2:
./a.h:1:5: error: redefinition of 'yyy'
int yyy = 42;

^

./a.h:1:5: note: previous definition is here
int yyy = 42;

^

1 error generated.

After this patch

In file included from x.c:2:
./a.h:1:5: error: redefinition of 'yyy'
int yyy = 42;

^

x.c:1:10: note: './a.h' included multiple times, consider augmenting this header with #ifdef guards
#include "a.h"

^

1 error generated.

[v.g.vassilev]

This looks good to me.

[bruno]

Thanks Vassil.

@rsmith is this ok for you?

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

8708 ↗

(On Diff #84759)

If we can't be sure whether or not the other include side was a modular include, making a guess is probably not helpful. (In fact, I've seen this issue show up a lot more where the header is a modular header in both modules.)

8710 ↗

(On Diff #84759)

This note ends in the middle of a sentence.

lib/Sema/SemaDecl.cpp

3731 ↗

(On Diff #84759)

Can you give this a name that suggests that it produces a note rather than a full diagnostic? notePreviousDefinition, maybe.

3747 ↗

(On Diff #84759)

Rather than listing where the module was first imported (which could be unrelated to the problem), it would make more sense to list the location at which the previous declaration was made visible. You can get that by querying the VisibleModuleSet for the owning module of the definition and every other module in its merged definitions set (see Sema::hasVisibleMergedDefinition).

3755–3759 ↗

(On Diff #84759)

Is this ("add the header to the module map for some other module that happened to include it first") really generally good advice? People have a tendency to follow such guidance blindly.

3763 ↗

(On Diff #84759)

I don't think this should be an else. If both locations were textually included in the current translation, we should still produce the "missing include guard" diagnostic. Conversely, it'd seem reasonable to ask the preprocessor if the header has an include guard before claiming that it doesn't.

[v.g.vassilev]

Here is an example that we recently had where we would be very happy to see better diagnostics:

MessageTypes.h:

#ifndef ROOT_MessageTypes
#define ROOT_MessageTypes

enum EMessageTypes {
   kROOTD_OPEN           = 2004,         //filename follows + mode
};

#endif

module.modulemap:

module "libNet.so" {
  requires cplusplus
  module "TMessage.h" { header "TMessage.h" export * }
  module "TNetFile.h" { header "TNetFile.h" export * }
  export *
}

Output:

TNetFile.cxx:1:10: remark: building module 'libNet.so' as '/home/teemperor/Downloads/merge_enum/pcms/3MNDY7KURXTXS/libNet.so-141H35WZHCMDE.pcm' [-Rmodule-build]
#include "TNetFile.h"
         ^
TNetFile.cxx:1:10: remark: finished building module 'libNet.so' [-Rmodule-build]
TNetFile.cxx:2:10: error: declaration of 'kROOTD_OPEN' must be imported from module 'libNet.so.TMessage.h' before it is required
auto v = kROOTD_OPEN;
         ^
In module 'libNet.so' imported from TNetFile.cxx:1:
./MessageTypes.h:5:4: note: previous declaration is here
   kROOTD_OPEN           = 2004,         //filename follows + mode
   ^
1 error generated.

test.sh:

#!/bin/bash

CLANG="/opt/clang/inst/bin/clang"

rm -rf ./pcms/
# Normal clang invocation
#"$CLANG" -cc1 -triple x86_64-apple-macosx10.11.0 -fsyntax-only -I . -std=c++11 -x c++ TNetFile.cxx -nostdsysteminc
# Modules clang invocation
"$CLANG" -cc1 -triple x86_64-apple-macosx10.11.0 -fsyntax-only -I . -std=c++11  -fmodules  -fimplicit-module-maps -fmodules-cache-path=./pcms/ -fcolor-diagnostics -fdiagnostics-show-option -fdiagnostics-show-note-include-stack -x c++ TNetFile.cxx -nostdsysteminc -Rmodule-build

TMessage.h:

#ifndef ROOT_TMessage
#define ROOT_TMessage
#include "MessageTypes.h"
#endif

TNetFile.cxx:

#include "TNetFile.h"
auto v = kROOTD_OPEN;

TNetFile.h:

#ifndef ROOT_TNetFile
#define ROOT_TNetFile
#include "MessageTypes.h"
#endif

[bruno]

include/clang/Basic/DiagnosticSemaKinds.td

8708 ↗

(On Diff #84759)

After thinking more about this I agree: if used together with "-Wnon-modular-include-in-module" and similars, it should become more evident to the user what this is about.

lib/Sema/SemaDecl.cpp

3747 ↗

(On Diff #84759)

I tried this, but AFAIU the Decls coming from non-modular headers are usually hidden, and since it has not been merged, which happens in the testcase because it's a var redefinition error, then we have no import location to get information from. Do you have a synthetic testcase in mind that I can use? I'm happy to follow up with that (here or in a next patch).

3755–3759 ↗

(On Diff #84759)

Indeed, going to remove that.

[bruno]

Updated the patch after Richard's comments:

Before

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;
    ^
In module 'X' imported from x.c:1:
Inputs4/C.h:3:5: note: previous definition is here
int c = 1;
    ^
1 error generated.

After

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;
    ^
In module 'X' imported from x.c:1:
Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional include site in header from module 'X.B'
#include "C.h"
         ^
Inputs4/module.modulemap:6:10: note: X.B defined here
  module B {
         ^
x.c:2:10: note: 'Inputs4/C.h' included multiple times, additional include site here
#include "C.h" // textual include
         ^
1 error generated.

[bruno]

@v.g.vassilev your testcase is interesting but it's unrelated to this specific improvement. The error message it's triggered by (a) the decl name not being found because kROOTD_OPEN is hidden, (b) type transforms looks for an alternative (c) Sema::diagnoseMissingImport is called and produces the error you're seeing. I can fix this one too once we decide what we consider a good diagnostic here, let's iterate in http://bugs.llvm.org/show_bug.cgi?id=32670

[v.g.vassilev]

@bruno, ok, sounds good.

[bruno]

Ping

[rsmith]

LGTM. I'd like to make sure we try to use something better than the first import location for a module (that location is especially meaningless under -fmodules-local-submodule-visibility), but I'm happy for that (the big comment) to be dealt with as a follow-on change, and all the other comments here are minor, so feel free to commit after addressing those.

include/clang/Basic/DiagnosticSemaKinds.td

4584 ↗

(On Diff #95369)

Nit: we generally put the Note< on the prior line.

8710 ↗

(On Diff #84759)

... this note still ends in the middle of a sentence.

lib/Sema/SemaDecl.cpp

3594 ↗

(On Diff #95369)

Reindent.

3812 ↗

(On Diff #95369)

Use isValid to avoid double-negative.

3747 ↗

(On Diff #84759)

The int c = 1; testcase seems like it ought to be pretty rare (defining a variable in a header), and it might make more sense to say "hey, you probably didn't want a strong definition in a header at all" rather than pointing out the header was included twice.

Here's one pattern we've seen a few times that might be useful as a testcase:

foo.h:

#ifndef FOO
#define FOO
#include "bar.h"
struct A {};
#endif

bar.h:

#ifndef BAR
#define BAR
#include "foo.h"
#endif

module.map:

module bar { header "bar.h" }

x.c:

#include "foo.h"

[This results in us entering the FOO include guard, importing bar.h (including a definition of A) and then parsing another definition of A.]

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def err_redefinition_extern_inline : Error<
   "%select{C99 mode|C++}1">;
 def warn_attr_abi_tag_namespace : Warning<
   "'abi_tag' attribute on %select{non-inline|anonymous}0 namespace ignored">,
   InGroup<IgnoredAttributes>;
 def err_abi_tag_on_redeclaration : Error<
   "cannot add 'abi_tag' attribute in a redeclaration">;
 def err_new_abi_tag_on_redeclaration : Error<
   "'abi_tag' %0 missing in original declaration">;
+def note_use_ifdef_guards : Note<
+  "unguarded header; consider using #ifdef guards or #pragma once">;
 
 def note_deleted_dtor_no_operator_delete : Note<
   "virtual destructor requires an unambiguous, accessible 'operator delete'">;
 def note_deleted_special_member_class_subobject : Note<
   "%select{default constructor of|copy constructor of|move constructor of|"
   "copy assignment operator of|move assignment operator of|destructor of|"
   "constructor inherited by}0 "
   "%1 is implicitly deleted because "
def err_export_not_in_module_interface : Error<
   "export declaration can only be used within a module interface unit after "
   "the module declaration">;
 
 def ext_equivalent_internal_linkage_decl_in_modules : ExtWarn<
   "ambiguous use of internal linkage declaration %0 defined in multiple modules">,
   InGroup<DiagGroup<"modules-ambiguous-internal-linkage">>;
 def note_equivalent_internal_linkage_decl : Note<
   "declared here%select{ in module '%1'|}0">;
+
+def note_redefinition_modules_same_file : Note<
+  "'%0' included multiple times, additional include site in header from module '%1'">;
+def note_redefinition_modules_same_file_modulemap : Note<
+  "consider adding '%0' as part of '%1' definition">;
+def note_redefinition_include_same_file : Note<
+  "'%0' included multiple times, additional include site here">;
 }
 
 let CategoryName = "Coroutines Issue" in {
 def err_return_in_coroutine : Error<
   "return statement not allowed in coroutine; did you mean 'co_return'?">;
 def note_declared_coroutine_here : Note<
   "function is a coroutine due to use of '%0' here">;
 def err_coroutine_objc_method : Error<

cfe/trunk/include/clang/Sema/Sema.h

bool MergeFunctionDecl(FunctionDecl *New, NamedDecl *&Old, Scope *S,
                          bool MergeTypeWithOld);
   bool MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
                                     Scope *S, bool MergeTypeWithOld);
   void mergeObjCMethodDecls(ObjCMethodDecl *New, ObjCMethodDecl *Old);
   void MergeVarDecl(VarDecl *New, LookupResult &Previous);
   void MergeVarDeclTypes(VarDecl *New, VarDecl *Old, bool MergeTypeWithOld);
   void MergeVarDeclExceptionSpecs(VarDecl *New, VarDecl *Old);
   bool checkVarDeclRedefinition(VarDecl *OldDefn, VarDecl *NewDefn);
+  void notePreviousDefinition(SourceLocation Old, SourceLocation New);
   bool MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old, Scope *S);
 
   // AssignmentAction - This is used by all the assignment diagnostic functions
   // to represent what is actually causing the operation
   enum AssignmentAction {
     AA_Assigning,
     AA_Passing,
     AA_Returning,

cfe/trunk/lib/Sema/SemaDecl.cpp

bool Sema::isIncompatibleTypedef(TypeDecl *Old, TypedefNameDecl *New) {
   QualType NewType = New->getUnderlyingType();
 
   if (NewType->isVariablyModifiedType()) {
     // Must not redefine a typedef with a variably-modified type.
     int Kind = isa<TypeAliasDecl>(Old) ? 1 : 0;
     Diag(New->getLocation(), diag::err_redefinition_variably_modified_typedef)
       << Kind << NewType;
     if (Old->getLocation().isValid())
-      Diag(Old->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(Old->getLocation(), New->getLocation());
     New->setInvalidDecl();
     return true;
   }
 
   if (OldType != NewType &&
       !OldType->isDependentType() &&
       !NewType->isDependentType() &&
       !Context.hasSameType(OldType, NewType)) {
     int Kind = isa<TypeAliasDecl>(Old) ? 1 : 0;
     Diag(New->getLocation(), diag::err_redefinition_different_typedef)
       << Kind << NewType << OldType;
     if (Old->getLocation().isValid())
-      Diag(Old->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(Old->getLocation(), New->getLocation());
     New->setInvalidDecl();
     return true;
   }
   return false;
 }
 
 /// MergeTypedefNameDecl - We just parsed a typedef 'New' which has the
 /// same name and scope as a previous declaration 'Old'.  Figure out
void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
   // Verify the old decl was also a type.
   TypeDecl *Old = OldDecls.getAsSingle<TypeDecl>();
   if (!Old) {
     Diag(New->getLocation(), diag::err_redefinition_different_kind)
       << New->getDeclName();
 
     NamedDecl *OldD = OldDecls.getRepresentativeDecl();
     if (OldD->getLocation().isValid())
-      Diag(OldD->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(OldD->getLocation(), New->getLocation());
 
     return New->setInvalidDecl();
   }
 
   // If the old declaration is invalid, just give up here.
   if (Old->isInvalidDecl())
     return New->setInvalidDecl();
 
if (getLangOpts().CPlusPlus) {
     //   };
     //
     // since that was the intent of DR56.
     if (!isa<TypedefNameDecl>(Old))
       return;
 
     Diag(New->getLocation(), diag::err_redefinition)
       << New->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     return New->setInvalidDecl();
   }
 
   // Modules always permit redefinition of typedefs, as does C11.
   if (getLangOpts().Modules || getLangOpts().C11)
     return;
 
   // If we have a redefinition of a typedef in C, emit a warning.  This warning
   // is normally mapped to an error, but can be controlled with
   // -Wtypedef-redefinition.  If either the original or the redefinition is
   // in a system header, don't emit this for compatibility with GCC.
   if (getDiagnostics().getSuppressSystemWarnings() &&
       // Some standard types are defined implicitly in Clang (e.g. OpenCL).
       (Old->isImplicit() ||
        Context.getSourceManager().isInSystemHeader(Old->getLocation()) ||
        Context.getSourceManager().isInSystemHeader(New->getLocation())))
     return;
 
   Diag(New->getLocation(), diag::ext_redefinition_of_typedef)
     << New->getDeclName();
-  Diag(Old->getLocation(), diag::note_previous_definition);
+  notePreviousDefinition(Old->getLocation(), New->getLocation());
 }
 
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {
   const OwnershipAttr *OA = dyn_cast<OwnershipAttr>(A);
   const AnnotateAttr *Ann = dyn_cast<AnnotateAttr>(A);
   for (const auto *i : D->attrs())
if (isa<AliasAttr>(NewAttribute) || isa<IFuncAttr>(NewAttribute)) {
         }
       } else {
         VarDecl *VD = cast<VarDecl>(New);
         unsigned Diag = cast<VarDecl>(Def)->isThisDeclarationADefinition() ==
                                 VarDecl::TentativeDefinition
                             ? diag::err_alias_after_tentative
                             : diag::err_redefinition;
         S.Diag(VD->getLocation(), Diag) << VD->getDeclName();
+        if (Diag == diag::err_redefinition)
+          S.notePreviousDefinition(Def->getLocation(), VD->getLocation());
+        else
           S.Diag(Def->getLocation(), diag::note_previous_definition);
         VD->setInvalidDecl();
       }
       ++I;
       continue;
     }
 
     if (const VarDecl *VD = dyn_cast<VarDecl>(Def)) {
       // Tentative definitions are only interesting for the alias check above.
if (UsingShadowDecl *Shadow = dyn_cast<UsingShadowDecl>(OldD)) {
 
       // Check whether the two declarations might declare the same function.
       if (checkUsingShadowRedecl<FunctionDecl>(*this, Shadow, New))
         return true;
       OldD = Old = cast<FunctionDecl>(Shadow->getTargetDecl());
     } else {
       Diag(New->getLocation(), diag::err_redefinition_different_kind)
         << New->getDeclName();
-      Diag(OldD->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(OldD->getLocation(), New->getLocation());
       return true;
     }
   }
 
   // If the old declaration is invalid, just give up here.
   if (Old->isInvalidDecl())
     return true;
 
if (getLangOpts().MicrosoftExt) {
       return true;
     }
   }
 
   if (New->hasAttr<InternalLinkageAttr>() &&
       !Old->hasAttr<InternalLinkageAttr>()) {
     Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
         << New->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     New->dropAttr<InternalLinkageAttr>();
   }
 
   // If a function is first declared with a calling convention, but is later
   // declared or defined without one, all following decls assume the calling
   // convention of the first.
   //
   // It's OK if a function is first declared without a calling convention,
if (NewTemplate) {
       if (auto *Shadow =
               dyn_cast<UsingShadowDecl>(Previous.getRepresentativeDecl()))
         if (checkUsingShadowRedecl<VarDecl>(*this, Shadow, New))
           return New->setInvalidDecl();
     }
   }
   if (!Old) {
     Diag(New->getLocation(), diag::err_redefinition_different_kind)
         << New->getDeclName();
-    Diag(Previous.getRepresentativeDecl()->getLocation(),
-         diag::note_previous_definition);
+    notePreviousDefinition(Previous.getRepresentativeDecl()->getLocation(),
+                           New->getLocation());
     return New->setInvalidDecl();
   }
 
   // Ensure the template parameters are compatible.
   if (NewTemplate &&
       !TemplateParameterListsAreEqual(NewTemplate->getTemplateParameters(),
                                       OldTemplate->getTemplateParameters(),
                                       /*Complain=*/true, TPL_TemplateMatch))
void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
 
   mergeDeclAttributes(New, Old);
   // Warn if an already-declared variable is made a weak_import in a subsequent
   // declaration
   if (New->hasAttr<WeakImportAttr>() &&
       Old->getStorageClass() == SC_None &&
       !Old->hasAttr<WeakImportAttr>()) {
     Diag(New->getLocation(), diag::warn_weak_import) << New->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     // Remove weak_import attribute on new declaration.
     New->dropAttr<WeakImportAttr>();
   }
 
   if (New->hasAttr<InternalLinkageAttr>() &&
       !Old->hasAttr<InternalLinkageAttr>()) {
     Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
         << New->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     New->dropAttr<InternalLinkageAttr>();
   }
 
   // Merge the types.
   VarDecl *MostRecent = Old->getMostRecentDecl();
   if (MostRecent != Old) {
     MergeVarDeclTypes(New, MostRecent,
                       mergeTypeWithPrevious(*this, New, MostRecent, Previous));
void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
   New->setAccess(Old->getAccess());
   if (NewTemplate)
     NewTemplate->setAccess(New->getAccess());
 
   if (Old->isInline())
     New->setImplicitlyInline();
 }
 
+void Sema::notePreviousDefinition(SourceLocation Old, SourceLocation New) {
+  SourceManager &SrcMgr = getSourceManager();
+  auto FNewDecLoc = SrcMgr.getDecomposedLoc(New);
+  auto FOldDecLoc = SrcMgr.getDecomposedLoc(Old);
+  auto *FNew = SrcMgr.getFileEntryForID(FNewDecLoc.first);
+  auto *FOld = SrcMgr.getFileEntryForID(FOldDecLoc.first);
+  auto &HSI = PP.getHeaderSearchInfo();
+  StringRef HdrFilename = SrcMgr.getFilename(SrcMgr.getSpellingLoc(Old));
+
+  auto noteFromModuleOrInclude = [&](SourceLocation &Loc,
+                                     SourceLocation &IncLoc) -> bool {
+    Module *Mod = nullptr;
+    // Redefinition errors with modules are common with non modular mapped
+    // headers, example: a non-modular header H in module A that also gets
+    // included directly in a TU. Pointing twice to the same header/definition
+    // is confusing, try to get better diagnostics when modules is on.
+    if (getLangOpts().Modules) {
+      auto ModLoc = SrcMgr.getModuleImportLoc(Old);
+      if (!ModLoc.first.isInvalid())
+        Mod = HSI.getModuleMap().inferModuleFromLocation(
+            FullSourceLoc(Loc, SrcMgr));
+    }
+
+    if (IncLoc.isValid()) {
+      if (Mod) {
+        Diag(IncLoc, diag::note_redefinition_modules_same_file)
+            << HdrFilename.str() << Mod->getFullModuleName();
+        if (!Mod->DefinitionLoc.isInvalid())
+          Diag(Mod->DefinitionLoc, diag::note_defined_here)
+              << Mod->getFullModuleName();
+      } else {
+        Diag(IncLoc, diag::note_redefinition_include_same_file)
+            << HdrFilename.str();
+      }
+      return true;
+    }
+
+    return false;
+  };
+
+  // Is it the same file and same offset? Provide more information on why
+  // this leads to a redefinition error.
+  bool EmittedDiag = false;
+  if (FNew == FOld && FNewDecLoc.second == FOldDecLoc.second) {
+    SourceLocation OldIncLoc = SrcMgr.getIncludeLoc(FOldDecLoc.first);
+    SourceLocation NewIncLoc = SrcMgr.getIncludeLoc(FNewDecLoc.first);
+    EmittedDiag = noteFromModuleOrInclude(Old, OldIncLoc);
+    EmittedDiag |= noteFromModuleOrInclude(New, NewIncLoc);
+
+    // If the header has no guards, emit a note suggesting one.
+    if (FOld && !HSI.isFileMultipleIncludeGuarded(FOld))
+      Diag(Old, diag::note_use_ifdef_guards);
+
+    if (EmittedDiag)
+      return;
+  }
+
+  // Redefinition coming from different files or couldn't do better above.
+  Diag(Old, diag::note_previous_definition);
+}
+
 /// We've just determined that \p Old and \p New both appear to be definitions
 /// of the same variable. Either diagnose or fix the problem.
 bool Sema::checkVarDeclRedefinition(VarDecl *Old, VarDecl *New) {
   if (!hasVisibleDefinition(Old) &&
       (New->getFormalLinkage() == InternalLinkage ||
        New->isInline() ||
        New->getDescribedVarTemplate() ||
        New->getNumTemplateParameterLists() ||
        New->getDeclContext()->isDependentContext())) {
     // The previous definition is hidden, and multiple definitions are
     // permitted (in separate TUs). Demote this to a declaration.
     New->demoteThisDefinitionToDeclaration();
 
     // Make the canonical definition visible.
     if (auto *OldTD = Old->getDescribedVarTemplate())
       makeMergedDefinitionVisible(OldTD, New->getLocation());
     makeMergedDefinitionVisible(Old, New->getLocation());
     return false;
   } else {
     Diag(New->getLocation(), diag::err_redefinition) << New;
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     New->setInvalidDecl();
     return true;
   }
 }
 
 /// ParsedFreeStandingDeclSpec - This method is invoked when a declspec with
 /// no declarator (e.g. "struct foo;") is parsed.
 Decl *
if (TagDecl *PrevTagDecl = dyn_cast<TagDecl>(PrevDecl)) {
                 return Def;
               } else if (!IsExplicitSpecializationAfterInstantiation) {
                 // A redeclaration in function prototype scope in C isn't
                 // visible elsewhere, so merely issue a warning.
                 if (!getLangOpts().CPlusPlus && S->containedInPrototypeScope())
                   Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name;
                 else
                   Diag(NameLoc, diag::err_redefinition) << Name;
-                Diag(Def->getLocation(), diag::note_previous_definition);
+                notePreviousDefinition(Def->getLocation(),
+                                       NameLoc.isValid() ? NameLoc : KWLoc);
                 // If this is a redefinition, recover by making this
                 // struct be anonymous, which will make any later
                 // references get the previous definition.
                 Name = nullptr;
                 Previous.clear();
                 Invalid = true;
               }
             } else {
// things like templates still live in the tag namespace.
         Diag(PrevDecl->getLocation(), diag::note_previous_decl) << PrevDecl;
         Invalid = true;
 
       // Otherwise, diagnose.
       } else {
         // The tag name clashes with something else in the target scope,
         // issue an error and recover by making this tag be anonymous.
         Diag(NameLoc, diag::err_redefinition_different_kind) << Name;
-        Diag(PrevDecl->getLocation(), diag::note_previous_definition);
+        notePreviousDefinition(PrevDecl->getLocation(), NameLoc);
         Name = nullptr;
         Invalid = true;
       }
 
       // The existing declaration isn't relevant to us; we're in a
       // new scope, so clear out the previous declaration.
       Previous.clear();
     }
if (PrevDecl) {
     assert((getLangOpts().CPlusPlus || !isa<TagDecl>(PrevDecl)) &&
            "Received TagDecl when not in C++!");
     if (!isa<TagDecl>(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S) &&
         shouldLinkPossiblyHiddenDecl(PrevDecl, New)) {
       if (isa<EnumConstantDecl>(PrevDecl))
         Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id;
       else
         Diag(IdLoc, diag::err_redefinition) << Id;
-      Diag(PrevDecl->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(PrevDecl->getLocation(), IdLoc);
       return nullptr;
     }
   }
 
   // Process attributes.
   if (Attr) ProcessDeclAttributeList(S, New, Attr);
   AddPragmaAttributes(S, New);
 

cfe/trunk/test/Modules/Inputs/SameHeader/A.h

+#ifndef __A_h__
+#define __A_h__
+#endif

cfe/trunk/test/Modules/Inputs/SameHeader/B.h

+#ifndef __B_h__
+#define __B_h__
+#include "C.h"
+#endif

cfe/trunk/test/Modules/Inputs/SameHeader/C.h

+#ifndef __C_h__
+#define __C_h__
+int c = 1;
+
+struct aaa {
+  int b;
+};
+
+typedef struct fd_set {
+  char c;
+};
+#endif

cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap

+module X {
+  module A {
+    header "A.h"
+    export *
+  }
+  module B {
+    header "B.h"
+    export *
+  }
+  export *
+}

cfe/trunk/test/Modules/redefinition-same-header.m

+// RUN: rm -rf %t.tmp
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify
+
+// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error@Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error@Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+#include "A.h" // maps to a modular
+#include "C.h" // textual include

cfe/trunk/test/Sema/redefinition-same-header.c

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'int yyy = 42;' > %t/a.h
+// RUN: %clang_cc1 -fsyntax-only %s -I%t  -verify
+
+// expected-error@a.h:1 {{redefinition of 'yyy'}}
+// expected-note@a.h:1 {{unguarded header; consider using #ifdef guards or #pragma once}}
+// expected-note-re@redefinition-same-header.c:11 {{'{{.*}}/a.h' included multiple times, additional include site here}}
+// expected-note-re@redefinition-same-header.c:12 {{'{{.*}}/a.h' included multiple times, additional include site here}}
+
+#include "a.h"
+#include "a.h"
+
+int foo() { return yyy; }

cfe/trunk/test/SemaCXX/modules-ts.cppm

 // expected-error@-2 {{redefinition of module 'foo'}}
 // [e-mail]:* {{loaded from}}
 #endif
 
 static int m; // ok, internal linkage, so no redefinition error
 int n;
 #if TEST >= 2
 // expected-error@-2 {{redefinition of '}}
-// expected-note@-3 {{previous}}
+// expected-note@-3 {{unguarded header; consider using #ifdef guards or #pragma once}}
+// [e-mail]:1 {{'{{.*}}/modules-ts.cppm' included multiple times, additional include site here}}
 #endif
 
 #if TEST == 0
 export {
   int a;
   int b;
   constexpr int *p = &n;
 }