Issue 5142049: [pph] Prepare for mutation detection [2/3] (Closed)
|
Created:
12 years, 9 months ago by Diego Novillo Modified:
12 years, 8 months ago Reviewers:
gcharette1 CC:
Lawrence Crowl, gcc-patches_gcc.gnu.org Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 4
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. */
--
1.7.3.1
--
This patch is available for review at http://codereview.appspot.com/5142049
Sign in to reply to this message.
More comments to come on [3/3], for now just a single comment below on
this specific patch:
> 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);
> + }
Seems like you replaced the pph_cache_get one liners with these
two-liners. Wouldn't a simple inline function be nicer for this?
Gab
Sign in to reply to this message.
On Wed, Sep 28, 2011 at 17:23, Gabriel Charette <gcharette1@gmail.com> wrote: > More comments to come on [3/3], for now just a single comment below on > this specific patch: > >> 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); >> + } > > Seems like you replaced the pph_cache_get one liners with these > two-liners. Wouldn't a simple inline function be nicer for this? I call them separately. Or do you mean a single call that combines them for the common case?
Sign in to reply to this message.
On Wed, Sep 28, 2011 at 5:31 PM, Diego Novillo <dnovillo@google.com> wrote: > On Wed, Sep 28, 2011 at 17:23, Gabriel Charette <gcharette1@gmail.com> wrote: >> More comments to come on [3/3], for now just a single comment below on >> this specific patch: >> >>> 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); >>> + } >> >> Seems like you replaced the pph_cache_get one liners with these >> two-liners. Wouldn't a simple inline function be nicer for this? > > I call them separately. Or do you mean a single call that combines > them for the common case? > Yes that's what I mean, a single call that combines both, since that's the common usage and I feel there should be as little cache code at the top of every pph_* function (in particular, every time a new pph streaming function is added all that code needs to be "duplicated", so the less code the better imo).
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
