Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -4609,6 +4609,8 @@ "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 @@ InGroup>; 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 { Index: cfe/trunk/include/clang/Sema/Sema.h =================================================================== --- cfe/trunk/include/clang/Sema/Sema.h +++ cfe/trunk/include/clang/Sema/Sema.h @@ -2353,6 +2353,7 @@ 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 Index: cfe/trunk/lib/Sema/SemaDecl.cpp =================================================================== --- cfe/trunk/lib/Sema/SemaDecl.cpp +++ cfe/trunk/lib/Sema/SemaDecl.cpp @@ -2021,7 +2021,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ ? 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 @@ } 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 @@ !Old->hasAttr()) { Diag(New->getLocation(), diag::err_internal_linkage_redeclaration) << New->getDeclName(); - Diag(Old->getLocation(), diag::note_previous_definition); + notePreviousDefinition(Old->getLocation(), New->getLocation()); New->dropAttr(); } @@ -3653,9 +3656,9 @@ } 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 @@ Old->getStorageClass() == SC_None && !Old->hasAttr()) { 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(); } @@ -3693,7 +3696,7 @@ !Old->hasAttr()) { Diag(New->getLocation(), diag::err_internal_linkage_redeclaration) << New->getDeclName(); - Diag(Old->getLocation(), diag::note_previous_definition); + notePreviousDefinition(Old->getLocation(), New->getLocation()); New->dropAttr(); } @@ -3850,6 +3853,67 @@ 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 @@ 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 @@ 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 @@ // 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 @@ 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; } } Index: cfe/trunk/test/Modules/Inputs/SameHeader/A.h =================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/A.h +++ cfe/trunk/test/Modules/Inputs/SameHeader/A.h @@ -0,0 +1,3 @@ +#ifndef __A_h__ +#define __A_h__ +#endif Index: cfe/trunk/test/Modules/Inputs/SameHeader/B.h =================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/B.h +++ cfe/trunk/test/Modules/Inputs/SameHeader/B.h @@ -0,0 +1,4 @@ +#ifndef __B_h__ +#define __B_h__ +#include "C.h" +#endif Index: cfe/trunk/test/Modules/Inputs/SameHeader/C.h =================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/C.h +++ cfe/trunk/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 Index: cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap =================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap +++ cfe/trunk/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 * +} Index: cfe/trunk/test/Modules/redefinition-same-header.m =================================================================== --- cfe/trunk/test/Modules/redefinition-same-header.m +++ cfe/trunk/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 Index: cfe/trunk/test/Sema/redefinition-same-header.c =================================================================== --- cfe/trunk/test/Sema/redefinition-same-header.c +++ cfe/trunk/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; } Index: cfe/trunk/test/SemaCXX/modules-ts.cppm =================================================================== --- cfe/trunk/test/SemaCXX/modules-ts.cppm +++ cfe/trunk/test/SemaCXX/modules-ts.cppm @@ -17,7 +17,8 @@ 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