PATCH: PR target/63815: [5 Regression] g++.dg/other/pr53811.C fails with -mcmodel=large -fpic

On Wed, Nov 12, 2014 at 1:39 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 12, 2014 at 01:11:40PM -0800, H.J. Lu wrote:
>> On Wed, Nov 12, 2014 at 1:02 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Nov 12, 2014 at 12:43:17PM -0800, H.J. Lu wrote:
>> >> @@ -42686,8 +42692,12 @@ x86_output_mi_thunk (FILE *file, tree, HOST_WIDE_INT delta,
>> >>    else
>> >>      {
>> >>        if (ix86_cmodel == CM_LARGE_PIC && SYMBOLIC_CONST (fnaddr))
>> >> -     fnaddr = legitimize_pic_address (fnaddr,
>> >> -                                      gen_rtx_REG (Pmode, tmp_regno));
>> >> +     {
>> >> +       SET_REGNO (pic_offset_table_rtx, R11_REG);
>> >
>> > If pic_offset_table_rtx has never been initialized, how you can use
>>
>> It is a pseudo PIC register which is uninitialized:
>>
>> (gdb) call debug_rtx (this_target_rtl->x_pic_offset_table_rtx)
>> (reg:DI 89)
>> (gdb)
>>
>> > SET_REGNO on it?  Shouldn't that be pic_offset_table_rtx = gen_raw_REG (Pmode, R11_REG);
>>
>> I added the following comments and am checking it into trunk:
>>
>>           // CM_LARGE_PIC always uses pseudo PIC register which is
>>           // uninitialized.  Since FUNCTION is local and calling it
>>           // doesn't go through PLT, we use scratch register %r11 as
>>           // PIC register and initialize it here.
>
> From what I can see, it probably in this case should be always non-NULL,
> as it is initialized in init_emit_regs:
> 5847      if ((unsigned) PIC_OFFSET_TABLE_REGNUM != INVALID_REGNUM)
> 5848        pic_offset_table_rtx = gen_raw_REG (Pmode, PIC_OFFSET_TABLE_REGNUM);
> 5849      else
> 5850        pic_offset_table_rtx = NULL_RTX;
> and as pic_offset_table_rtx is NULL there, PIC_OFFSET_TABLE_REGNUM is
> BX_REG.
> Later on, assign_params will change that to:
> 3684      if (targetm.use_pseudo_pic_reg ())
> 3685        pic_offset_table_rtx = gen_reg_rtx (Pmode);
> and do_reload, after reload temporarily changing the REGNO, will overwrite
> it again to the chosen pseudo:
> 5470      if (pic_offset_table_regno != INVALID_REGNUM)
> 5471        pic_offset_table_rtx = gen_rtx_REG (Pmode, pic_offset_table_regno);
>
> So, all in all, SET_REGNO sounds inappropriate to me, everywhere else
> gen_raw_REG or gen_rtx_REG is used instead, so IMNSHO you should do the
> same.
>
>         Jakub

It makes sense.  I checked in the following patch.

Thanks.

On Wed, Nov 12, 2014 at 01:51:01PM -0800, H.J. Lu wrote:
> > So, all in all, SET_REGNO sounds inappropriate to me, everywhere else
> > gen_raw_REG or gen_rtx_REG is used instead, so IMNSHO you should do the
> > same.
> >
> >         Jakub
> 
> It makes sense.  I checked in the following patch.

gen_raw_REG would be better, the difference is that gen_rtx_REG might
in theory return you pic_offset_table_rtx again.

Anyway, I don't think anything is needed for release branches,
pic_offset_table_rtx should be initialized there just once and not changed
afterwards.

	Jakub

On Wed, Nov 12, 2014 at 11:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 12, 2014 at 01:51:01PM -0800, H.J. Lu wrote:
>> > So, all in all, SET_REGNO sounds inappropriate to me, everywhere else
>> > gen_raw_REG or gen_rtx_REG is used instead, so IMNSHO you should do the
>> > same.
>> >
>> >         Jakub
>>
>> It makes sense.  I checked in the following patch.
>
> gen_raw_REG would be better, the difference is that gen_rtx_REG might
> in theory return you pic_offset_table_rtx again.
>
> Anyway, I don't think anything is needed for release branches,
> pic_offset_table_rtx should be initialized there just once and not changed
> afterwards.

Please see the test(s) from [1]. The executable segfaults when
compiled with gcc-4.9.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63815#c10

Uros.

On Wed, Nov 12, 2014 at 2:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 12, 2014 at 01:51:01PM -0800, H.J. Lu wrote:
>> > So, all in all, SET_REGNO sounds inappropriate to me, everywhere else
>> > gen_raw_REG or gen_rtx_REG is used instead, so IMNSHO you should do the
>> > same.
>> >
>> >         Jakub
>>
>> It makes sense.  I checked in the following patch.
>
> gen_raw_REG would be better, the difference is that gen_rtx_REG might
> in theory return you pic_offset_table_rtx again.

The whole  x86_output_mi_thunk function uses gen_rtx_REG.  Using
gen_raw_REG is out of space.

> Anyway, I don't think anything is needed for release branches,
> pic_offset_table_rtx should be initialized there just once and not changed
> afterwards.

Try this with released GCC:

[hjl@gnu-6 tmp]$ cat foo.h
struct ICCStringClass
{
  virtual int CreateString (int) = 0;
};

struct AGSCCDynamicObject
{
  virtual void Unserialize () = 0;
};


struct ScriptString:AGSCCDynamicObject, ICCStringClass
{
  virtual int CreateString (int);
  virtual void Unserialize ();
};
[hjl@gnu-6 tmp]$ cat foo1.cc
#include "foo.h"

int
__attribute__ ((noinline))
CreateNewScriptString (int fromText, bool reAllocate = true)
{
  return fromText;
}

int
__attribute__ ((noinline))
ScriptString::CreateString (int fromText)
{
  return CreateNewScriptString (fromText);
}

void
__attribute__ ((noinline))
ScriptString::Unserialize ()
{
}
[hjl@gnu-6 tmp]$ cat foo2.cc
#include "foo.h"

int
main ()
{
  ICCStringClass *x = new ScriptString;
  if (x->CreateString (1) != 1)
    __builtin_abort ();
  return 0;
}
[hjl@gnu-6 tmp]$ g++ -shared -fpic -O2 -mcmodel=large -o libfoo.so foo1.cc
[hjl@gnu-6 tmp]$ g++ foo2.cc ./libfoo.so
[hjl@gnu-6 tmp]$ ./a.out
Segmentation fault
[hjl@gnu-6 tmp]$