From patchwork Mon Apr 27 08:34:17 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 464896 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id E18851402B6 for ; Mon, 27 Apr 2015 18:34:29 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=lFUF2TGE; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=AkFdxIMEzJybjJ2B duwmLxTm5cg2uuOTaIFjLZ5OHfPY32MMgb8rI8LPjr/Vpu7rFKsXUwg/qYaBt/y3 b2SsBHPa56o6wTvv7WuaI4BxTEinxAU7fsOrooRubvmo+2lopxTNX7Sa+jUIS//7 7KtApsFR8x3NkeRh/xgjeTHyPF4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=+a9fQaiq1WAfUm85WnAxtA 1Q8hc=; b=lFUF2TGE3pFgot4O0ZFThghww4pUTCosjksudsnvcgKEWNZVS3sPo/ 2eagekIrmono4XOaOClU8yfeA3/07f48zYbuUuKlDoWoMiZ/J4k/bTEA2jdCvjFk 0V2W7LADPpboPGar892MdEt10MzbLxRG6U7c/cBXSR1NGkJoWEYxg= Received: (qmail 22822 invoked by alias); 27 Apr 2015 08:34:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 22804 invoked by uid 89); 27 Apr 2015 08:34:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 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: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 27 Apr 2015 08:34:20 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 8A11C544D19; Mon, 27 Apr 2015 10:34:17 +0200 (CEST) Date: Mon, 27 Apr 2015 10:34:17 +0200 From: Jan Hubicka To: gcc-patches@gcc.gnu.org, rguenther@suse.de Subject: Improve LTO type checking during symtab merging Message-ID: <20150427083417.GC46857@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) 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? 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. 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. 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))