On Mon, May 11, 2015 at 6:42 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> Hi,
> this is the updated version of patch with testsuite compensation and
> some changes:
> - As discussed with Richard on IRC, I added -Wlto-type-mismatch to control
> the warnings (enabled by default) and split ODR warnings from warnings
> about incompatible types (in a sense of weak structural equality tested by
> types_compatible_p at LTO time).
>
> Both -Wlto-type-mismatch and -Wodr are on by default. -Wodr points will
> output warnings only on conflicts between C++ programs that are positively
> undefined by the standard (modulo implementation bugs) and
> -Wlto-type-mismatch positives points out likely wrong code since the
> declarations are not compatible to -fstrit-aliasing.
>
> The testuiste fallout of Fortran was not that hard to fix, so I hope
> -Wlto-type-mismatch is weak enough to make sense for mixed language
> units. In fact I can use the ODR type matching tocde to implement
> more strict checks for separate flag (-Wlto-strict-type-mismatch)
> that would be off by default or perhaps just warn between C and C++
> units.
> - I got Firefox building and noticed a false positives on functions
> when forward declaration and prototype was mixed. THis is because
> useless_type_conversion compares function types but requires the outer
> type to be the one more specified (prototype).
> types_compatible_p checks that the types are convertible both directions
> so it return false.
>
> This whole code does not make much sense to be. I do not see why
> useless_type_conversion needs to care about funcition types - we never
> convert these. I also do not see why it match TYPE_METHOD_BASETYPE
> when this field is never used for codegen. It is used by ubsan
> and dbxout only.
>
> I think useless_type_conversion can jsut ICE on function/method types
> and types_compatible_p can be extended by same code as I have
> in warn_types_mismatch minus the TYPE_METHOD_BASETYPE matching.
>
> On a positive note, the patch also finds some real bugs in Firefox :)
> - I also noticed that we may miss some ODR warnings when the declaration
> is of compound type, not named type. (i.e. odr_type a[4];).
> To make these working I added odr_or_derived_type_p and exported
> the functionality to make structural compare from ipa-devirt.
> The way of walking compount types was discussed with Jason here:
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00382.html
>
> Bootstrapped/regtested x86_64-linux, ppc64 running. Also tested with
> ltobootstrap.
> OK?
Looks good to me.
Richard.
> * ipa-utils.h (warn_types_mismatch, odr_or_derived_type_p,
> odr_types_equivalent_p): Declare.
> (odr_type_p): Use gcc_checking_assert.
> * common.opt (Wlto-type-mismatch): New warning.
> * ipa-devirt.c (compound_type_base): New function.
> (odr_or_derived_type_p): New function.
> (odr_types_equivalent_p): New function.
> (add_type_duplicate): Simplify.
>
> * lto-symtab.c (warn_type_compatibility_p): Break out from ...;
> compare ODR types (if available) and function types.
> (lto_symtab_merge): ... here; output ODR violation warnings
> and call warn_types_mismatch.
>
> * gfortran.dg/lto/20091028-2_1.c: Fix return value.
> * gfortran.dg/lto/pr41576_1.f90: Add interface.
> * gfortran.dg/lto/pr41521_0.f90: Disable lto-type-mismatch
> * gfortran.dg/lto/pr60635_0.f90: Disable lto-type-mismatch.
> * gfortran.dg/lto/20091028-1_1.c: Fix return type.
> * gcc.dg/lto/20120723_0.c: Disbale lto-type-mismatch.
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h (revision 222991)
> +++ ipa-utils.h (working copy)
> @@ -84,6 +84,9 @@ bool types_must_be_same_for_odr (tree, t
> bool types_odr_comparable (tree, tree, bool strict = false);
> cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
> ipa_polymorphic_call_context);
> +void warn_types_mismatch (tree t1, tree t2);
> +bool odr_or_derived_type_p (const_tree t);
> +bool odr_types_equivalent_p (tree type1, tree type2);
>
> /* Return vector containing possible targets of polymorphic call E.
> If COMPLETEP is non-NULL, store true if the list is complete.
> @@ -164,7 +167,7 @@ odr_type_p (const_tree t)
> return true;
> /* We do not have this information when not in LTO, but we do not need
> to care, since it is used only for type merging. */
> - gcc_assert (in_lto_p || flag_lto);
> + gcc_checking_assert (in_lto_p || flag_lto);
>
> return (TYPE_NAME (t)
> && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))));
> Index: common.opt
> ===================================================================
> --- common.opt (revision 222991)
> +++ common.opt (working copy)
> @@ -607,6 +607,10 @@ Woverflow
> Common Var(warn_overflow) Init(1) Warning
> Warn about overflow in arithmetic expressions
>
> +Wlto-type-mismatch
> +Common Var(warn_lto_type_mismatch) Init(1) Warning
> +During link time optimization warn about mismatched types of global
> declarations
> +
> Wpacked
> Common Var(warn_packed) Warning
> Warn when the packed attribute has no effect on struct layout
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c (revision 222991)
> +++ lto/lto-symtab.c (working copy)
> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.
> #include "ipa-prop.h"
> #include "ipa-inline.h"
> #include "builtins.h"
> +#include "print-tree.h"
>
> /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> all edges and removing the old node. */
> @@ -203,45 +204,49 @@ lto_varpool_replace_node (varpool_node *
> vnode->remove ();
> }
>
> -/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> - Return false if the symbols are not fully compatible and a diagnostic
> - should be emitted. */
> +/* Return non-zero if we want to output waring about T1 and T2.
> + Return value is a bitmask of reasons of violation:
> + Bit 0 indicates that types are not compatible of memory layout.
> + Bot 1 indicates that types are not compatible because of C++ ODR rule. */
>
> -static bool
> -lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +static int
> +warn_type_compatibility_p (tree prevailing_type, tree type)
> {
> - tree prevailing_decl = prevailing->decl;
> - tree decl = entry->decl;
> - tree prevailing_type, type;
> -
> - if (prevailing_decl == decl)
> - return true;
> -
> - /* Merge decl state in both directions, we may still end up using
> - the new decl. */
> - TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> - TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> -
> - /* The linker may ask us to combine two incompatible symbols.
> - Detect this case and notify the caller of required diagnostics. */
> -
> - if (TREE_CODE (decl) == FUNCTION_DECL)
> - {
> - if (!types_compatible_p (TREE_TYPE (prevailing_decl),
> - TREE_TYPE (decl)))
> - /* If we don't have a merged type yet...sigh. The linker
> - wouldn't complain if the types were mismatched, so we
> - probably shouldn't either. Just use the type from
> - whichever decl appears to be associated with the
> - definition. If for some odd reason neither decl is, the
> - older one wins. */
> - (void) 0;
> -
> - return true;
> + int lev = 0;
> + /* C++ provide a robust way to check for type compatibility via the ODR
> + rule. */
> + if (odr_or_derived_type_p (prevailing_type) && odr_type_p (type)
> + && !odr_types_equivalent_p (prevailing_type, type))
> + lev = 2;
> +
> + /* Function types needs special care, because types_compatible_p never
> + thinks prototype is compatible to non-prototype. */
> + if ((TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE)
> + && TREE_CODE (type) == TREE_CODE (prevailing_type))
> + {
> + lev |= warn_type_compatibility_p (TREE_TYPE (prevailing_type),
> + TREE_TYPE (type));
> + if (TREE_CODE (type) == METHOD_TYPE)
> + lev |= warn_type_compatibility_p (TYPE_METHOD_BASETYPE
> (prevailing_type),
> + TYPE_METHOD_BASETYPE (type));
> + if (prototype_p (prevailing_type) && prototype_p (type)
> + && TYPE_ARG_TYPES (prevailing_type) != TYPE_ARG_TYPES (type))
> + {
> + tree parm1, parm2;
> + for (parm1 = TYPE_ARG_TYPES (prevailing_type),
> + parm2 = TYPE_ARG_TYPES (type);
> + parm1 && parm2;
> + parm1 = TREE_CHAIN (prevailing_type),
> + parm2 = TREE_CHAIN (type))
> + lev |= warn_type_compatibility_p (TREE_VALUE (parm1),
> + TREE_VALUE (parm2));
> + if (parm1 || parm2)
> + lev = 3;
> + }
> + if (comp_type_attributes (prevailing_type, type) == 0)
> + lev = 3;
> + return lev;
> }
> -
> - /* Now we exclusively deal with VAR_DECLs. */
> -
> /* Sharing a global symbol is a strong hint that two types are
> compatible. We could use this information to complete
> incomplete pointed-to types more aggressively here, ignoring
> @@ -254,19 +259,22 @@ lto_symtab_merge (symtab_node *prevailin
> ??? In principle we might want to only warn for structurally
> incompatible types here, but unless we have protective measures
> for TBAA in place that would hide useful information. */
> - prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
> - type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
> + prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
> + type = TYPE_MAIN_VARIANT (type);
>
> if (!types_compatible_p (prevailing_type, type))
> {
> - if (COMPLETE_TYPE_P (type))
> - return false;
> + if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
> + || TREE_CODE (type) == METHOD_TYPE)
> + return 1 | lev;
> + if (COMPLETE_TYPE_P (type) && COMPLETE_TYPE_P (prevailing_type))
> + return 1 | lev;
>
> /* If type is incomplete then avoid warnings in the cases
> that TBAA handles just fine. */
>
> if (TREE_CODE (prevailing_type) != TREE_CODE (type))
> - return false;
> + return 1 | lev;
>
> if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
> {
> @@ -280,10 +288,10 @@ lto_symtab_merge (symtab_node *prevailin
> }
>
> if (TREE_CODE (tem1) != TREE_CODE (tem2))
> - return false;
> + return 1 | lev;
>
> if (!types_compatible_p (tem1, tem2))
> - return false;
> + return 1 | lev;
> }
>
> /* Fallthru. Compatible enough. */
> @@ -292,6 +300,43 @@ lto_symtab_merge (symtab_node *prevailin
> /* ??? We might want to emit a warning here if type qualification
> differences were spotted. Do not do this unconditionally though. */
>
> + return lev;
> +}
> +
> +/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> + Return false if the symbols are not fully compatible and a diagnostic
> + should be emitted. */
> +
> +static bool
> +lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +{
> + tree prevailing_decl = prevailing->decl;
> + tree decl = entry->decl;
> +
> + if (prevailing_decl == decl)
> + return true;
> +
> + /* Merge decl state in both directions, we may still end up using
> + the new decl. */
> + TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> + TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> +
> + /* The linker may ask us to combine two incompatible symbols.
> + Detect this case and notify the caller of required diagnostics. */
> +
> + if (TREE_CODE (decl) == FUNCTION_DECL)
> + {
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> + return true;
> + }
> +
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> /* There is no point in comparing too many details of the decls here.
> The type compatibility checks or the completing of types has properly
> dealt with most issues. */
> @@ -483,24 +528,39 @@ lto_symtab_merge_decls_2 (symtab_node *f
> /* Diagnose all mismatched re-declarations. */
> FOR_EACH_VEC_ELT (mismatches, i, decl)
> {
> - if (!types_compatible_p (TREE_TYPE (prevailing->decl),
> - TREE_TYPE (decl)))
> - diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
> - "type of %qD does not match original "
> - "declaration", decl);
> -
> + int level = warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl));
> + if (level)
> + {
> + bool diag = false;
> + if (level > 1)
> + diag = warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wodr,
> + "%qD violates the C++ One Definition Rule ",
> + decl);
> + if (!diag && (level & 1))
> + diag = warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wlto_type_mismatch,
> + "type of %qD does not match original "
> + "declaration", decl);
> + if (diag)
> + warn_types_mismatch (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl));
> + diagnosed_p |= diag;
> + }
> else if ((DECL_USER_ALIGN (prevailing->decl)
> && DECL_USER_ALIGN (decl))
> && DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
> {
> - diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
> + diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wlto_type_mismatch,
> "alignment of %qD is bigger than "
> "original declaration", decl);
> }
> }
> if (diagnosed_p)
> inform (DECL_SOURCE_LOCATION (prevailing->decl),
> - "previously declared here");
> + "%qD was previously declared here", prevailing->decl);
>
> mismatches.release ();
> }
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c (revision 222991)
> +++ ipa-devirt.c (working copy)
> @@ -549,6 +549,59 @@ types_must_be_same_for_odr (tree t1, tre
> return TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2);
> }
>
> +/* If T is compound type, return type it is based on. */
> +
> +static tree
> +compound_type_base (const_tree t)
> +{
> + if (TREE_CODE (t) == ARRAY_TYPE
> + || POINTER_TYPE_P (t)
> + || TREE_CODE (t) == COMPLEX_TYPE
> + || VECTOR_TYPE_P (t))
> + return TREE_TYPE (t);
> + if (TREE_CODE (t) == METHOD_TYPE)
> + return TYPE_METHOD_BASETYPE (t);
> + if (TREE_CODE (t) == OFFSET_TYPE)
> + return TYPE_OFFSET_BASETYPE (t);
> + return NULL_TREE;
> +}
> +
> +/* Return true if T is either ODR type or compound type based from it.
> + If the function return true, we know that T is a type originating from C++
> + source even at link-time. */
> +
> +bool
> +odr_or_derived_type_p (const_tree t)
> +{
> + do
> + {
> + if (odr_type_p (t))
> + return true;
> + /* Function type is a tricky one. Basically we can consider it
> + ODR derived if return type or any of the parameters is.
> + We need to check all parameters because LTO streaming merges
> + common types (such as void) and they are not considered ODR then. */
> + if (TREE_CODE (t) == FUNCTION_TYPE)
> + {
> + if (TYPE_METHOD_BASETYPE (t))
> + t = TYPE_METHOD_BASETYPE (t);
> + else
> + {
> + if (TREE_TYPE (t) && odr_or_derived_type_p (TREE_TYPE (t)))
> + return true;
> + for (t = TYPE_ARG_TYPES (t); t; t = TREE_CHAIN (t))
> + if (odr_or_derived_type_p (TREE_VALUE (t)))
> + return true;
> + return false;
> + }
> + }
> + else
> + t = compound_type_base (t);
> + }
> + while (t);
> + return t;
> +}
> +
> /* Compare types T1 and T2 and return true if they are
> equivalent. */
>
> @@ -1577,6 +1642,20 @@ odr_types_equivalent_p (tree t1, tree t2
> return true;
> }
>
> +/* Return true if TYPE1 and TYPE2 are equivalent for One Definition Rule. */
> +
> +bool
> +odr_types_equivalent_p (tree type1, tree type2)
> +{
> + hash_set<type_pair,pair_traits> visited;
> +
> +#ifdef ENABLE_CHECKING
> + gcc_assert (odr_or_derived_type_p (type1) && odr_or_derived_type_p
> (type2));
> +#endif
> + return odr_types_equivalent_p (type1, type2, false, NULL,
> + &visited);
> +}
> +
> /* TYPE is equivalent to VAL by ODR, but its tree representation differs
> from VAL->type. This may happen in LTO where tree merging did not merge
> all variants of the same type or due to ODR violation.
> @@ -1701,12 +1780,8 @@ add_type_duplicate (odr_type val, tree t
> base_mismatch = true;
> }
> else
> - {
> - hash_set<type_pair,pair_traits> visited;
> - if (!odr_types_equivalent_p (type1, type2, false, NULL,
> - &visited))
> - base_mismatch = true;
> - }
> + if (!odr_types_equivalent_p (type1, type2))
> + base_mismatch = true;
> if (base_mismatch)
> {
> if (!warned && !val->odr_violated)
> Index: testsuite/gfortran.dg/lto/20091028-2_1.c
> ===================================================================
> --- testsuite/gfortran.dg/lto/20091028-2_1.c (revision 222991)
> +++ testsuite/gfortran.dg/lto/20091028-2_1.c (working copy)
> @@ -1,9 +1,9 @@
> extern void *memcpy(void *dest, const void *src, __SIZE_TYPE__ n);
> char *p;
> -int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> - int * itypesize, int * typesize,
> - int * DataHandle, char * Data,
> - int * Count, int * code)
> +void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> + int * itypesize, int * typesize,
> + int * DataHandle, char * Data,
> + int * Count, int * code)
> {
> memcpy (typesize, p, sizeof(int)) ;
> memcpy (Data, p, *Count * *typesize) ;
> Index: testsuite/gfortran.dg/lto/pr41576_1.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41576_1.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41576_1.f90 (working copy)
> @@ -5,3 +5,8 @@ program test
> if (c/=1 .or. d/=2) call abort
> end program test
>
> +interface
> + subroutine foo()
> + end subroutine
> +end interface
> +
> Index: testsuite/gfortran.dg/lto/pr41521_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41521_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41521_0.f90 (working copy)
> @@ -1,9 +1,9 @@
> ! { dg-lto-do link }
> -! { dg-lto-options {{-g -flto} {-g -O -flto}} }
> +! { dg-lto-options {{-g -flto -Wno-lto-type-mismatch} {-g -O -flto
> -Wno-lto-type-mismatch}} }
> program species
> integer spk(2)
> real eval(2)
> spk = 2
> call atom(1.1,spk,eval)
> end program
>
> Index: testsuite/gfortran.dg/lto/pr60635_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr60635_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr60635_0.f90 (working copy)
> @@ -1,4 +1,5 @@
> ! { dg-lto-do link }
> +! { dg-lto-options {{ -Wno-lto-type-mismatch }} }
> program test
> use iso_fortran_env
>
> Index: testsuite/gfortran.dg/lto/pr41576_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41576_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41576_0.f90 (working copy)
> @@ -1,5 +1,5 @@
> ! { dg-lto-do run }
> -! { dg-lto-options { { -O2 -flto -Werror } } }
> +! { dg-lto-options { { -O2 -flto -Werror -Wno-lto-type-mismatch } } }
>
> subroutine foo
> common /bar/ a, b
> Index: testsuite/gfortran.dg/lto/20091028-1_1.c
> ===================================================================
> --- testsuite/gfortran.dg/lto/20091028-1_1.c (revision 222991)
> +++ testsuite/gfortran.dg/lto/20091028-1_1.c (working copy)
> @@ -1,9 +1,9 @@
> extern void bcopy(const void *, void *, __SIZE_TYPE__ n);
> char *p;
> -int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> - int * itypesize, int * typesize,
> - int * DataHandle, char * Data,
> - int * Count, int * code)
> +void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> + int * itypesize, int * typesize,
> + int * DataHandle, char * Data,
> + int * Count, int * code)
> {
> bcopy (typesize, p, sizeof(int)) ;
> bcopy (Data, p, *Count * *typesize) ;
> Index: testsuite/gcc.dg/lto/20120723_0.c
> ===================================================================
> --- testsuite/gcc.dg/lto/20120723_0.c (revision 222991)
> +++ testsuite/gcc.dg/lto/20120723_0.c (working copy)
> @@ -3,7 +3,7 @@
> ??? This testcase is invalid C and can only pass on specific platforms.
> */
> /* { dg-lto-do run } */
> /* { dg-skip-if "" { { sparc*-*-* } && ilp32 } { "*" } { "" } } */
> -/* { dg-lto-options { {-O3 -fno-early-inlining -flto}} } */
> +/* { dg-lto-options { {-O3 -fno-early-inlining -flto
> -Wno-lto-type-mismatch}} } */
>
> extern void abort (void);
>
