Author: bruno Date: Thu May 11 01:20:07 2017 New Revision: 302765 URL: http://llvm.org/viewvc/llvm-project?rev=302765&view=rev Log: [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 Added: cfe/trunk/test/Modules/Inputs/SameHeader/ cfe/trunk/test/Modules/Inputs/SameHeader/A.h cfe/trunk/test/Modules/Inputs/SameHeader/B.h cfe/trunk/test/Modules/Inputs/SameHeader/C.h cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap cfe/trunk/test/Modules/redefinition-same-header.m cfe/trunk/test/Sema/redefinition-same-header.c Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/include/clang/Sema/Sema.h cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/test/SemaCXX/modules-ts.cppm Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=302765&r1=302764&r2=302765&view=diff ============================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu May 11 01:20:07 2017 @@ -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 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 { Modified: cfe/trunk/include/clang/Sema/Sema.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=302765&r1=302764&r2=302765&view=diff ============================================================================== --- cfe/trunk/include/clang/Sema/Sema.h (original) +++ cfe/trunk/include/clang/Sema/Sema.h Thu May 11 01:20:07 2017 @@ -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 Modified: cfe/trunk/lib/Sema/SemaDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=302765&r1=302764&r2=302765&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu May 11 01:20:07 2017 @@ -2021,7 +2021,7 @@ bool Sema::isIncompatibleTypedef(TypeDec 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(TypeDec 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 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 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 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(S ? 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(FunctionDec } 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(FunctionDec !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, Lo } 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, Lo 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, Lo !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, Lo 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(VarD 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 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 // 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, 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; } } Added: cfe/trunk/test/Modules/Inputs/SameHeader/A.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/SameHeader/A.h?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/A.h (added) +++ cfe/trunk/test/Modules/Inputs/SameHeader/A.h Thu May 11 01:20:07 2017 @@ -0,0 +1,3 @@ +#ifndef __A_h__ +#define __A_h__ +#endif Added: cfe/trunk/test/Modules/Inputs/SameHeader/B.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/SameHeader/B.h?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/B.h (added) +++ cfe/trunk/test/Modules/Inputs/SameHeader/B.h Thu May 11 01:20:07 2017 @@ -0,0 +1,4 @@ +#ifndef __B_h__ +#define __B_h__ +#include "C.h" +#endif Added: cfe/trunk/test/Modules/Inputs/SameHeader/C.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/SameHeader/C.h?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/C.h (added) +++ cfe/trunk/test/Modules/Inputs/SameHeader/C.h Thu May 11 01:20:07 2017 @@ -0,0 +1,12 @@ +#ifndef __C_h__ +#define __C_h__ +int c = 1; + +struct aaa { + int b; +}; + +typedef struct fd_set { + char c; +}; +#endif Added: cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap (added) +++ cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap Thu May 11 01:20:07 2017 @@ -0,0 +1,11 @@ +module X { + module A { + header "A.h" + export * + } + module B { + header "B.h" + export * + } + export * +} Added: cfe/trunk/test/Modules/redefinition-same-header.m URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/redefinition-same-header.m?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Modules/redefinition-same-header.m (added) +++ cfe/trunk/test/Modules/redefinition-same-header.m Thu May 11 01:20:07 2017 @@ -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 Added: cfe/trunk/test/Sema/redefinition-same-header.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/redefinition-same-header.c?rev=302765&view=auto ============================================================================== --- cfe/trunk/test/Sema/redefinition-same-header.c (added) +++ cfe/trunk/test/Sema/redefinition-same-header.c Thu May 11 01:20:07 2017 @@ -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; } Modified: cfe/trunk/test/SemaCXX/modules-ts.cppm URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/modules-ts.cppm?rev=302765&r1=302764&r2=302765&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/modules-ts.cppm (original) +++ cfe/trunk/test/SemaCXX/modules-ts.cppm Thu May 11 01:20:07 2017 @@ -17,7 +17,8 @@ static int m; // ok, internal linkage, s 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...@modules-ts.cppm:1 {{'{{.*}}/modules-ts.cppm' included multiple times, additional include site here}} #endif #if TEST == 0 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits