Re-adding gcc-patches (forgot to send plain text last time...sigh!)
On Tue, Jul 12, 2011 at 10:56 AM, Gabriel Charette <gch...@google.com> wrote:
> I like this implementation!
> Only one thing, if we ACTUALLY want "to_register" NULL instead of the read
> value we can't as in your current implementation NULL means don't do the
> alternate registration.
> Could that be a problem or do we expect the structures we pass to this to
> always be non-null? (i.e. for scope_chain->bindings this is definitely ok,
> but what about other potential uses?), if we keep it this way we should at
> least make it clear in the function's comment that NULL means don't do
> alternate registration I think.
> Gab
>
> On Mon, Jul 11, 2011 at 2:42 PM, Diego Novillo <dnovi...@google.com> wrote:
>>
>> This patch adapts an idea from Gab that allow us to register alternate
>> addresses in the cache. The problem here is making sure that symbols
>> read from a PPH file reference the right bindings.
>>
>> If a symbol is in the global namespace when compiling a header file,
>> its bindings will point to NAMESPACE_LEVEL(global_namespace)->bindings,
>> but that global_namespace is the global_namespace instantiated for the
>> header file. When reading that PPH image from a translation unit, we
>> need to refer to the bindings of the *current* global_namespace.
>>
>> In general we solve this by inserting the pointer in the streamer
>> cache. For instance, to avoid instantiating a second global_namespace
>> decl, the initialization code of both the writer and the reader store
>> global_namespace into the streaming cache. This way, all the
>> references to global_namespace point to the current global_namespace
>> as known by the writer and the reader.
>>
>> However, we cannot use the same trick on the bindings for
>> global_namespace. If we simply inserted it into the cache then
>> writing out NAMESPACE_LEVEL(global_namespace)->bindings would simply
>> write a reference to the current one and on the reader side, it would
>> simply restore a pointer to the current translation unit's bindings.
>> Without ever actually writing or reading anything (since it was
>> satisified from the cache).
>>
>> Therefore, we want a mechanism that allows the reader to: (a) read all
>> the symbols in the global bindings, and (b) references to the
>> global binding made by the symbols should point to the global bindings
>> of the current translation unit (instead of the one in the PPH image).
>>
>> That's where ALLOC_AND_REGISTER_ALTERNATE comes in. When called, it
>> allocates the data structure but registers another pointer in the
>> cache. We use this trick when calling pph_in_binding_level from the
>> toplevel:
>>
>> + new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>>
>> This way, when pph_in_binding_level tries to allocate the binding
>> structure read from STREAM, it registers scope_chain->bindings in the
>> cache. This way, references to the original file's global binding are
>> automatically redirected to the current translation unit's global
>> bindings.
>>
>> Gab, I modified your original implementation to move all the logic to
>> the place where we need to make this decision. This way, it is easier
>> to tell which functions need this alternate registration, instead of
>> relying on some status flag squirreled away in the STREAM data
>> structure.
>>
>>
>> Tested on x86_64. Applied to branch.
>>
>> 2011-07-11 Diego Novillo <dnovi...@google.com>
>> Gabriel Charette <gch...@google.com>
>>
>> * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
>> (pph_in_binding_level): Add argument TO_REGISTER. Call
>> ALLOC_AND_REGISTER_ALTERNATE if set.
>> Update all users.
>> (pph_register_decls_in_symtab): Call varpool_finalize_decl
>> on all file-local symbols.
>> (pph_in_scope_chain): Call pph_in_binding_level with
>> scope_chain->bindings as the alternate pointer to
>> register in the streaming cache.
>>
>> diff --git a/gcc/cp/ChangeLog.pph b/gcc/cp/ChangeLog.pph
>> index 1011902..f18c2f4 100644
>> --- a/gcc/cp/ChangeLog.pph
>> +++ b/gcc/cp/ChangeLog.pph
>> @@ -1,3 +1,16 @@
>> +2011-07-11 Diego Novillo <dnovi...@google.com>
>> + Gabriel Charette <gch...@google.com>
>> +
>> + * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
>> + (pph_in_binding_level): Add argument TO_REGISTER. Call
>> + ALLOC_AND_REGISTER_ALTERNATE if set.
>> + Update all users.
>> + (pph_register_decls_in_symtab): Call varpool_finalize_decl
>> + on all file-local symbols.
>> + (pph_in_scope_chain): Call pph_in_binding_level with
>> + scope_chain->bindings as the alternate pointer to
>> + register in the streaming cache.
>> +
>> 2011-07-07 Diego Novillo <dnovi...@google.com>
>>
>> * pph-streamer-in.c (pph_register_decls_in_symtab): Rename
>> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
>> index 571ebf5..903cd94 100644
>> --- a/gcc/cp/pph-streamer-in.c
>> +++ b/gcc/cp/pph-streamer-in.c
>> @@ -42,6 +42,18 @@ along with GCC; see the file COPYING3. If not see
>> pph_register_shared_data (STREAM, DATA, IX); \
>> } while (0)
>>
>> +/* Same as ALLOC_AND_REGISTER, but instead of registering DATA into the
>> + cache at slot IX, it registers ALT_DATA. Used to support mapping
>> + pointers to global data in the original STREAM that need to point
>> + to a different instance when aggregating individual PPH files into
>> + the current translation unit (see pph_in_binding_level for an
>> + example). */
>> +#define ALLOC_AND_REGISTER_ALTERNATE(STREAM, IX, DATA, ALLOC_EXPR,
>> ALT_DATA)\
>> + do { \
>> + (DATA) = (ALLOC_EXPR); \
>> + pph_register_shared_data (STREAM, ALT_DATA, IX); \
>> + } while (0)
>> +
>> /* Callback for unpacking value fields in ASTs. BP is the bitpack
>> we are unpacking from. EXPR is the tree to unpack. */
>>
>> @@ -482,7 +494,8 @@ pph_in_qual_use_vec (pph_stream *stream)
>>
>>
>> /* Forward declaration to break cyclic dependencies. */
>> -static struct cp_binding_level *pph_in_binding_level (pph_stream *);
>> +static struct cp_binding_level *pph_in_binding_level (pph_stream *,
>> + struct
>> cp_binding_level *);
>>
>> /* Helper for pph_in_cxx_binding. Read and return a cxx_binding
>> instance from STREAM. */
>> @@ -505,7 +518,7 @@ pph_in_cxx_binding_1 (pph_stream *stream)
>> value = pph_in_tree (stream);
>> type = pph_in_tree (stream);
>> ALLOC_AND_REGISTER (stream, ix, cb, cxx_binding_make (value, type));
>> - cb->scope = pph_in_binding_level (stream);
>> + cb->scope = pph_in_binding_level (stream, NULL);
>> bp = pph_in_bitpack (stream);
>> cb->value_is_inherited = bp_unpack_value (&bp, 1);
>> cb->is_local = bp_unpack_value (&bp, 1);
>> @@ -581,10 +594,26 @@ pph_in_label_binding (pph_stream *stream)
>> }
>>
>>
>> -/* Read and return an instance of cp_binding_level from STREAM. */
>> +/* Read and return an instance of cp_binding_level from STREAM.
>> + TO_REGISTER is used when the caller wants to read a binding level,
>> + but register a different binding level in the streaming cache.
>> + This is needed when reading the binding for global_namespace.
>> +
>> + When the file was created global_namespace was localized to that
>> + particular file. However, when instantiating a PPH file into
>> + memory, we are now using the global_namespace for the current
>> + translation unit. Therefore, every reference to the
>> + global_namespace and its bindings should be pointing to the
>> + CURRENT global namespace, not the one in STREAM.
>> +
>> + Therefore, if TO_REGISTER is set, and we read a binding level from
>> + STREAM for the first time, instead of registering the newly
>> + instantiated binding level into the cache, we register the binding
>> + level given in TO_REGISTER. This way, subsequent references to the
>> + global binding level will be done to the one set in TO_REGISTER. */
>>
>> static struct cp_binding_level *
>> -pph_in_binding_level (pph_stream *stream)
>> +pph_in_binding_level (pph_stream *stream, struct cp_binding_level
>> *to_register)
>> {
>> unsigned i, num, ix;
>> struct cp_binding_level *bl;
>> @@ -597,7 +626,14 @@ pph_in_binding_level (pph_stream *stream)
>> else if (marker == PPH_RECORD_SHARED)
>> return (struct cp_binding_level *) pph_in_shared_data (stream, ix);
>>
>> - ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level
>> ());
>> + /* If TO_REGISTER is set, register that binding level instead of the
>> newly
>> + allocated binding level into slot IX. */
>> + if (to_register == NULL)
>> + ALLOC_AND_REGISTER (stream, ix, bl,
>> ggc_alloc_cleared_cp_binding_level ());
>> + else
>> + ALLOC_AND_REGISTER_ALTERNATE (stream, ix, bl,
>> + ggc_alloc_cleared_cp_binding_level (),
>> + to_register);
>>
>> bl->names = pph_in_chain (stream);
>> bl->namespaces = pph_in_chain (stream);
>> @@ -627,7 +663,7 @@ pph_in_binding_level (pph_stream *stream)
>>
>> bl->blocks = pph_in_chain (stream);
>> bl->this_entity = pph_in_tree (stream);
>> - bl->level_chain = pph_in_binding_level (stream);
>> + bl->level_chain = pph_in_binding_level (stream, NULL);
>> bl->dead_vars_from_for = pph_in_tree_vec (stream);
>> bl->statement_list = pph_in_chain (stream);
>> bl->binding_depth = pph_in_uint (stream);
>> @@ -713,7 +749,7 @@ pph_in_language_function (pph_stream *stream)
>>
>> /* FIXME pph. We are not reading lf->x_named_labels. */
>>
>> - lf->bindings = pph_in_binding_level (stream);
>> + lf->bindings = pph_in_binding_level (stream, NULL);
>> lf->x_local_names = pph_in_tree_vec (stream);
>>
>> /* FIXME pph. We are not reading lf->extern_decl_map. */
>> @@ -854,7 +890,7 @@ pph_in_struct_function (pph_stream *stream, tree decl)
>> static void
>> pph_in_ld_ns (pph_stream *stream, struct lang_decl_ns *ldns)
>> {
>> - ldns->level = pph_in_binding_level (stream);
>> + ldns->level = pph_in_binding_level (stream, NULL);
>> }
>>
>>
>> @@ -1140,10 +1176,10 @@ pph_in_lang_type (pph_stream *stream)
>>
>>
>> /* Register all the symbols in binding level BL in the callgraph symbol
>> - table. NS is the namespace where all the symbols in BL live. */
>> + table. */
>>
>> static void
>> -pph_register_decls_in_symtab (struct cp_binding_level *bl, tree ns)
>> +pph_register_decls_in_symtab (struct cp_binding_level *bl)
>> {
>> tree t;
>>
>> @@ -1153,18 +1189,16 @@ pph_register_decls_in_symtab (struct
>> cp_binding_level *bl, tree ns)
>> bl->namespaces = nreverse (bl->namespaces);
>>
>> for (t = bl->names; t; t = DECL_CHAIN (t))
>> - if (DECL_NAME (t) && IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t)))
>> - {
>> - cxx_binding *b = IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t));
>> - b->scope = NAMESPACE_LEVEL (ns);
>> -
>> - if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
>> (t))
>> - varpool_finalize_decl (t);
>> - }
>> + {
>> + /* Add file-local symbols to the varpool. */
>> + if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
>> (t))
>> + varpool_finalize_decl (t);
>> + }
>>
>> + /* Recurse into the namespaces contained in BL. */
>> for (t = bl->namespaces; t; t = DECL_CHAIN (t))
>> if (NAMESPACE_LEVEL (t))
>> - pph_register_decls_in_symtab (NAMESPACE_LEVEL (t), t);
>> + pph_register_decls_in_symtab (NAMESPACE_LEVEL (t));
>> }
>>
>>
>> @@ -1173,18 +1207,21 @@ pph_register_decls_in_symtab (struct
>> cp_binding_level *bl, tree ns)
>> static void
>> pph_in_scope_chain (pph_stream *stream)
>> {
>> - struct saved_scope *file_scope_chain;
>> unsigned i;
>> tree decl;
>> cp_class_binding *cb;
>> cp_label_binding *lb;
>> struct cp_binding_level *cur_bindings, *new_bindings;
>>
>> - file_scope_chain = ggc_alloc_cleared_saved_scope ();
>> - file_scope_chain->bindings = new_bindings = pph_in_binding_level
>> (stream);
>> + /* When reading the symbols in STREAM's global binding level, make
>> + sure that references to the global binding level point to
>> + scope_chain->bindings. Otherwise, identifiers read from STREAM
>> + will have the wrong bindings and will fail name lookups. */
>> cur_bindings = scope_chain->bindings;
>> + new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>>
>> - pph_register_decls_in_symtab (new_bindings, global_namespace);
>> + /* Register all the symbols in STREAM with the call graph. */
>> + pph_register_decls_in_symtab (new_bindings);
>>
>> /* Merge the bindings from STREAM into saved_scope->bindings. */
>> chainon (cur_bindings->names, new_bindings->names);
>>
>>
>> --
>> This patch is available for review at
>> http://codereview.appspot.com/4685054
>
>
