Re: Improve LTO type checking during symtab merging

[Richard Biener <rguenther at suse dot de>]

On Mon, 27 Apr 2015, Jan Hubicka wrote:

> Hi,
> this patch strengten ODR checking for requiring match on declarations
> (if both of them are defined in C++ tranlsation unit).  Currently
> ipa-symtab.c will warn only if declarations are incompatible in 
> useless_type_conversion sense.  Now we warn more often, i.e. in the following
> example:
> 
> t.C:
> char a;
> 
> t2.C:
> #include <stdtype.h>
> extern int8_t a;
> void *b=&a;
> 
> Will lead to warning:
> t2.C:2:15: warning: type of ???a??? does not match original declaration
>  extern int8_t a;
>                ^
> <built-in>: note: type name ???char??? should match type name ???signed char???
> /usr/include/stdint.h:37:22: note: the incompatible type is defined here
>  typedef signed char  int8_t;
>                       ^
> /aux/hubicka/t.C:1:6: note: previously declared here
>  char a;
>       ^
> 
> Funnilly enough we do not get warning when t2.C is just "signed char"
> or "unsigned char" because that is builtin type and thus non-ODR. Something
> I need to deal with incrementally.  
> 
> I also added call to warn_types_mismatch that outputs extra note about
> what is wrong with the type: in C++ the mismatch is often carried over
> several levels on declarations and it is hard to work out the reason
> without extra info.
> 
> Richard, According to comments, it seems that the code was tailored to not
> output warning when we can avoid wrong code issues on mismatches based on fact
> that linker would be also silent. I am particularly entertained by the
> following hunk which I killed:
> 
> -      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;
> 
> It is fun we go through the trouble of comparing types and then do nothing ;)
> Type merging is also completed at this time, so at least the comment looks
> out of date.
>
> I think as QOI issue, we do want to warn in cases the code is 
> inconsistent (it is pretty much surely a bug), but perhaps we may want 
> to have the warnings with -Wodr only and use slightly different warning 
> and different -W switch for warnings that unavoidably leads to wrong 
> code surprises?  This should be easy to implement by adding two levels 
> into new warn_type_compatibility_p predicate. I am personaly also OK 
> however with warning always or making all the warnings -Wodr.
>
> What do you think?

Only emitting the warnings with -Wodr looks good to me.  I can't see
how we can decide what cases lead to wrong code surprises and what not
(other than using types_compatible_p ...).  Wrong-code can only(?) happen
if we happen to inline in a way that makes the incosistency visible in
a single function.
 
> Incrementally I am heading towards proper definition of decl 
> compatibility that I can plug into symtab merging and avoid merging 
> incompatible decls (so FORTIFY_SOURCE works).
>
> lto-symtab and ipa-icf both have some knowledge of the problem, I want to get
> both right and factor out common logic.

Sounds good.
 
> Other improvement is to preserve the ODR type info when non-ODR variant
> previals so one can diagnose clash in between C++ units even in mixed
> language linktimes.

Hmm, but then merging ODR with non-ODR variant is already pointing to
a ODR violation?  So why do you need to retain that info?

Btw, there is that old PR41227 which has both wrong-code and diagnostic
impact...

Richard.

> Bootstrapped/regtested x86_64-linux with no new ODR warnings hit by -Werror.
> 
> Honza
> 
> 	* ipa-devirt.c (register_odr_type): Be ready for non-odr main variant
> 	of odr type.
> 	* ipa-utils.h (warn_types_mismatch): New.
> 	* lto/lto-symtab.c (warn_type_compatibility_p): Break out from ...
> 	(lto_symtab_merge): ... here.
> 	(lto_symtab_merge_decls_2): Improve warnings.
> 
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c	(revision 222391)
> +++ ipa-devirt.c	(working copy)
> @@ -2029,10 +2030,14 @@ register_odr_type (tree type)
>        if (in_lto_p)
>          odr_vtable_hash = new odr_vtable_hash_type (23);
>      }
> -  /* Arrange things to be nicer and insert main variants first.  */
> -  if (odr_type_p (TYPE_MAIN_VARIANT (type)))
> +  /* Arrange things to be nicer and insert main variants first.
> +     ??? fundamental prerecorded types do not have mangled names; this
> +     makes it possible that non-ODR type is main_odr_variant of ODR type.
> +     Things may get smoother if LTO FE set mangled name of those types same
> +     way as C++ FE does.  */
> +  if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type))))
>      get_odr_type (TYPE_MAIN_VARIANT (type), true);
> -  if (TYPE_MAIN_VARIANT (type) != type)
> +  if (TYPE_MAIN_VARIANT (type) != type && odr_type_p (main_odr_variant (type)))
>      get_odr_type (type, true);
>  }
>  
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h	(revision 222391)
> +++ ipa-utils.h	(working copy)
> @@ -84,6 +84,7 @@ 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);
>  
>  /* Return vector containing possible targets of polymorphic call E.
>     If COMPLETEP is non-NULL, store true if the list is complete. 
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c	(revision 222391)
> +++ lto/lto-symtab.c	(working copy)
> @@ -203,45 +203,16 @@ 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 true if we want to output waring about T1 and T2.  */
>  
>  static bool
> -lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +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)
> +  /* C++ provide robust way to check for type compatibility via the ODR
> +     rule.  */
> +  if (odr_type_p (prevailing_type) && odr_type_p (type)
> +      && !types_same_for_odr (prevailing_type, type, true))
>      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;
> -    }
> -
> -  /* 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 +225,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 (TREE_CODE (prevailing_type) == FUNCTION_TYPE
> +	  || TREE_CODE (type) == METHOD_TYPE)
> +	return true;
>        if (COMPLETE_TYPE_P (type))
> -	return false;
> +	return true;
>  
>        /* 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 true;
>  
>        if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
>  	{
> @@ -280,10 +254,10 @@ lto_symtab_merge (symtab_node *prevailin
>  	    }
>  
>  	  if (TREE_CODE (tem1) != TREE_CODE (tem2))
> -	    return false;
> +	    return true;
>  
>  	  if (!types_compatible_p (tem1, tem2))
> -	    return false;
> +	    return true;
>  	}
>  
>        /* Fallthru.  Compatible enough.  */
> @@ -292,6 +266,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 false;
> +}
> +
> +/* 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,12 +494,18 @@ 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);
> -
> +      if (warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
> +				     TREE_TYPE (decl)))
> +	{
> +	  bool diag;
> +	  diag = warning_at (DECL_SOURCE_LOCATION (decl), 0,
> +			     "type of %qD does not match original "
> +			     "declaration", decl);
> +	  diagnosed_p |= diag;
> +	  if (diag)
> +	    warn_types_mismatch (TREE_TYPE (prevailing->decl),
> +				 TREE_TYPE (decl));
> +	}
>        else if ((DECL_USER_ALIGN (prevailing->decl)
>  	        && DECL_USER_ALIGN (decl))
>  	       && DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)