r302765 - [Sema] Improve redefinition errors pointing to the same header
Bruno Cardoso Lopes via cfe-commits
cfe-commits at lists.llvm.org
Wed May 10 23:20:07 PDT 2017
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 at Inputs/SameHeader/C.h:3 {{redefinition of 'c'}}
+// expected-note-re at Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note at Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re at redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error at Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}}
+// expected-note-re at Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note at Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re at redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error at Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}}
+// expected-note-re at Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note at Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re at 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 at a.h:1 {{redefinition of 'yyy'}}
+// expected-note at a.h:1 {{unguarded header; consider using #ifdef guards or #pragma once}}
+// expected-note-re at redefinition-same-header.c:11 {{'{{.*}}/a.h' included multiple times, additional include site here}}
+// expected-note-re at 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 at -2 {{redefinition of '}}
-// expected-note at -3 {{previous}}
+// expected-note at -3 {{unguarded header; consider using #ifdef guards or #pragma once}}
+// expected-note-re at modules-ts.cppm:1 {{'{{.*}}/modules-ts.cppm' included multiple times, additional include site here}}
#endif
#if TEST == 0
More information about the cfe-commits
mailing list