This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]