[Sema] Improve redefinition errors pointing to the same header

Diagnostics related to redefinition errors that point to the same header
file do not provide much information that helps users fixing the issue.

- In the modules context, it usually happens because of non modular
includes.
- When modules aren't involved it might happen because of the lack of
header guards.

Enhance diagnostics in these scenarios.

Differential Revision: https://reviews.llvm.org/D28832

rdar://problem/31669175

llvm-svn: 302765

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -4609,6 +4609,8 @@ 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'">;
@@ -8888,6 +8890,13 @@ def ext_equivalent_internal_linkage_decl_in_modules : ExtWarn<
   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 {

clang/include/clang/Sema/Sema.h

@@ -2353,6 +2353,7 @@ public:
   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

clang/lib/Sema/SemaDecl.cpp

@@ -2021,7 +2021,7 @@ bool Sema::isIncompatibleTypedef(TypeDecl *Old, TypedefNameDecl *New) {
     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;
   }
@@ -2034,7 +2034,7 @@ bool Sema::isIncompatibleTypedef(TypeDecl *Old, TypedefNameDecl *New) {
     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;
   }
@@ -2101,7 +2101,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 
     NamedDecl *OldD = OldDecls.getRepresentativeDecl();
     if (OldD->getLocation().isValid())
-      Diag(OldD->getLocation(), diag::note_previous_definition);
+      notePreviousDefinition(OldD->getLocation(), New->getLocation());
 
     return New->setInvalidDecl();
   }
@@ -2193,7 +2193,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 
     Diag(New->getLocation(), diag::err_redefinition)
       << New->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
+    notePreviousDefinition(Old->getLocation(), New->getLocation());
     return New->setInvalidDecl();
   }
 
@@ -2214,7 +2214,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 
   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
@@ -2501,7 +2501,10 @@ static void checkNewAttributesAfterDef(Sema &S, Decl *New, const Decl *Old) {
                             ? diag::err_alias_after_tentative
                             : diag::err_redefinition;
         S.Diag(VD->getLocation(), Diag) << VD->getDeclName();
-        S.Diag(Def->getLocation(), diag::note_previous_definition);
+        if (Diag == diag::err_redefinition)
+          S.notePreviousDefinition(Def->getLocation(), VD->getLocation());
+        else
+          S.Diag(Def->getLocation(), diag::note_previous_definition);
         VD->setInvalidDecl();
       }
       ++I;
@@ -2888,7 +2891,7 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
     } 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;
     }
   }
@@ -2925,7 +2928,7 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
       !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>();
   }
 
@@ -3653,9 +3656,9 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
   }
   if (!Old) {
     Diag(New->getLocation(), diag::err_redefinition_different_kind)
-      << New->getDeclName();
-    Diag(Previous.getRepresentativeDecl()->getLocation(),
-         diag::note_previous_definition);
+        << New->getDeclName();
+    notePreviousDefinition(Previous.getRepresentativeDecl()->getLocation(),
+                           New->getLocation());
     return New->setInvalidDecl();
   }
 
@@ -3684,7 +3687,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
       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>();
   }
@@ -3693,7 +3696,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
       !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>();
   }
 
@@ -3850,6 +3853,67 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
     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) {
@@ -3870,7 +3934,7 @@ bool Sema::checkVarDeclRedefinition(VarDecl *Old, VarDecl *New) {
     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;
   }
@@ -13485,7 +13549,8 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                   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.
@@ -13575,7 +13640,7 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
         // 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;
       }
@@ -15279,7 +15344,7 @@ Decl *Sema::ActOnEnumConstant(Scope *S, Decl *theEnumDecl, Decl *lastEnumConst,
         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;
     }
   }

clang/test/Modules/Inputs/SameHeader/A.h

@@ -0,0 +1,3 @@
+#ifndef __A_h__
+#define __A_h__
+#endif

clang/test/Modules/Inputs/SameHeader/B.h

@@ -0,0 +1,4 @@
+#ifndef __B_h__
+#define __B_h__
+#include "C.h"
+#endif

clang/test/Modules/Inputs/SameHeader/C.h

@@ -0,0 +1,12 @@
+#ifndef __C_h__
+#define __C_h__
+int c = 1;
+
+struct aaa {
+  int b;
+};
+
+typedef struct fd_set {
+  char c;
+};
+#endif

clang/test/Modules/Inputs/SameHeader/module.modulemap

@@ -0,0 +1,11 @@
+module X {
+  module A {
+    header "A.h"
+    export *
+  }
+  module B {
+    header "B.h"
+    export *
+  }
+  export *
+}

clang/test/Modules/redefinition-same-header.m

@@ -0,0 +1,20 @@
+// 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

clang/test/Sema/redefinition-same-header.c

@@ -0,0 +1,14 @@
+// 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; }

clang/test/SemaCXX/modules-ts.cppm

@@ -17,7 +17,8 @@ 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}}
+// expected-note-re@modules-ts.cppm:1 {{'{{.*}}/modules-ts.cppm' included multiple times, additional include site here}}
 #endif
 
 #if TEST == 0