From patchwork Wed Feb 1 01:31:01 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Diego Novillo X-Patchwork-Id: 138888 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]) by ozlabs.org (Postfix) with SMTP id 82263B6F98 for ; Wed, 1 Feb 2012 12:31:29 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1328664690; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:Received:Received:Received:To:Subject:Message-Id:Date: From:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=sVc14tS uY3QRmLVzkwm8u0uUXc4=; b=CxsGdrNmNUUq4ay3Fo2zo8YcUNyFcHvNsYcTJy9 S6NBU08o+U3pYeLnDd5vKu5Gz81iCn5RGOdt9TfTASKtxB2GlkDBhy63TMhm34gH TE20Kc1MDTbytffa9HPWenhWJcerqPSMe8Lx5mxCllz+aa3IlDfoLa0EVZ0oP/i9 OUR4= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:Received:Received:Received:To:Subject:Message-Id:Date:From:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=OJBZg7vD3R0tJ6ij7frqwxXxiTfDXbb2p02mfVmrOq9CEYtLZcrpsqyotVAro5 PKH/U5lQtyvcijg3BKfLNXxalAW5Zohd5M0yDuJVICoDJF05+BCRSlTFturSNeDO gEr5DSt9VdVG8IbGC942cU8UvY1ss+WQgjNkDfvKruzUg=; Received: (qmail 14956 invoked by alias); 1 Feb 2012 01:31:24 -0000 Received: (qmail 14881 invoked by uid 22791); 1 Feb 2012 01:31:20 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_LOW, SARE_OBFU_PART_INA, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-wi0-f201.google.com (HELO mail-wi0-f201.google.com) (209.85.212.201) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 01 Feb 2012 01:31:05 +0000 Received: by wibhq12 with SMTP id hq12so28206wib.2 for ; Tue, 31 Jan 2012 17:31:03 -0800 (PST) Received: by 10.14.122.205 with SMTP id t53mr1395373eeh.8.1328059863638; Tue, 31 Jan 2012 17:31:03 -0800 (PST) Received: by 10.14.122.205 with SMTP id t53mr1395360eeh.8.1328059863520; Tue, 31 Jan 2012 17:31:03 -0800 (PST) Received: from hpza10.eem.corp.google.com ([74.125.121.33]) by gmr-mx.google.com with ESMTPS id g43si15954308eea.0.2012.01.31.17.31.03 (version=TLSv1/SSLv3 cipher=AES128-SHA); Tue, 31 Jan 2012 17:31:03 -0800 (PST) Received: from tobiano.tor.corp.google.com (tobiano.tor.corp.google.com [172.29.41.6]) by hpza10.eem.corp.google.com (Postfix) with ESMTP id 0D10A20004E; Tue, 31 Jan 2012 17:31:03 -0800 (PST) Received: by tobiano.tor.corp.google.com (Postfix, from userid 54752) id AE92FAE1DA; Tue, 31 Jan 2012 20:31:01 -0500 (EST) To: reply@codereview.appspotmail.com, crowl@google.com, gcc-patches@gcc.gnu.org Subject: [pph] Fix streaming of structures inside shared structures (issue5607045) Message-Id: <20120201013101.AE92FAE1DA@tobiano.tor.corp.google.com> Date: Tue, 31 Jan 2012 20:31:01 -0500 (EST) From: dnovillo@google.com (Diego Novillo) X-IsSubscribed: yes 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 This patch fixes a problem with streaming of symbols and types that have mutated from one PPH to another. --- parent.h ----------------------------------------------------------------- #include "child.h" struct C1 { int field1; float field2; }; ----------------------------------------------------------------------------- --- child.h ----------------------------------------------------------------- struct C1; ----------------------------------------------------------------------------- When we read child.pph from parent.pph, we will notice that struct C1 has mutated, so we read its body again (which fills in the fields, etc). However, the TYPE_LANG_DECL structure for C1 is also present in the pickle cache for child.pph, and we were using it unmodified. In this case, one of the fields in TYPE_LANG_DECL was CLASSTYPE_LAZY_DEFAULT_CTOR, which had not been set in child.pph, so when we tried to use the default ctor from struct C1 during code generation, we were never finding it. This was causing the failure in x1namespace-alias2.cc and two other ICEs in the dynarray tests. What the patch does is to never insert in the pickle cache any structures that are embedded in structures that are already inside other structures that get put in the cache. We now always write out the contents of these structures (using the PPH_RECORD_START_NO_CACHE marker). This includes the structures: PPH_binding_table, PPH_function, PPH_lang_decl, PPH_lang_type, PPH_language_function and PPH_sorted_fields_type. Since these structures are always embedded in structures that are already shared, they are never read and allocated more than once, unless the parent structure is pickled. Additionally, this simplifies the logic in the reader, since it never needs to worry about adding these structures to the cache. cp/ChangeLog.pph * name-lookup.c (pph_out_binding_table): Use PPH_RECORD_START_NO_CACHE records. (pph_in_binding_table): Likewise. * pph-in.c (pph_in_language_function): Remove caching code, assume that the structure is always in a PPH_RECORD_START_NO_CACHE. (pph_in_struct_function): Likewise. (pph_in_lang_decl): Likewise. (pph_in_sorted_fields_type): Likewise. (pph_in_lang_type_class): Likewise. (pph_in_lang_type): Likewise. (pph_in_lang_decl_start): Remove. * pph-out.c (pph_get_marker_for): Always return PPH_RECORD_START_NO_CACHE for PPH_binding_table, PPH_function, PPH_lang_decl, PPH_lang_type, PPH_language_function and PPH_sorted_fields_type. (pph_out_start_record): Do not write IX for PPH_RECORD_START_NO_CACHE. (pph_out_language_function): Only support PPH_RECORD_START_NO_CACHE or PPH_RECORD_END. (pph_out_struct_function): Likewise. (pph_out_lang_decl): Likewise. (pph_out_sorted_fields_type): Likewise. (pph_out_lang_type_class): Likewise. (pph_out_lang_type): Likewise. testsuite/ChangeLog.pph * g++.dg/pph/x1namespace-alias2.cc: Mark fixed. * g++.dg/pph/x6dynarray4.cc: Remove expected ICE. * g++.dg/pph/x7dynarray5.cc: Remove expected ICE. --- This patch is available for review at http://codereview.appspot.com/5607045 diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 463b14d..cbc8ee9 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -6057,7 +6057,7 @@ pph_out_binding_table (pph_stream *stream, binding_table bt) { if (bt->chain[i]) { - pph_out_record_marker (stream, PPH_RECORD_START, PPH_binding_entry); + pph_out_record_marker (stream, PPH_RECORD_START_NO_CACHE, PPH_binding_entry); pph_out_tree (stream, bt->chain[i]->name); pph_out_tree (stream, bt->chain[i]->type); } @@ -6083,7 +6083,7 @@ pph_in_binding_table (pph_stream *stream) enum pph_tag tag; enum pph_record_marker marker = pph_in_record_marker (stream, &tag); gcc_assert (tag == PPH_binding_entry); - if (marker == PPH_RECORD_START) + if (marker == PPH_RECORD_START_NO_CACHE) { tree name = pph_in_tree (stream); tree type = pph_in_tree (stream); diff --git a/gcc/cp/pph-in.c b/gcc/cp/pph-in.c index 52d21ae..114ceda 100644 --- a/gcc/cp/pph-in.c +++ b/gcc/cp/pph-in.c @@ -1300,16 +1300,10 @@ pph_in_language_function (pph_stream *stream) marker = pph_in_start_record (stream, &image_ix, &ix, PPH_language_function); if (marker == PPH_RECORD_END) return NULL; - else if (pph_is_reference_marker (marker)) - return (struct language_function *) pph_cache_find (stream, marker, - image_ix, ix, - PPH_language_function); - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); - ALLOC_AND_REGISTER (&stream->cache, ix, PPH_language_function, lf, - ggc_alloc_cleared_language_function ()); + lf = ggc_alloc_cleared_language_function (); lf->base.x_stmt_tree.x_cur_stmt_list = pph_in_tree_vec (stream); lf->base.x_stmt_tree.stmts_are_full_exprs_p = pph_in_uint (stream); lf->x_cdtor_label = pph_in_tree (stream); @@ -1353,37 +1347,23 @@ pph_in_struct_function (pph_stream *stream, tree decl) marker = pph_in_start_record (stream, &image_ix, &ix, PPH_function); if (marker == PPH_RECORD_END) return; - else if (pph_is_reference_marker (marker)) - { - fn = (struct function *) pph_cache_find (stream, marker, image_ix, ix, - PPH_function); - gcc_assert (DECL_STRUCT_FUNCTION (decl) == fn); - return; - } - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); /* Possibly allocate a new DECL_STRUCT_FUNCTION for DECL. */ t = pph_in_tree (stream); gcc_assert (t == decl); fn = DECL_STRUCT_FUNCTION (decl); - if (!fn) + if (fn == NULL) { /* The DECL_STRUCT_FUNCTION does not already already exists, which implies that we are reading an entirely new function and not merging into an existing function. */ - /* We would normally use ALLOC_AND_REGISTER, - but allocate_struct_function does not return a pointer. */ allocate_struct_function (decl, false); fn = DECL_STRUCT_FUNCTION (decl); } - /* Now register it. */ - pph_cache_insert_at (&stream->cache, fn, ix, PPH_function); - /* FIXME pph: For now, accept the new version of the fields when merging. */ - input_struct_function_base (fn, stream->encoder.r.data_in, stream->encoder.r.ib); @@ -1574,79 +1554,32 @@ pph_in_ld_parm (pph_stream *stream, struct lang_decl_parm *ldp) } -/* Read from STREAM the start of a lang_decl record for DECL. If the - caller should do a merge-read, set *IS_MERGE_P to true. Return - lang_decl structure associated with DECL. If this function returns - NULL, it means that the lang_decl record has already been read and - nothing else needs to be done. */ +/* Read language specific data in DECL from STREAM. */ -static struct lang_decl * -pph_in_lang_decl_start (pph_stream *stream, tree decl, bool *is_merge_p) +static void +pph_in_lang_decl (pph_stream *stream, tree decl) { + struct lang_decl *ld; + struct lang_decl_base *ldb; + bool is_merge; enum pph_record_marker marker; unsigned image_ix, ix; - struct lang_decl *ld; marker = pph_in_start_record (stream, &image_ix, &ix, PPH_lang_decl); if (marker == PPH_RECORD_END) - return NULL; - else if (pph_is_reference_marker (marker)) - { - DECL_LANG_SPECIFIC (decl) = - (struct lang_decl *) pph_cache_find (stream, marker, image_ix, ix, - PPH_lang_decl); - return NULL; - } - else if (marker == PPH_RECORD_START_MERGE_BODY) - { - /* If we are about to read the merge body for this lang_decl - structure, the instance we found in the cache, must be the - same one associated with DECL. */ - ld = (struct lang_decl *) pph_cache_get (&stream->cache, ix); - gcc_assert (ld == DECL_LANG_SPECIFIC (decl)); - *is_merge_p = true; - } - else - { - gcc_assert (marker == PPH_RECORD_START); + return; - /* FIXME pph, we should not be getting a DECL_LANG_SPECIFIC - instance here. This is being allocated by - pph_in_merge_key_namespace_decl, but this should be the only - place where we allocate it. + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); - Change the if() below to: - gcc_assert (DECL_LANG_SPECIFIC (decl) == NULL); - */ - if (DECL_LANG_SPECIFIC (decl) == NULL) - { - /* Allocate a lang_decl structure for DECL. */ - retrofit_lang_decl (decl); - } - ld = DECL_LANG_SPECIFIC (decl); - /* Now register it. We would normally use ALLOC_AND_REGISTER, - but retrofit_lang_decl does not return a pointer. */ - pph_cache_insert_at (&stream->cache, ld, ix, PPH_lang_decl); - *is_merge_p = false; + is_merge = true; + if (DECL_LANG_SPECIFIC (decl) == NULL) + { + is_merge = false; + retrofit_lang_decl (decl); } - return ld; -} - - -/* Read language specific data in DECL from STREAM. */ - -static void -pph_in_lang_decl (pph_stream *stream, tree decl) -{ - struct lang_decl *ld; - struct lang_decl_base *ldb; - bool is_merge; - - ld = pph_in_lang_decl_start (stream, decl, &is_merge); - if (ld == NULL) - return; + ld = DECL_LANG_SPECIFIC (decl); /* Read all the fields in lang_decl_base. */ ldb = &ld->u.base; @@ -1724,16 +1657,11 @@ pph_in_sorted_fields_type (pph_stream *stream) marker = pph_in_start_record (stream, &image_ix, &ix, PPH_sorted_fields_type); if (marker == PPH_RECORD_END) return NULL; - else if (pph_is_reference_marker (marker)) - return (struct sorted_fields_type *) - pph_cache_find (stream, marker, image_ix, ix, PPH_sorted_fields_type); - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); num_fields = pph_in_uint (stream); - ALLOC_AND_REGISTER (&stream->cache, ix, PPH_sorted_fields_type, v, - sorted_fields_type_new (num_fields)); + v = sorted_fields_type_new (num_fields); for (i = 0; i < num_fields; i++) v->elts[i] = pph_in_tree (stream); @@ -1804,20 +1732,12 @@ pph_in_lang_type_class (pph_stream *stream, struct lang_type_class *ltc) ltc->vbases = pph_in_tree_vec (stream); marker = pph_in_start_record (stream, &image_ix, &ix, PPH_binding_table); - if (marker == PPH_RECORD_START) - { - ltc->nested_udts = pph_in_binding_table (stream); - pph_cache_insert_at (&stream->cache, ltc->nested_udts, ix, - PPH_binding_table); - } - else if (pph_is_reference_marker (marker)) - ltc->nested_udts = (binding_table) pph_cache_find (stream, marker, - image_ix, ix, - PPH_binding_table); + if (marker == PPH_RECORD_START_NO_CACHE) + ltc->nested_udts = pph_in_binding_table (stream); else { - /* Remove if we start emitting merge keys for this structure. */ gcc_assert (marker == PPH_RECORD_END); + ltc->nested_udts = NULL; } ltc->as_base = pph_in_tree (stream); @@ -1856,15 +1776,10 @@ pph_in_lang_type (pph_stream *stream) marker = pph_in_start_record (stream, &image_ix, &ix, PPH_lang_type); if (marker == PPH_RECORD_END) return NULL; - else if (pph_is_reference_marker (marker)) - return (struct lang_type *) pph_cache_find (stream, marker, image_ix, ix, - PPH_lang_type); - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); - ALLOC_AND_REGISTER (&stream->cache, ix, PPH_lang_type, lt, - ggc_alloc_cleared_lang_type (sizeof (struct lang_type))); + lt = ggc_alloc_cleared_lang_type (sizeof (struct lang_type)); pph_in_lang_type_header (stream, <->u.h); if (lt->u.h.is_lang_type_class) diff --git a/gcc/cp/pph-out.c b/gcc/cp/pph-out.c index a1ce886..8a984d9 100644 --- a/gcc/cp/pph-out.c +++ b/gcc/cp/pph-out.c @@ -478,6 +478,28 @@ pph_get_marker_for (pph_stream *stream, void *data, unsigned *include_ix_p, if (data == NULL) return PPH_RECORD_END; + /* Some structures should never be shared as they are already + members of shared structures. If the parent structure is shared, + the inner structure will be shared too. Additionally, this is a + problem when the parent structure has mutated from one PPH to the + other. If we allow the inner structure to be shared, we will + read the mutated state of the outer structure, but we will + happily re-use the data from the inner one (and miss mutated + state in it). */ + switch (tag) + { + case PPH_binding_table: + case PPH_function: + case PPH_lang_decl: + case PPH_lang_type: + case PPH_language_function: + case PPH_sorted_fields_type: + return PPH_RECORD_START_NO_CACHE; + + default: + break; + } + /* If DATA is a pre-loaded tree node, return a pre-loaded reference marker. */ if (pph_cache_lookup (NULL, data, ix_p, tag)) @@ -608,9 +630,11 @@ pph_out_start_record (pph_stream *stream, void *data, enum pph_tag tag) /* The caller will have to write a physical representation for DATA. */ gcc_assert (marker == PPH_RECORD_START + || marker == PPH_RECORD_START_NO_CACHE || marker == PPH_RECORD_START_MERGE_BODY); pph_out_record_marker (stream, marker, tag); - pph_out_uint (stream, ix); + if (marker != PPH_RECORD_START_NO_CACHE) + pph_out_uint (stream, ix); return marker; } @@ -1404,11 +1428,10 @@ pph_out_language_function (pph_stream *stream, struct language_function *lf) enum pph_record_marker marker; marker = pph_out_start_record (stream, lf, PPH_language_function); - if (pph_is_reference_or_end_marker (marker)) + if (marker == PPH_RECORD_END) return; - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); pph_out_tree_vec (stream, lf->base.x_stmt_tree.x_cur_stmt_list); pph_out_uint (stream, lf->base.x_stmt_tree.stmts_are_full_exprs_p); @@ -1462,11 +1485,10 @@ pph_out_struct_function (pph_stream *stream, struct function *fn) enum pph_record_marker marker; marker = pph_out_start_record (stream, fn, PPH_function); - if (pph_is_reference_or_end_marker (marker)) + if (marker == PPH_RECORD_END) return; - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); pph_out_tree (stream, fn->decl); output_struct_function_base (stream->encoder.w.ob, fn); @@ -1638,9 +1660,11 @@ pph_out_lang_decl (pph_stream *stream, tree decl) ld = DECL_LANG_SPECIFIC (decl); marker = pph_out_start_record (stream, ld, PPH_lang_decl); - if (pph_is_reference_or_end_marker (marker)) + if (marker == PPH_RECORD_END) return; + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); + /* Write all the fields in lang_decl_base. */ ldb = &ld->u.base; pph_out_ld_base (stream, ldb); @@ -1715,11 +1739,10 @@ pph_out_sorted_fields_type (pph_stream *stream, struct sorted_fields_type *sft) enum pph_record_marker marker; marker = pph_out_start_record (stream, sft, PPH_sorted_fields_type); - if (pph_is_reference_or_end_marker (marker)) + if (marker == PPH_RECORD_END) return; - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); pph_out_uint (stream, sft->len); for (i = 0; i < sft->len; i++) @@ -1790,10 +1813,9 @@ pph_out_lang_type_class (pph_stream *stream, struct lang_type_class *ltc) { enum pph_record_marker marker; marker = pph_out_start_record (stream, ltc->nested_udts, PPH_binding_table); - if (!pph_is_reference_or_end_marker (marker)) + if (marker != PPH_RECORD_END) { - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); pph_out_binding_table (stream, ltc->nested_udts); } } @@ -1830,11 +1852,10 @@ pph_out_lang_type (pph_stream *stream, tree type) lt = TYPE_LANG_SPECIFIC (type); marker = pph_out_start_record (stream, lt, PPH_lang_type); - if (pph_is_reference_or_end_marker (marker)) + if (marker == PPH_RECORD_END) return; - /* Remove if we start emitting merge keys for this structure. */ - gcc_assert (marker == PPH_RECORD_START); + gcc_assert (marker == PPH_RECORD_START_NO_CACHE); pph_out_lang_type_header (stream, <->u.h); if (lt->u.h.is_lang_type_class) diff --git a/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc b/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc index 4dbe174..e4c1344 100644 --- a/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc +++ b/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc @@ -1,6 +1,3 @@ -// pph asm xdiff 34830 -// The assembly difference is due to missing code in function f(). -// The PPH version removes the whole return expression. #include "x1namespace-alias1.h" #include "x1namespace-alias2.h" diff --git a/gcc/testsuite/g++.dg/pph/x6dynarray4.cc b/gcc/testsuite/g++.dg/pph/x6dynarray4.cc index b846312..82bae9d 100644 --- a/gcc/testsuite/g++.dg/pph/x6dynarray4.cc +++ b/gcc/testsuite/g++.dg/pph/x6dynarray4.cc @@ -1,5 +1,4 @@ // { dg-xfail-if "BOGUS MERGING" { "*-*-*" } { "-fpph-map=pph.map" } } -// { dg-bogus ".*internal compiler error.*" "" { xfail *-*-* } 0 } // Too many failures to diagnose. #include "x6dynarray5.h" diff --git a/gcc/testsuite/g++.dg/pph/x7dynarray5.cc b/gcc/testsuite/g++.dg/pph/x7dynarray5.cc index 8e23edf..740c10d 100644 --- a/gcc/testsuite/g++.dg/pph/x7dynarray5.cc +++ b/gcc/testsuite/g++.dg/pph/x7dynarray5.cc @@ -1,5 +1,4 @@ // { dg-xfail-if "BOGUS POSSIBLY DROPPING SYMBOLS " { "*-*-*" } { "-fpph-map=pph.map" } } -// { dg-bogus ".*internal compiler error.*" "" { xfail *-*-* } 0 } #include "x0dynarray4.h" #include "x6dynarray5.h"