This is an archive of the discontinued LLVM Phabricator instance.

Improve redefinition errors pointing to the same header.
ClosedPublic

Authored by bruno on Jan 17 2017, 3:45 PM.

Details

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.

Diff Detail

Event Timeline

bruno created this revision.Jan 17 2017, 3:45 PM
v.g.vassilev edited edge metadata.Jan 18 2017, 1:57 AM

This looks good to me.

Thanks Vassil.

@rsmith is this ok for you?

rsmith added inline comments.Jan 24 2017, 1:44 PM
include/clang/Basic/DiagnosticSemaKinds.td
8840

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.)

8842

This note ends in the middle of a sentence.

lib/Sema/SemaDecl.cpp
3789

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

3805

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).

3813–3817

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.

3821

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.

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 marked 5 inline comments as done.Apr 14 2017, 5:57 PM
bruno added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8840

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
3805

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).

3813–3817

Indeed, going to remove that.

bruno updated this revision to Diff 95369.Apr 14 2017, 6:05 PM
bruno marked 2 inline comments as done.

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 added a comment.Apr 14 2017, 6:24 PM

@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

rsmith accepted this revision.Apr 24 2017, 5:46 PM

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

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

8842

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

lib/Sema/SemaDecl.cpp
3594

Reindent.

3805

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.]

3812

Use isValid to avoid double-negative.

This revision is now accepted and ready to land.Apr 24 2017, 5:46 PM
This revision was automatically updated to reflect the committed changes.