More informative ODR warnings
Trevor Saunders
tsaunders@mozilla.com
Wed Jul 2 22:34:00 GMT 2014
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 = 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
More information about the Gcc-patches
mailing list