From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64772 invoked by alias); 27 Apr 2015 08:54:41 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 64760 invoked by uid 89); 27 Apr 2015 08:54:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 27 Apr 2015 08:54:38 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 83F43AB1C; Mon, 27 Apr 2015 08:54:35 +0000 (UTC) Date: Mon, 27 Apr 2015 08:54:00 -0000 From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: Improve LTO type checking during symtab merging In-Reply-To: <20150427083417.GC46857@kam.mff.cuni.cz> Message-ID: References: <20150427083417.GC46857@kam.mff.cuni.cz> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-04/txt/msg01586.txt.bz2 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 > 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; > ^ > : 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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)