Re: [PATCH] Fix miscompilation in output_constant_def

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

<Sorry for the long delay>

Jeff Law wrote:
> Maxim Kuvyrkov wrote:

...

> > > --- varasm.c.orig       2008-11-14 18:04:27.693643900 +0100
> > > +++ varasm.c    2008-11-14 17:58:06.522748300 +0100
> > > @@ -3245,7 +3245,7 @@
> > >      }
> > >
> > >    maybe_output_constant_def_contents (desc, defer);
> > > -  return desc->rtl;
> > > +  return copy_rtx (desc->rtl);
> > >  }
> >
> > This patch seems correct to me.  It makes output_constant_def() always 
> > return a copy of the value rather than the value.
> >
> > I will regtest this patch on ColdFire [where the miscompilation 
> > occurs], wound anyone volunteer to test it on x86[_64]?
> >
> > --
> > Maxim
>
> Unfortunately, there is no clear documentation on sharing of constant 
> pool entries; however, upon reviewing the code in GCC itself, it appears 
> to me the compiler expects entries in the constant pool to be shared.  
> That would make your patch incorrect.
>
> Instead we need to focus on the code which reloads the symbol into a 
> register transforming (mem (symbol)) into (mem ((reg)).   That code 
> should be copying the MEM before modifying it.  If you search for 
> CONSTANT_POOL_ADDRESS in reload.c you'll see an example of what I mean.

After some more investigation I'm not sure that your conclusions are 
correct.  Indeed CONSTANT_POOL_ADDRESS entries are expected to be shared 
and reload properly copies those before changing anything inside.

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.

Thoughts?

FWIW, I can patch up reload, but that doesn't look to be in the spirit 
of TREE_CONSTANT_POOL_ADDRESS.

Thanks,

--
Maxim