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