Re: [PATCH] Fix miscompilation in output_constant_def

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

Mark Mitchell wrote:
> Maxim Kuvyrkov wrote:
>
> > But here we have a TREE_CONSTANT_POOL_ADDRESS entry; the difference
> > between the two is that the former is used for function-scope constants
> > and the later is used for file-scope constants; the entries between the
> > pools do not mix, it is either the one or the other.  The second, more
> > relevant to us right now difference, is that TREE_CONSTANT_POOL_ADDRESS
> > entries are handled exclusively in varasm.c:
> >
> > /* 1 if RTX is a symbol_ref that addresses a value in the file's
> >    tree constant pool.  This information is private to varasm.c.  */
> > #define TREE_CONSTANT_POOL_ADDRESS_P(RTX) ...
> >
> > This can only be achieved by returning copies of memory references.
>
> Yes, it does look like TREE_CONSTANT_POOL_ADDRESS_P applies only to
> file-scope constants and is handled entirely within varasm.c.  I agree
> with your original analysis that something needs to make a copy before
> placing the result of a call to output_constant_def into an instruction.
>
> However, I'm not sure whether that copying needs to happen in
> output_constant_def, or could happen somewhere later in the process.
> What does the call stack look like at the point where the RTX "escapes"
> from varasm.c?

The backtrace is rather unamusing:

#0  output_constant_def (exp=0xf7dd7e70, defer=1) at varasm.c:3259
#1  0x0830c8ea in expand_expr_constant (exp=0xf7dd7e70, defer=1, 
modifier=EXPAND_NORMAL) at expr.c:6799
#2  0x0830fb7d in expand_expr_real_1 (exp=0xf7dd7e70, target=0xf7d5bc48, 
tmode=HImode, modifier=EXPAND_NORMAL, alt_rtl=0xffffba64) at expr.c:7515
#3  0x0830dd11 in expand_expr_real (exp=0xf7dd7e70, target=0xf7d5bc48, 
tmode=HImode, modifier=EXPAND_NORMAL, alt_rtl=0xffffba64) at expr.c:7183
#4  0x08300309 in store_expr (exp=0xf7dd7e70, target=0xf7d5bc48, 
call_param_p=0, nontemporal=0 '\0') at expr.c:4644

But you're right, copying should be done sometime later and that place 
is, I believe, unshare rtl pass.  E.g., if there were two same 
references (mem (symbol)) within one function, the second one would've 
been copied (the important point here is that the copying occurs before 
any [con/de]structive transformations take place).  However, for RTX'es 
that live across several functions, this doesn't work because the reload 
pass of the first function is run before the unshare pass of the second 
function.

In the unshare pass the whole function is scanned and copies are made of 
every RTX that needs unsharing.  The unshare pass determines if a 
particular RTX was already seen earlier in the function by looking at 
RTX_FLAG 'used'; if RTX was seen before, a copy of it is substituted in 
the parent RTX.

Therefore, if we set this flag in output_constant_def, the RTX will be 
copied, if still used, in unshare pass.  Still, this is basically the 
same as copying the rtx in output_constant_def itself.

The attached patch implements that.

Comments?

--
Maxim

2009-07-05  Maxim Kuvyrkov  <maxim@codesourcery.com>

	* varasm.c (build_constant_desc): Don't share RTL in pool entries.

Index: varasm.c
===================================================================
--- varasm.c	(revision 148831)
+++ varasm.c	(working copy)
@@ -3208,6 +3208,10 @@ build_constant_desc (tree exp)
   set_mem_alias_set (rtl, 0);
   set_mem_alias_set (rtl, const_alias_set);
 
+  /* We cannot share RTX'es in pool entries.
+     Mark this piece of RTL as required for unsharing.  */
+  RTX_FLAG (rtl, used) = 1;
+
   /* Set flags or add text to the name to record information, such as
      that it is a local symbol.  If the name is changed, the macro
      ASM_OUTPUT_LABELREF will have to know how to strip this