From patchwork Tue Sep 27 16:54:54 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Diego Novillo X-Patchwork-Id: 116632 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 84DB6B6F6F for ; Wed, 28 Sep 2011 02:55:24 +1000 (EST) Received: (qmail 9995 invoked by alias); 27 Sep 2011 16:55:19 -0000 Received: (qmail 9965 invoked by uid 22791); 27 Sep 2011 16:55:13 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_CX, TW_FN X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.44.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Sep 2011 16:54:57 +0000 Received: from wpaz17.hot.corp.google.com (wpaz17.hot.corp.google.com [172.24.198.81]) by smtp-out.google.com with ESMTP id p8RGsu5w027948; Tue, 27 Sep 2011 09:54:56 -0700 Received: from topo.tor.corp.google.com (topo.tor.corp.google.com [172.29.41.2]) by wpaz17.hot.corp.google.com with ESMTP id p8RGssmc031825; Tue, 27 Sep 2011 09:54:55 -0700 Received: by topo.tor.corp.google.com (Postfix, from userid 54752) id 91CDD1DA1C7; Tue, 27 Sep 2011 12:54:54 -0400 (EDT) To: reply@codereview.appspotmail.com, crowl@google.com, gcc-patches@gcc.gnu.org Subject: [pph] Prepare for mutation detection [2/3] (issue5142049) Message-Id: <20110927165454.91CDD1DA1C7@topo.tor.corp.google.com> Date: Tue, 27 Sep 2011 12:54:54 -0400 (EDT) From: dnovillo@google.com (Diego Novillo) X-System-Of-Record: true 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 The second patch re-factors pph_cache_get to remove the cache selection logic into a separate function. I needed this to implement pph_cache_sign in terms of pph_cache_get. Before this, pph_cache_get tried to decide what cache to use. After this patch, a separate pph_cache_select() function makes that decision, which simplifies pph_cache_get. I also added a pointer to the global pph_preloaded_cache into pph_stream. This way, every stream can refer to the same preloaded cache without having to expose it as a global. The patch also starts cleaning up the streaming of builtins and constants. These were a source of problems because they can trick the cache. In the case of builtins, when an expression E is written as a builtin, we lookup the cache using E but we write out the associated builtin B. Later on, we will try to write B itself, since B was not in the cache we add it. When the reader tries to instantiate the image, it will never find E, it will find B in *two* different cache slots. Since neither builtins nor constants are worth saving in the cache (their representation is smaller than a cache reference), we skip them altogether. This implementation is a bit dodgy. Cleaned up in part 3. Tested on x86_64. Committed to branch. * pph-streamer-in.c (pph_read_namespace_tree): Do not insert builtins into the cache. Call pph_cache_sign for decls and types. * pph-streamer-out.c (pph_write_builtin): New. (pph_write_namespace_tree): Call it. Handle builtins before calling pph_out_start_record. * pph-streamer.c (pph_init_preloaded_cache): Add comment. (pph_stream_open): Initialize preloaded_cache field. (pph_cache_insert_at): Assert that *MAP_SLOT is NULL. Disable for now. (pph_cache_sign): New. (pph_cache_get): Move to pph-streamer.h. * pph-streamer.h (pph_cache_entry): Change type of field crc_nbytes to size_t. (pph_stream): Add field preloaded_cache. (pph_cache_select): Factor out of ... (pph_cache_get): ... here. Change all callers to call pph_cache_select first. (pph_cache_get_entry): New. diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c index 0bd4d64..b267833 100644 --- a/gcc/cp/pph-streamer-in.c +++ b/gcc/cp/pph-streamer-in.c @@ -439,7 +439,10 @@ pph_in_cxx_binding_1 (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cxx_binding *) pph_cache_get (&stream->cache, image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (cxx_binding *) pph_cache_get (cache, ix); + } value = pph_in_tree (stream); type = pph_in_tree (stream); @@ -487,8 +490,10 @@ pph_in_class_binding (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_class_binding *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (cp_class_binding *) pph_cache_get (cache, ix); + } ALLOC_AND_REGISTER (&stream->cache, ix, cb, ggc_alloc_cleared_cp_class_binding ()); @@ -512,8 +517,10 @@ pph_in_label_binding (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_label_binding *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (cp_label_binding *) pph_cache_get (cache, ix); + } ALLOC_AND_REGISTER (&stream->cache, ix, lb, ggc_alloc_cleared_cp_label_binding ()); @@ -555,8 +562,10 @@ pph_in_binding_level (pph_stream *stream, cp_binding_level *to_register) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_binding_level *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (cp_binding_level *) pph_cache_get (cache, ix); + } /* If TO_REGISTER is set, register that binding level instead of the newly allocated binding level into slot IX. */ @@ -642,8 +651,10 @@ pph_in_c_language_function (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct c_language_function *) pph_cache_get (&stream->cache, - image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (struct c_language_function *) pph_cache_get (cache, ix); + } ALLOC_AND_REGISTER (&stream->cache, ix, clf, ggc_alloc_cleared_c_language_function ()); @@ -668,8 +679,10 @@ pph_in_language_function (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct language_function *) pph_cache_get (&stream->cache, - image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (struct language_function *) pph_cache_get (cache, ix); + } ALLOC_AND_REGISTER (&stream->cache, ix, lf, ggc_alloc_cleared_language_function ()); @@ -763,8 +776,8 @@ pph_in_struct_function (pph_stream *stream, tree decl) return; else if (pph_is_reference_marker (marker)) { - fn = (struct function *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + fn = (struct function *) pph_cache_get (cache, ix); gcc_assert (DECL_STRUCT_FUNCTION (decl) == fn); return; } @@ -873,9 +886,9 @@ pph_in_lang_specific (pph_stream *stream, tree decl) return; else if (pph_is_reference_marker (marker)) { - DECL_LANG_SPECIFIC (decl) = - (struct lang_decl *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + DECL_LANG_SPECIFIC (decl) = (struct lang_decl *) pph_cache_get (cache, + ix); return; } @@ -969,8 +982,10 @@ pph_in_sorted_fields_type (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct sorted_fields_type *) pph_cache_get (&stream->cache, - image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (struct sorted_fields_type *) pph_cache_get (cache, ix); + } num_fields = pph_in_uint (stream); ALLOC_AND_REGISTER (&stream->cache, ix, v, @@ -1051,8 +1066,10 @@ pph_in_lang_type_class (pph_stream *stream, struct lang_type_class *ltc) pph_cache_insert_at (&stream->cache, ltc->nested_udts, ix); } else if (pph_is_reference_marker (marker)) - ltc->nested_udts = (binding_table) pph_cache_get (&stream->cache, - image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + ltc->nested_udts = (binding_table) pph_cache_get (cache, ix); + } ltc->as_base = pph_in_tree (stream); ltc->pure_virtuals = pph_in_tree_vec (stream); @@ -1091,8 +1108,10 @@ pph_in_lang_type (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct lang_type *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (struct lang_type *) pph_cache_get (cache, ix); + } ALLOC_AND_REGISTER (&stream->cache, ix, lt, ggc_alloc_cleared_lang_type (sizeof (struct lang_type))); @@ -1306,8 +1325,10 @@ pph_in_cgraph_node (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct cgraph_node *) pph_cache_get (&stream->cache, image_ix, ix, - marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (struct cgraph_node *) pph_cache_get (cache, ix); + } fndecl = pph_in_tree (stream); ALLOC_AND_REGISTER (&stream->cache, ix, node, cgraph_create_node (fndecl)); @@ -2094,7 +2115,10 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (tree) pph_cache_get (&stream->cache, image_ix, ix, marker); + { + pph_cache *cache = pph_cache_select (stream, marker, image_ix); + return (tree) pph_cache_get (cache, ix); + } /* We did not find the tree in the pickle cache, allocate the tree by reading the header fields (different tree nodes need to be @@ -2104,9 +2128,9 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace) if (tag == LTO_builtin_decl) { /* If we are going to read a built-in function, all we need is - the code and class. */ + the code and class. Note that builtins are never stored in + the pickle cache. */ expr = streamer_get_builtin_tree (ib, data_in); - pph_cache_insert_at (&stream->cache, expr, ix); } else if (tag == lto_tree_code_to_tag (INTEGER_CST)) { @@ -2134,7 +2158,12 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace) } pph_cache_insert_at (&stream->cache, expr, ix); pph_read_tree_body (stream, expr); + + /* If needed, sign the recently materialized tree to detect mutations. */ + if (DECL_P (expr) || TYPE_P (expr)) + pph_cache_sign (&stream->cache, ix, tree_size (expr)); } + return expr; } diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c index 23597a1..7157275 100644 --- a/gcc/cp/pph-streamer-out.c +++ b/gcc/cp/pph-streamer-out.c @@ -1909,6 +1909,21 @@ pph_write_tree_header (pph_stream *stream, tree expr) } +/* Write builtin EXPR to STREAM. MD and NORMAL builtins do not need + to be written out completely as they are always instantiated by the + compiler on startup. The only builtins that need to be written out + are BUILT_IN_FRONTEND. For all other builtins, we simply write the + class and code. */ + +static void +pph_write_builtin (pph_stream *stream, tree expr) +{ + pph_out_record_marker (stream, PPH_RECORD_START); + pph_out_uint (stream, -1); + streamer_write_builtin (stream->encoder.w.ob, expr); +} + + /* Callback for writing ASTs to a stream. Write EXPR to the PPH stream in OB. */ @@ -1923,21 +1938,35 @@ void pph_write_namespace_tree (pph_stream *stream, tree expr, tree enclosing_namespace ) { + /* Handle builtins first. We do not want builtins in the cache for + two reasons: + + - They never need pickling. Much like pre-loaded tree nodes, + builtins are pre-built by the compiler and accessible with + their class and code. + + - They can trick the cache. When possible, user-provided + functions are generally replaced by their builtin counterparts + (e.g., strcmp, malloc, etc). When this happens, the writer + cache will store in its cache two different expressions, one + for the user provided function and another for the builtin + itself. However, when written out to the PPH image, they both + get emitted as a reference to the builtin. Therefore, when the + reader tries to instantiate the second copy, it tries to store + the same tree in two different cache slots (and proceeds to ICE + in pph_cache_insert_at). */ + if (expr && streamer_handle_as_builtin_p (expr)) + { + pph_write_builtin (stream, expr); + return; + } + /* If EXPR is NULL or it already existed in the pickle cache, nothing else needs to be done. */ if (!pph_out_start_record (stream, expr)) return; - if (streamer_handle_as_builtin_p (expr)) - { - /* MD and NORMAL builtins do not need to be written out - completely as they are always instantiated by the - compiler on startup. The only builtins that need to - be written out are BUILT_IN_FRONTEND. For all other - builtins, we simply write the class and code. */ - streamer_write_builtin (stream->encoder.w.ob, expr); - } - else if (TREE_CODE (expr) == INTEGER_CST) + if (TREE_CODE (expr) == INTEGER_CST) { /* INTEGER_CST nodes are special because they need their original type to be materialized by the reader (to implement diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c index c3dee7a..c212790 100644 --- a/gcc/cp/pph-streamer.c +++ b/gcc/cp/pph-streamer.c @@ -108,6 +108,9 @@ pph_cache_init (pph_cache *cache) } +/* Initialize the pre-loaded cache. This contains all the common + tree nodes built by the compiler on startup. */ + void pph_init_preloaded_cache (void) { @@ -137,6 +140,7 @@ pph_stream_open (const char *name, const char *mode) stream->name = xstrdup (name); stream->write_p = (strchr (mode, 'w') != NULL); pph_cache_init (&stream->cache); + stream->preloaded_cache = pph_preloaded_cache; if (stream->write_p) pph_init_write (stream); else @@ -411,18 +415,16 @@ pph_cache_insert_at (pph_cache *cache, void *data, unsigned ix) pph_cache_entry e = { data, 0, 0 }; map_slot = pointer_map_insert (cache->m, data); - if (*map_slot == NULL) - { - *map_slot = (void *) (intptr_t) ix; - if (ix + 1 > VEC_length (pph_cache_entry, cache->v)) - VEC_safe_grow_cleared (pph_cache_entry, heap, cache->v, ix + 1); - VEC_replace (pph_cache_entry, cache->v, ix, &e); - } + #if 0 - /* FIXME pph. Re-add after mutated references are implemented. */ - else - gcc_assert (ix == (intptr_t) *((intptr_t **) map_slot)); + /* We should not be trying to insert the same data more than once. */ + gcc_assert (*map_slot == NULL); #endif + + *map_slot = (void *) (intptr_t) ix; + if (ix + 1 > VEC_length (pph_cache_entry, cache->v)) + VEC_safe_grow_cleared (pph_cache_entry, heap, cache->v, ix + 1); + VEC_replace (pph_cache_entry, cache->v, ix, &e); } @@ -533,36 +535,20 @@ pph_cache_add (pph_cache *cache, void *data, unsigned *ix_p) } -/* Return the pointer at slot IX in STREAM_CACHE if MARKER is an IREF. - If MARKER is a XREF then instead of looking up in STREAM_CACHE, the - pointer is looked up in the CACHE of pph_read_images[INCLUDE_IX]. - Finally if MARKER is a PREF return the pointer at slot IX in the - preloaded cache. */ +/* Generate a CRC32 signature for the first NBYTES of the area memory + pointed to by slot IX of CACHE. The signature is stored in + CACHE[IX] and returned. */ -void * -pph_cache_get (pph_cache *stream_cache, unsigned include_ix, unsigned ix, - enum pph_record_marker marker) +unsigned +pph_cache_sign (pph_cache *cache, unsigned ix, size_t nbytes) { pph_cache_entry *e; - pph_cache *cache; - /* Determine which cache to use. */ - switch (marker) - { - case PPH_RECORD_IREF: - cache = stream_cache; - break; - case PPH_RECORD_XREF: - cache = &VEC_index (pph_stream_ptr, pph_read_images, include_ix)->cache; - break; - case PPH_RECORD_PREF: - cache = pph_preloaded_cache; - break; - default: - gcc_unreachable (); - } + gcc_assert (nbytes == (size_t) (int) nbytes); + + e = pph_cache_get_entry (cache, ix); + e->crc = xcrc32 ((const unsigned char *) e->data, nbytes, -1); + e->crc_nbytes = nbytes; - e = VEC_index (pph_cache_entry, cache->v, ix); - gcc_assert (e); - return e->data; + return e->crc; } diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h index 2a1ef8b..28b698f 100644 --- a/gcc/cp/pph-streamer.h +++ b/gcc/cp/pph-streamer.h @@ -129,10 +129,10 @@ typedef struct pph_cache_entry { /* Checksum information for DATA. */ unsigned int crc; - /* Length of the checksummed area pointed by DATA. Note that this - is *not* the size of the memory area pointed by DATA, just the - number of bytes in DATA that we have checksummed. */ - unsigned int crc_nbytes; + /* Length in bytes of the checksummed area pointed by DATA. Note + that this is *not* the size of the memory area pointed by DATA, + just the number of bytes in DATA that we have checksummed. */ + size_t crc_nbytes; } pph_cache_entry; DEF_VEC_O(pph_cache_entry); @@ -246,6 +246,12 @@ typedef struct pph_stream { /* Cache of pickled data structures. */ pph_cache cache; + /* Pointer to the pre-loaded cache. This cache contains all the + trees that are always built by the compiler on startup (and + thus need not be pickled). This cache is shared by all the + pph_stream objects. */ + pph_cache *preloaded_cache; + /* Nonzero if the stream was opened for writing. */ unsigned int write_p : 1; @@ -279,7 +285,7 @@ void pph_cache_insert_at (pph_cache *, void *, unsigned); bool pph_cache_lookup (pph_cache *, void *, unsigned *); bool pph_cache_lookup_in_includes (void *, unsigned *, unsigned *); bool pph_cache_add (pph_cache *, void *, unsigned *); -void *pph_cache_get (pph_cache *, unsigned, unsigned, enum pph_record_marker); +unsigned pph_cache_sign (pph_cache *, unsigned, size_t); /* In pph-streamer-out.c. */ void pph_flush_buffers (pph_stream *); @@ -330,6 +336,46 @@ pph_enabled_p (void) return pph_writer_enabled_p () || pph_reader_enabled_p (); } +/* Return the pickle cache in STREAM corresponding to MARKER. + if MARKER is PPH_RECORD_IREF, it returns the cache in STREAM itself. + If MARKER is PPH_RECORD_XREF, it returns the cache in + pph_read_images[INCLUDE_IX]. + If MARKER is a PREF, it returns the preloaded cache. */ +static inline pph_cache * +pph_cache_select (pph_stream *stream, enum pph_record_marker marker, + unsigned include_ix) +{ + switch (marker) + { + case PPH_RECORD_IREF: + return &stream->cache; + break; + case PPH_RECORD_XREF: + return &VEC_index (pph_stream_ptr, pph_read_images, include_ix)->cache; + break; + case PPH_RECORD_PREF: + return stream->preloaded_cache; + break; + default: + gcc_unreachable (); + } +} + +/* Return the data pointer at slot IX in CACHE */ +static inline void * +pph_cache_get (pph_cache *cache, unsigned ix) +{ + pph_cache_entry *e = VEC_index (pph_cache_entry, cache->v, ix); + gcc_assert (e); + return e->data; +} + +/* Return entry IX in CACHE. */ +static inline pph_cache_entry * +pph_cache_get_entry (pph_cache *cache, unsigned ix) +{ + return VEC_index (pph_cache_entry, cache->v, ix); +} /* Output array A of cardinality C of ASTs to STREAM. */ /* FIXME pph: hold for alternate routine. */