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
