[pph] Stream first/weak_object_global_name (issue4641086)

first/weak_global_object_name are part of the global state.

This seems to be used to produce the assembler names. They are set only once as early as possible in the parsing; thus we should define it to be whatever it was in the first pph (or even in the C file if it was set before any pph was even loaded.

I'm not sure exactly what this does, but trying to find a fix for the asm diffs this is the first thing that I hit in the compiler logic that was different in the good compiler vs the pph compiler. With this fix this bit of logic is now correct.
However, this doesn't fix any pph test (nor does it even change any pph asm (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).

Tested with bootstrap build and pph regression testing.

2011-06-30  Gabriel Charette  <gchare@google.com>

	* Make-lang.in (pph-streamer-out.o): Add dependence on output.h
	(pph-streamer-in.o): Add dependence on output.h
	* pph-streamer-in.c (pph_read_file_contents): Stream in
	first_global_object_name and weak_global_object_name.
	* gcc/cp/pph-streamer-out.c (pph_write_file_contents): Stream out
	first_global_object_name and weak_global_object_name.


--
This patch is available for review at http://codereview.appspot.com/4641086

On Thu, Jun 30, 2011 at 16:24, Gabriel Charette <gchare@google.com> wrote:
> first/weak_global_object_name are part of the global state.
>
> This seems to be used to produce the assembler names. They are set only once as early as possible in the parsing; thus we should define it to be whatever it was in the first pph (or even in the C file if it was set before any pph was even loaded.
>
> I'm not sure exactly what this does, but trying to find a fix for the asm diffs this is the first thing that I hit in the compiler logic that was different in the good compiler vs the pph compiler. With this fix this bit of logic is now correct.
> However, this doesn't fix any pph test (nor does it even change any pph asm (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).

This does not look right to me.  These two symbols are set when
calling notice_global_symbol, which is done during code generation
(you will see it called from cgraph_finalize_function,
cgraph_mark_reachable_node, etc).

You are probably close to the cause of this relative ordering problem,
but streaming these two globals is not the solution.  They both should
be set in the same order by both compilers, but not by forcing them
this way.

One thing that may be happening here is that the order in which we
call cgraph_finalize_function is different in the two compilers.
That's what needs to change.  One thing you could do is set a
breakpoint in notice_global_symbol.  The two compilers should be
calling it in the same order.


Diego.

notice_global_symbol is actually called in the parser (before we stream out).

I just confirmed this by setting a break point on the function in the
pph generation of c1varoder.pph and hitting the function twice for the
two variables defined in c1varorder.h

It looks like rtl and assembler_name are set BEFORE we stream out...
so that we would need to stream them (looks like assembler_name is
already streamed, but not rtl...).

Not streaming rtl causes it to be recreated later (which i think might
be the cause of the bad ordering).

Now another problem is that when two pph are generated, they are not
aware of each other (and of their respective
first/weak_global_object_name...) so even if we stream those
variables, their could still be conflicts (and/or asm diffs..)

However, I'm not sure about how make_decl_rtl works, but if it can
regenerate the same rtl we had before (and even potentially solve the
conflicts mentioned in the previous paragraph), we may not need to
actually stream the rtl, we would just need to fix the ordering as the
pph rtl's are being regenerated.

(they are regenerated because DECL_RTL(NODE) either returns
(NODE)->decl_with_rtl.rtl or calls make_decl_rtl(NODE) if it's
NULL...)

If we do this, I think we wanna have the first/weak_object_global_name
set to what it should be by streaming it in from the first pph, to
ensure all the rtl and assembler_name choices are made correctly (I'm
not sure I understand all of this, but it seems to rely on those
fields).

Gab

On Thu, Jun 30, 2011 at 4:33 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Jun 30, 2011 at 16:24, Gabriel Charette <gchare@google.com> wrote:
>> first/weak_global_object_name are part of the global state.
>>
>> This seems to be used to produce the assembler names. They are set only once as early as possible in the parsing; thus we should define it to be whatever it was in the first pph (or even in the C file if it was set before any pph was even loaded.
>>
>> I'm not sure exactly what this does, but trying to find a fix for the asm diffs this is the first thing that I hit in the compiler logic that was different in the good compiler vs the pph compiler. With this fix this bit of logic is now correct.
>> However, this doesn't fix any pph test (nor does it even change any pph asm (I diff'ed the old bad assemblies (*.s+pph) with the new ones)).
>
> This does not look right to me.  These two symbols are set when
> calling notice_global_symbol, which is done during code generation
> (you will see it called from cgraph_finalize_function,
> cgraph_mark_reachable_node, etc).
>
> You are probably close to the cause of this relative ordering problem,
> but streaming these two globals is not the solution.  They both should
> be set in the same order by both compilers, but not by forcing them
> this way.
>
> One thing that may be happening here is that the order in which we
> call cgraph_finalize_function is different in the two compilers.
> That's what needs to change.  One thing you could do is set a
> breakpoint in notice_global_symbol.  The two compilers should be
> calling it in the same order.
>
>
> Diego.
>