Re: More informative ODR warnings
Hi,
this is version after taking into account all the feedback. I reformulated the
warnings on fields and methods to show the type first and then inform about
mismatching elements. On mainline (without ODR type merging) there are no
warnings on libxul, but there are few others during Firefox build.
One interesting is in breakpad where there are types compiled with signed and
unsigned char. I suppose it is ODR violation to use two variants of char in one
program, but I am not 100% sure.
Bootstrapped/regtested x86_64-linux and comitted
Honza
* tree.c (type_in_anonymous_namespace_p): Ignore TREE_PUBLIC
on builtin types.
* ipa-devirt.c: Include stor-layout.h and intl.h
(odr_subtypes_equivalent_p): New function.
(warn_odr): New function.
(warn_type_mismatch): New function.
(odr_types_equivalent_p): New function.
(add_type_duplicate): Use it.
* common.opt (Wodr): New flag.
* doc/invoke.texi (Wodr): Document new warning.
Index: tree.c
===
--- tree.c (revision 212477)
+++ tree.c (working copy)
@@ -11864,6 +11864,10 @@ obj_type_ref_class (tree ref)
bool
type_in_anonymous_namespace_p (const_tree t)
{
+ /* TREE_PUBLIC of TYPE_STUB_DECL may not be properly set for
+ bulitin types; those have CONTEXT NULL. */
+ if (!TYPE_CONTEXT (t))
+return false;
return (TYPE_STUB_DECL (t) !TREE_PUBLIC (TYPE_STUB_DECL (t)));
}
Index: doc/invoke.texi
===
--- doc/invoke.texi (revision 212477)
+++ doc/invoke.texi (working copy)
@@ -260,7 +260,7 @@ Objective-C and Objective-C++ Dialects}.
-Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
-Wmain -Wmaybe-uninitialized -Wmissing-braces -Wmissing-field-initializers
@gol
-Wmissing-include-dirs @gol
--Wno-multichar -Wnonnull -Wno-overflow -Wopenmp-simd @gol
+-Wno-multichar -Wnonnull -Wodr -Wno-overflow -Wopenmp-simd @gol
-Woverlength-strings -Wpacked -Wpacked-bitfield-compat -Wpadded @gol
-Wparentheses -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
-Wpointer-arith -Wno-pointer-to-int-cast @gol
@@ -4915,6 +4915,12 @@ attribute.
@opindex Woverflow
Do not warn about compile-time overflow in constant expressions.
+@opindex Wodr
+@opindex Wno-odr
+@opindex Wodr
+Warn about One Definition Rule violations during link time optimization.
+Require @option{-flto-odr-type-merging} to be enabled. Enabled by default
+
@item -Wopenmp-simd
@opindex Wopenm-simd
Warn if the vectorizer cost model overrides the OpenMP or the Cilk Plus
Index: common.opt
===
--- common.opt (revision 212477)
+++ common.opt (working copy)
@@ -587,6 +587,10 @@ Warn if the loop cannot be optimized due
Wmissing-noreturn
Common Alias(Wsuggest-attribute=noreturn)
+Wodr
+Common Warning
+Warn about some C++ One Definition Rule violations during link time
optimization
+
Woverflow
Common Var(warn_overflow) Init(1) Warning
Warn about overflow in arithmetic expressions
Index: ipa-devirt.c
===
--- ipa-devirt.c(revision 212477)
+++ ipa-devirt.c(working copy)
@@ -130,6 +130,10 @@ along with GCC; see the file COPYING3.
#include tree-dfa.h
#include demangle.h
#include dbgcnt.h
+#include stor-layout.h
+#include intl.h
+
+static bool odr_types_equivalent_p (tree, tree, bool, bool *, pointer_set_t *);
static bool odr_violation_reported = false;
@@ -431,6 +435,498 @@ set_type_binfo (tree type, tree binfo)
gcc_assert (!TYPE_BINFO (type));
}
+/* Compare T2 and T2 based on name or structure. */
+
+static bool
+odr_subtypes_equivalent_p (tree t1, tree t2, pointer_set_t *visited)
+{
+ bool an1, an2;
+
+ /* This can happen in incomplete types that should be handled earlier. */
+ gcc_assert (t1 t2);
+
+ t1 = main_odr_variant (t1);
+ t2 = main_odr_variant (t2);
+ if (t1 == t2)
+return true;
+ if (TREE_CODE (t1) != TREE_CODE (t2))
+return false;
+ if ((TYPE_NAME (t1) == NULL_TREE) != (TYPE_NAME (t2) == NULL_TREE))
+return false;
+ if (TYPE_NAME (t1) DECL_NAME (TYPE_NAME (t1)) != DECL_NAME (TYPE_NAME
(t2)))
+return false;
+
+ /* Anonymous namespace types must match exactly. */
+ an1 = type_in_anonymous_namespace_p (t1);
+ an2 = type_in_anonymous_namespace_p (t2);
+ if (an1 != an2 || an1)
+return false;
+
+ /* For types where we can not establish ODR equivalency, recurse and deeply
+ compare. */
+ if (TREE_CODE (t1) != RECORD_TYPE
+ || !TYPE_BINFO (t1) || !TYPE_BINFO (t2)
+ || !polymorphic_type_binfo_p (TYPE_BINFO (t1))
+ || !polymorphic_type_binfo_p (TYPE_BINFO (t2)))
+{
+ /* This should really be a pair hash, but for the moment we do not need
+100% reliability and it would be better to compare all ODR types so
+recursion here
Re: More informative ODR warnings
Hi Honzo,
On Sun, 13 Jul 2014, Jan Hubicka wrote:
+@opindex Wodr
+@opindex Wno-odr
+@opindex Wodr
+Warn about One Definition Rule violations during link time optimization.
+Require @option{-flto-odr-type-merging} to be enabled. Enabled by default
here in invoke.texi you talk about One Definition Rule violations
(which might imply all) whereas...
+Wodr
+Common Warning
+Warn about some C++ One Definition Rule violations during link time
optimization
...here you note that it's some and add C++.
Should these two restrictions be noted in invoke.texi as well?
Gerald
Re: More informative ODR warnings
Hi Honzo,
Nazdar Geralde,
On Sun, 13 Jul 2014, Jan Hubicka wrote:
+@opindex Wodr
+@opindex Wno-odr
+@opindex Wodr
+Warn about One Definition Rule violations during link time optimization.
+Require @option{-flto-odr-type-merging} to be enabled. Enabled by default
here in invoke.texi you talk about One Definition Rule violations
(which might imply all) whereas...
+Wodr
+Common Warning
+Warn about some C++ One Definition Rule violations during link time
optimization
...here you note that it's some and add C++.
Should these two restrictions be noted in invoke.texi as well?
Actually the first hunk (in invoke.texi) is a version that is correct only with
my
ODR streaming patch that may or may not land mainline. I will update it to the
simplified
version from common.opt. Basically we now warn only on ODR violations within
polymorphic
types.
Honza
Re: More informative ODR warnings
+@opindex Wodr
+@opindex Wno-odr
+@opindex Wodr
+Warn about One Definition Rule violations during link time optimization.
+Require @option{-flto-odr-type-merging} to be enabled. Enabled by default
Duplicated @opindex Wodr. (@item is missing)
Requires. Period after default
But according to current practice (which I dislike), this should be
@item -Wno-odr
@opindex Wodr
@opindex Wno-odr
Disable warnings about One Definition Rule violations during link time
optimization.
These warnings are enabled by default and require
@option{-flto-odr-type-merging} to be enabled.
See the example for -Woverflow just above where you added Wodr.
+ if (!warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (t1)), OPT_Wodr,
+ type %qT violates one definition rule ,
+ t1))
+return;
Why the trailing whitespace within the warning message?
+ if (!warning_at (DECL_SOURCE_LOCATION (st1), OPT_Wodr,
+ field %qD (of type %qT) violates one definition rule ,
+ st1, t1))
I agree with others that say that (of type %qT) is understood as %qD
being of type %qT. I would also suggest to drop the parentheses in the
new message.
+inform (UNKNOWN_LOCATION, Conflicting compilation units: %s and %s,
+IDENTIFIER_POINTER (name),
+IDENTIFIER_POINTER (name1));
GNU-standard diagnostics do not start with uppercase. What do you
think about using %qs here?
+G_(a type with attributes
+ is defined in another translation unit
Is this a type with different attributes?
+ bool warned = 0;
We should really use true/false for booleans.
+ FIXME: disable for now; because ODR types are now build during
+ streaming in, the variants do not need to be linked to the type,
+ yet. We need to do the merging in cleanup pass to be implemented
+ soon. */
if (!flag_ltrans merge
+ 0
TREE_CODE (val-type) == RECORD_TYPE
TREE_CODE (type) == RECORD_TYPE
TYPE_BINFO (val-type) TYPE_BINFO (type)
@@ -569,7 +1076,6 @@ add_type_duplicate (odr_type val, tree t
== master_binfo)
set_type_binfo ((*val-types)[i], TYPE_BINFO (type));
}
- BINFO_TYPE (TYPE_BINFO (type)) = val-type;
}
else
Why commit this part if it is disabled? There seems to be other parts
of the code that are known to not work but will be committed. Perhaps
I am misunderstanding and this is going to a development branch and
not trunk.
And tescases? Shouldn't there be one testcase for each possible
diagnostic? Otherwise we get diagnostics that are never tested and
they stop to work (or they even didn't work in the first place) even
though there are huge chunks of code devoted to them.
Cheers,
Manuel.
More informative ODR warnings
Hi,
this patch adds structural comparsion into ODR warnings, so we do not rely
on types_compatible_p to checks if the individual variants of same
name looks same. This allows us to give more precise reason for the
mismatch and also be more strict than canonical type merging.
Function odr_types_equivalent_p is based on canonical type hash equivalency
from lto.c. Originally I wanted to share the implementation, but details
seems sufficiently different to justify a fresh copy of the whole monster.
To avoid false warnings on types containg va_list pointer, I added
the disucssed hack to type_in_anonymous_namespace_p to return false
on types that do not have public stub but no context (since all anonymous
types produced by FE have as a context something)
Bootstrapped/regtested x86_64-linux, will commit it later this week if there
are no complains.
Currently we warn only about polymorphic types. With my experimental patch for
full ODR type merging we get the following warnings (minus the information
about conflicting units). Those seems to be real bugs in firefox.
/aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning:
field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule
[-Wodr]
struct sctp_sendv_spa *mSpa;
^
../../../../dist/include/mozilla/net/DataChannel.h:64:0: note: a field of same
name but different type is defined in another translation unit
/aux/hubicka/firefox/netwerk/streamconv/converters/nsMultiMixedConv.cpp:466:0:
warning: field ‘mBuffer’ (of type ‘struct AutoFree’) violates one definition
rule [-Wodr]
char *mBuffer;
^
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp:307:0: note: a field with
different name is defined in another translation unit
void *mPtr;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp and
/aux/hubicka/build-firefox491-inst4/netwerk/streamconv/converters/Unified_cpp_netwerk_streamconv_converters0.cpp
../../dist/include/zipstruct.h:47:25: warning: field ‘MOZ_Z_crc32’ (of type
‘struct ZipCentral_’) violates one definition rule [-Wodr]
../../dist/include/zipstruct.h:47:0: note: a field with different name is
defined in another translation unit
lto1: note: Conflicting compilation units:
/aux/hubicka/build-firefox491-inst4/dom/file/Unified_cpp_dom_file0.cpp and
/aux/hubicka/firefox/xpcom/build/FileLocation.cpp
/aux/hubicka/firefox/modules/libjar/zipstruct.h:24:0: warning: field
‘MOZ_Z_crc32’ (of type ‘struct ZipLocal_’) violates one definition rule
[-Wodr]
unsigned char crc32 [4];
^
../../dist/include/zipstruct.h:24:0: note: a field with different name is
defined in another translation unit
lto1: note: Conflicting compilation units:
/aux/hubicka/build-firefox491-inst4/dom/file/Unified_cpp_dom_file0.cpp and
/aux/hubicka/firefox/modules/libjar/nsZipArchive.cpp
../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: warning: field
‘sample_format_’ (of type ‘struct AudioDecoderConfig’) violates one definition
rule [-Wodr]
../../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: note: a field
of same name but different type is defined in another translation unit
SampleFormat sample_format_;
^
../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: type
‘SampleFormat’ should match type ‘AVSampleFormat’
../../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: the
incompatible type is defined here
enum SampleFormat {
^
/aux/hubicka/firefox/modules/libpref/src/nsPrefBranch.cpp:51:16: warning: field
‘parent’ (of type ‘struct EnumerateData’) violates one definition rule [-Wodr]
const char *parent;
^
/aux/hubicka/firefox/layout/base/nsPresArena.cpp:123:0: note: a field with
different name is defined in another translation unit
nsArenaMemoryStats* stats;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/layout/base/nsPresArena.cpp and
/aux/hubicka/build-firefox491-inst4/modules/libpref/src/Unified_cpp_modules_libpref_src0.cpp
./glslang_lex.cpp:815:0: warning: field ‘yyextra_r’ (of type ‘struct yyguts_t’)
violates one definition rule [-Wodr]
./Tokenizer.cpp:575:0: note: a field of same name but different type is defined
in another translation unit
lto1: note: Conflicting compilation units:
/aux/hubicka/build-firefox491-inst4/gfx/angle/Unified_cpp_gfx_angle3.cpp and
/aux/hubicka/firefox/gfx/angle/src/compiler/glslang_lex.cpp
/aux/hubicka/firefox/rdf/base/src/nsInMemoryDataSource.cpp:148:0: warning:
field ‘mHdr’ (of type ‘struct Entry’) violates one definition rule [-Wodr]
PLDHashEntryHdr mHdr;
^
/aux/hubicka/firefox/gfx/skia/trunk/src/core/SkFlattenable.cpp:67:0: note: a
field with different name is defined in another translation unit
const char* fName;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/gfx/skia/trunk/src/core/SkFlattenable.cpp and
/aux/hubicka/build-firefox491-inst4/rdf/base/src/Unified_cpp_rdf_base_src0.cpp
Re: More informative ODR warnings
- Original Message - /aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning: field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule [-Wodr] Can we reword this warning? The of type 'struct BufferedMsg' could be easily taken to mean that the type of the field is 'struct BufferedMsg', rather than the intended meaning. Perhaps within type 'struct BufferedMsg'? -Nathan
Re: More informative ODR warnings
On Jul 2, 2014, at 10:52 AM, Nathan Froyd froy...@mozilla.com wrote: - Original Message - /aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning: field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule [-Wodr] Can we reword this warning? The of type 'struct BufferedMsg' could be easily taken to mean that the type of the field is 'struct BufferedMsg', rather than the intended meaning. Perhaps within type 'struct BufferedMsg’? type is redundant with struct, all structs are types, so within ‘struct” would be slightly shorter, as would just “in ‘struct”.
Re: More informative ODR warnings
On Jul 2, 2014, at 10:52 AM, Nathan Froyd froy...@mozilla.com wrote: - Original Message - /aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning: field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule [-Wodr] Can we reword this warning? The of type 'struct BufferedMsg' could be easily taken to mean that the type of the field is 'struct BufferedMsg', rather than the intended meaning. Perhaps within type 'struct BufferedMsg’? type is redundant with struct, all structs are types, so within ‘struct” would be slightly shorter, as would just “in ‘struct”. Yep, within `struct foo` sounds better. I will update the patch. Any other suggestions are welcome - it is not completely easy to give useful warnings here. At least we are better than previous poitining out two equivalent location and saying they are not compatible Honza
Re: More informative ODR warnings
On Wed, Jul 02, 2014 at 05:15:20PM +0200, Jan Hubicka wrote:
Hi,
this patch adds structural comparsion into ODR warnings, so we do not rely
on types_compatible_p to checks if the individual variants of same
name looks same. This allows us to give more precise reason for the
mismatch and also be more strict than canonical type merging.
Function odr_types_equivalent_p is based on canonical type hash equivalency
from lto.c. Originally I wanted to share the implementation, but details
seems sufficiently different to justify a fresh copy of the whole monster.
To avoid false warnings on types containg va_list pointer, I added
the disucssed hack to type_in_anonymous_namespace_p to return false
on types that do not have public stub but no context (since all anonymous
types produced by FE have as a context something)
Bootstrapped/regtested x86_64-linux, will commit it later this week if there
are no complains.
Currently we warn only about polymorphic types. With my experimental patch
for
full ODR type merging we get the following warnings (minus the information
about conflicting units). Those seems to be real bugs in firefox.
I can't find the code for the SampleFormat thing, but the rest of them
look like ODR violations to me.
/aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning:
field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule
[-Wodr]
struct sctp_sendv_spa *mSpa;
^
../../../../dist/include/mozilla/net/DataChannel.h:64:0: note: a field of
same name but different type is defined in another translation unit
/aux/hubicka/firefox/netwerk/streamconv/converters/nsMultiMixedConv.cpp:466:0:
warning: field ‘mBuffer’ (of type ‘struct AutoFree’) violates one definition
rule [-Wodr]
char *mBuffer;
^
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp:307:0: note: a field with
different name is defined in another translation unit
it seems to me this doesn't get at the real issue that the type names
are the same but the fields are different. maybe a type of the same
name with different fields defined here?
void *mPtr;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp and
/aux/hubicka/build-firefox491-inst4/netwerk/streamconv/converters/Unified_cpp_netwerk_streamconv_converters0.cpp
../../dist/include/zipstruct.h:47:25: warning: field ‘MOZ_Z_crc32’ (of type
‘struct ZipCentral_’) violates one definition rule [-Wodr]
../../dist/include/zipstruct.h:47:0: note: a field with different name is
defined in another translation unit
is it worth poking into why this case doesn't have the caret stuff?
+ /* In Firefox it is a common bug to have same types but in
+ different namespaces. Be a bit more informative on
+ this. */
hm? I make no claim to being a spec lawyer, but I thought something like
namespace foo {
struct bar { int x; };
}
namespace baz {
struct bar { float b; };
}
was legal? or are you refering to some other construct?
+ if (TYPE_CONTEXT (t1) TYPE_CONTEXT (t2)
+ (((TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL)
+ != (TREE_CODE (TYPE_CONTEXT (t2)) == NAMESPACE_DECL))
+|| (TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL
+ (DECL_NAME (TYPE_CONTEXT (t1)) !=
+DECL_NAME (TYPE_CONTEXT (t2))
imho that would be a lot more readable with some temporary variables,
but shrug
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT but is defined
+ in different namespace ,
+ t1, t2);
+ else
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT,
+ t1, t2);
+ inform (DECL_SOURCE_LOCATION (TYPE_NAME (t2)),
+ the incompatible type is defined here);
I didn't see any of these come up in the list of warnings at the
beginning of your mail?
@@ -445,31 +955,23 @@ add_type_duplicate (odr_type val, tree t
{
bool merge = true;
bool base_mismatch = false;
- bool warned = 0;
unsigned int i,j;
+ bool warned = 0;
false?
Trev
Re: More informative ODR warnings
I can't find the code for the SampleFormat thing, but the rest of them
look like ODR violations to me.
I think it is some define renaming the field, I was also puzled by this one.
/aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning:
field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one definition rule
[-Wodr]
struct sctp_sendv_spa *mSpa;
^
../../../../dist/include/mozilla/net/DataChannel.h:64:0: note: a field of
same name but different type is defined in another translation unit
/aux/hubicka/firefox/netwerk/streamconv/converters/nsMultiMixedConv.cpp:466:0:
warning: field ‘mBuffer’ (of type ‘struct AutoFree’) violates one
definition rule [-Wodr]
char *mBuffer;
^
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp:307:0: note: a field with
different name is defined in another translation unit
it seems to me this doesn't get at the real issue that the type names
are the same but the fields are different. maybe a type of the same
name with different fields defined here?
This is what I print when I see name mismatch. It usually means completely
different structure/field.
It speaks of fields names rather than type names. Better wording is welcome.
void *mPtr;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp and
/aux/hubicka/build-firefox491-inst4/netwerk/streamconv/converters/Unified_cpp_netwerk_streamconv_converters0.cpp
../../dist/include/zipstruct.h:47:25: warning: field ‘MOZ_Z_crc32’ (of type
‘struct ZipCentral_’) violates one definition rule [-Wodr]
../../dist/include/zipstruct.h:47:0: note: a field with different name is
defined in another translation unit
is it worth poking into why this case doesn't have the caret stuff?
+ /* In Firefox it is a common bug to have same types but in
+ different namespaces. Be a bit more informative on
+ this. */
hm? I make no claim to being a spec lawyer, but I thought something like
namespace foo {
struct bar { int x; };
}
namespace baz {
struct bar { float b; };
}
was legal? or are you refering to some other construct?
This is legal as long as you don't define
struct foo {
struct bar a;
}
Where foo is same name but bar is once from foo and once from baz.
There are cases where struct bar is in an namespace in one unit, but
it is toplevel in another.
+ if (TYPE_CONTEXT (t1) TYPE_CONTEXT (t2)
+ (((TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL)
+ != (TREE_CODE (TYPE_CONTEXT (t2)) == NAMESPACE_DECL))
+ || (TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL
+ (DECL_NAME (TYPE_CONTEXT (t1)) !=
+ DECL_NAME (TYPE_CONTEXT (t2))
imho that would be a lot more readable with some temporary variables,
but shrug
What? stepping away from old-school GCC writting style???
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT but is defined
+ in different namespace ,
+ t1, t2);
+ else
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT,
+ t1, t2);
+ inform (DECL_SOURCE_LOCATION (TYPE_NAME (t2)),
+ the incompatible type is defined here);
I didn't see any of these come up in the list of warnings at the
beginning of your mail?
I get it here:
../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: warning: field
�~@~Xsample_format_�~@~Y (of type �~@~Xstruct AudioDecoderConfig�~@~Y) violates
one definition rule [-Wodr]
../../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: note: a field
of same name but different type is defined in another translation unit
SampleFormat sample_format_;
^
../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: type
�~@~XSampleFormat�~@~Y should match type �~@~XAVSampleFormat�~@~Y
../../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: the
incompatible type is defined here
enum SampleFormat {
Which suggest that autdio_decoder_config.h is sometimes included with define
turing SampleFormat to XSampleFormat
and sometimes to XAVSampleFormat. Not the most readable warning around, but
probably can't do much better.
The carret diagnostics should not get confused by relative names - I was
thinking about turing all locations into absolute names, but
that is probably not coolest thing either.
@@ -445,31 +955,23 @@ add_type_duplicate (odr_type val, tree t
{
bool merge = true;
bool base_mismatch = false;
- bool warned = 0;
unsigned int i,j;
+ bool warned = 0;
false?
Yep.
Thanks!
I will prepare updated patch with all the comments.
Honza
Trev
Re: More informative ODR warnings
On Wed, Jul 02, 2014 at 09:28:03PM +0200, Jan Hubicka wrote:
I can't find the code for the SampleFormat thing, but the rest of them
look like ODR violations to me.
I think it is some define renaming the field, I was also puzled by this one.
/aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0:
warning: field ‘mSpa’ (of type ‘struct BufferedMsg’) violates one
definition rule [-Wodr]
struct sctp_sendv_spa *mSpa;
^
../../../../dist/include/mozilla/net/DataChannel.h:64:0: note: a field of
same name but different type is defined in another translation unit
/aux/hubicka/firefox/netwerk/streamconv/converters/nsMultiMixedConv.cpp:466:0:
warning: field ‘mBuffer’ (of type ‘struct AutoFree’) violates one
definition rule [-Wodr]
char *mBuffer;
^
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp:307:0: note: a field
with different name is defined in another translation unit
it seems to me this doesn't get at the real issue that the type names
are the same but the fields are different. maybe a type of the same
name with different fields defined here?
This is what I print when I see name mismatch. It usually means completely
different structure/field.
It speaks of fields names rather than type names. Better wording is welcome.
I was sort of suggesting that part I quoted, but its not great either.
maybe
note: First differing member of $whatever defined here
?
void *mPtr;
^
lto1: note: Conflicting compilation units:
/aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp and
/aux/hubicka/build-firefox491-inst4/netwerk/streamconv/converters/Unified_cpp_netwerk_streamconv_converters0.cpp
../../dist/include/zipstruct.h:47:25: warning: field ‘MOZ_Z_crc32’ (of
type ‘struct ZipCentral_’) violates one definition rule [-Wodr]
../../dist/include/zipstruct.h:47:0: note: a field with different name is
defined in another translation unit
is it worth poking into why this case doesn't have the caret stuff?
+ /* In Firefox it is a common bug to have same types but in
+ different namespaces. Be a bit more informative on
+ this. */
hm? I make no claim to being a spec lawyer, but I thought something like
namespace foo {
struct bar { int x; };
}
namespace baz {
struct bar { float b; };
}
was legal? or are you refering to some other construct?
This is legal as long as you don't define
struct foo {
struct bar a;
}
Where foo is same name but bar is once from foo and once from baz.
h, that seems like a kind of confusing thing to write.
There are cases where struct bar is in an namespace in one unit, but
it is toplevel in another.
+ if (TYPE_CONTEXT (t1) TYPE_CONTEXT (t2)
+ (((TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL)
+ != (TREE_CODE (TYPE_CONTEXT (t2)) == NAMESPACE_DECL))
+|| (TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL
+ (DECL_NAME (TYPE_CONTEXT (t1)) !=
+DECL_NAME (TYPE_CONTEXT (t2))
imho that would be a lot more readable with some temporary variables,
but shrug
What? stepping away from old-school GCC writting style???
absolutely ;)
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT but is defined
+ in different namespace ,
+ t1, t2);
+ else
+inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
+ type %qT should match type %qT,
+ t1, t2);
+ inform (DECL_SOURCE_LOCATION (TYPE_NAME (t2)),
+ the incompatible type is defined here);
I didn't see any of these come up in the list of warnings at the
beginning of your mail?
I get it here:
yeah, should have read more carefully.
Trev
../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: warning: field
�~@~Xsample_format_�~@~Y (of type �~@~Xstruct AudioDecoderConfig�~@~Y)
violates one definition rule [-Wodr]
../../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: note: a field
of same name but different type is defined in another translation unit
SampleFormat sample_format_;
^
../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: type
�~@~XSampleFormat�~@~Y should match type �~@~XAVSampleFormat�~@~Y
../../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: the
incompatible type is defined here
enum SampleFormat {
Which suggest that autdio_decoder_config.h is sometimes included with define
turing SampleFormat to XSampleFormat
and sometimes to XAVSampleFormat. Not the most readable warning around, but
probably can't do much better.
The carret diagnostics should not get confused by relative names - I was
thinking about turing all locations into absolute names, but
that is probably not coolest thing either.
@@ -445,31 +955,23 @@ add_type_duplicate (odr_type val, tree t
{
bool merge = true;
bool base_mismatch
Re: More informative ODR warnings
On 07/02/2014 03:34 PM, Trevor Saunders wrote: On Wed, Jul 02, 2014 at 09:28:03PM +0200, Jan Hubicka wrote: it seems to me this doesn't get at the real issue that the type names are the same but the fields are different. maybe a type of the same name with different fields defined here? This is what I print when I see name mismatch. It usually means completely different structure/field. It speaks of fields names rather than type names. Better wording is welcome. I was sort of suggesting that part I quoted, but its not great either. maybe note: First differing member of $whatever defined here ? I think that would be better. The problem is that the different BufferedMsg definitions are not equivalent by the ODR rules, so the primary error should be about BufferedMsg, followed by information about the differing data members. Jason
