Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:

https://gist.github.com/jpf91/1de81d6ff55587d702ae

I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).


Iain, any clue what could cause this? Otherwise I'll have to debug this later on.

On 11 June 2015 at 20:27, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:

> Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:
>
> https://gist.github.com/jpf91/1de81d6ff55587d702ae
>
> I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).
>
>
> Iain, any clue what could cause this? Otherwise I'll have to debug this later on.
>

The comment above says:

 If the DECL isn't in memory, then the DECL wasn't properly
 marked TREE_ADDRESSABLE, which will be either a front-end
 or a tree optimizer bug.

Peek returns a ubyte[4] - so my initial guess would be the recent NRVO change.  Perhaps it wasn't quite as bullet-proof as I hoped.

Am Thu, 11 Jun 2015 22:30:39 +0200
schrieb "Iain Buclaw via D.gnu" <d.gnu@puremagic.com>:

> On 11 June 2015 at 20:27, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:
> 
> > Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:
> >
> > https://gist.github.com/jpf91/1de81d6ff55587d702ae
> >
> > I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).
> >
> >
> > Iain, any clue what could cause this? Otherwise I'll have to debug this later on.
> >
> 
> The comment above says:
> 
>  If the DECL isn't in memory, then the DECL wasn't properly
>  marked TREE_ADDRESSABLE, which will be either a front-end
>  or a tree optimizer bug.
> 
> Peek returns a ubyte[4] - so my initial guess would be the recent NRVO change.  Perhaps it wasn't quite as bullet-proof as I hoped.
> 

Small update: It's indeed the NRVO changes, reduced test case:
------------------
private union EndianSwapper
{
    uint value;
    ubyte[4] array;
}

struct CRC32
{
    public:
        @trusted pure nothrow finish()
        {
            auto tmp = peek();
            return tmp;
        }

        @trusted pure nothrow peek()
        {
            EndianSwapper es;
            es.value = 0;
            return es.array;
        }
}
------------------

Only happens with certain optimizations (-O or higher). Using -O -fno-tree-sra or -O -tree-no-inline hides the problem.

Tree dump before crash:

------------------
finish (struct CRC32 & this)
{
  union EndianSwapper es.0;

  <bb 2>:
  es.0 ={v} {CLOBBER};
  MEM[(ubyte[4] *)&<retval>] = 0;
  return <retval>;

}

Am Tue, 16 Jun 2015 22:05:44 +0200
schrieb Johannes Pfau <nospam@example.com>:

> Am Thu, 11 Jun 2015 22:30:39 +0200
> schrieb "Iain Buclaw via D.gnu" <d.gnu@puremagic.com>:
> 
> > On 11 June 2015 at 20:27, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:
> > 
> > > Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:
> > >
> > > https://gist.github.com/jpf91/1de81d6ff55587d702ae
> > >
> > > I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).
> > >
> > >
> > > Iain, any clue what could cause this? Otherwise I'll have to debug this later on.
> > >
> > 
> > The comment above says:
> > 
> >  If the DECL isn't in memory, then the DECL wasn't properly
> >  marked TREE_ADDRESSABLE, which will be either a front-end
> >  or a tree optimizer bug.
> > 
> > Peek returns a ubyte[4] - so my initial guess would be the recent NRVO change.  Perhaps it wasn't quite as bullet-proof as I hoped.
> > 
> 
> Small update: It's indeed the NRVO changes, reduced test case:
> ------------------
> private union EndianSwapper
> {
>     uint value;
>     ubyte[4] array;
> }
> 
> struct CRC32
> {
>     public:
>         @trusted pure nothrow finish()
>         {
>             auto tmp = peek();
>             return tmp;
>         }
> 
>         @trusted pure nothrow peek()
>         {
>             EndianSwapper es;
>             es.value = 0;
>             return es.array;
>         }
> }
> ------------------
> 

@Iain please read my questions at the bottom of this message, the first (debugging) part probably isn't important.


Debugging showed that the backend assigns a REG:SI rtx for tmp. However, tmp does have the ADDRESSABLE flag set:

--------------------------------------------------------
 <result_decl 0x7ffff7ff5078 tmp
    type <array_type 0x7ffff71f1738
        type <integer_type 0x7ffff73e5f18 ubyte public unsigned QI
            size <integer_cst 0x7ffff73cdf00 constant 8>
            unit size <integer_cst 0x7ffff73cdf18 constant 1>
            align 8 symtab 0 alias set -1 canonical type 0x7ffff73e5f18
 precision 8 min <integer_cst 0x7ffff73dd2e8 0> max <integer_cst
 0x7ffff73dd2b8 255> pointer_to_this <pointer_type 0x7ffff71f17e0>>
 no-force-blk BLK size <integer_cst 0x7ffff73cddc8 constant 32>
        unit size <integer_cst 0x7ffff73cdde0 constant 4>
        align 8 symtab 0 alias set -1 canonical type 0x7ffff71f1738
        domain <integer_type 0x7ffff71f1690 type <integer_type
 0x7ffff73d10a8 sizetype> public SI size <integer_cst 0x7ffff73cddc8
 32> unit size <integer_cst 0x7ffff73cdde0 4> align 32 symtab 0 alias
 set -1 canonical type 0x7ffff71f1690 precision 32 min <integer_cst
 0x7ffff73cddf8 0> max <integer_cst 0x7ffff71e34c8 3>> pointer_to_this
 <pointer_type 0x7ffff71fb2a0>> addressable used ignored regdecl BLK
 file /home/build/gdc-build-configs/configs/x86_64-linux-gnu/gcc-snapshot/arm-gdcproject-linux-gnueabi/.build/src/gcc-custom/libphobos/src/std/digest/crc.d
 line 10 col 14 size <integer_cst 0x7ffff73cddc8 32> unit size
 <integer_cst 0x7ffff73cdde0 4> align 8 context <function_decl
 0x7ffff71f4000 finish> (reg:SI 110 [ <retval> ])>
--------------------------------------------------------


The wrong RTL is generated in function.c:expand_function_start. expand_function_start checks aggregate_value_p (DECL_RESULT (subr), subr). aggregate_value_p does not look at TREE_ADDRESSABLE of the result decl, it only looks at TREE_ADDRESSABLE for the result type and the function type. (which could be a gcc bug?)

Because of this we might have to set DECL_BY_REFERENCE for the return value as well. Will test this tomorrow.

X86 might avoid this issue because of this:
if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
or this
if (targetm.calls.return_in_memory (type, fntype))



However, writing this down and thinking about it now I've got one
fundamental question: Can we even legally do NRVO in the posted test
case? The return types are PODs and fit in a register. So from an API
perspective they should be returned in registers unless otherwise
requested by the user. We can probably set DECL_REFERENCE on the result
to force this. But if we do this we change the ABI (register vs memory
pointer) so we also need to be able to determine whether this function
returns in memory when calling it. I don't see anything
special about the function signature in this example which would allow
this.

Could it be possible the
else if (TREE_CODE (return_type) == ARRAY_TYPE) ...BLKmode
check isn't doing what it's supposed to do?
Maybe we have to call if (targetm.calls.return_in_memory (type, fntype))
instead, like aggregate_value_p does.

Also why is the extra array check necessary? Shouldn't aggregate_value_p handle arrays as well?

On 18 June 2015 at 22:18, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:

> Am Tue, 16 Jun 2015 22:05:44 +0200
> schrieb Johannes Pfau <nospam@example.com>:
>
> > Am Thu, 11 Jun 2015 22:30:39 +0200
> > schrieb "Iain Buclaw via D.gnu" <d.gnu@puremagic.com>:
> >
> > > On 11 June 2015 at 20:27, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:
> > >
> > > > Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:
> > > >
> > > > https://gist.github.com/jpf91/1de81d6ff55587d702ae
> > > >
> > > > I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).
> > > >
> > > >
> > > > Iain, any clue what could cause this? Otherwise I'll have to debug this later on.
> > > >
> > >
> > > The comment above says:
> > >
> > >  If the DECL isn't in memory, then the DECL wasn't properly
> > >  marked TREE_ADDRESSABLE, which will be either a front-end
> > >  or a tree optimizer bug.
> > >
> > > Peek returns a ubyte[4] - so my initial guess would be the recent NRVO change.  Perhaps it wasn't quite as bullet-proof as I hoped.
> > >
> >
> > Small update: It's indeed the NRVO changes, reduced test case:
> > ------------------
> > private union EndianSwapper
> > {
> >     uint value;
> >     ubyte[4] array;
> > }
> >
> > struct CRC32
> > {
> >     public:
> >         @trusted pure nothrow finish()
> >         {
> >             auto tmp = peek();
> >             return tmp;
> >         }
> >
> >         @trusted pure nothrow peek()
> >         {
> >             EndianSwapper es;
> >             es.value = 0;
> >             return es.array;
> >         }
> > }
> > ------------------
> >
>
> @Iain please read my questions at the bottom of this message, the first (debugging) part probably isn't important.
>
>
> Debugging showed that the backend assigns a REG:SI rtx for tmp. However, tmp does have the ADDRESSABLE flag set:
>
> --------------------------------------------------------
>  <result_decl 0x7ffff7ff5078 tmp
>     type <array_type 0x7ffff71f1738
>         type <integer_type 0x7ffff73e5f18 ubyte public unsigned QI
>             size <integer_cst 0x7ffff73cdf00 constant 8>
>             unit size <integer_cst 0x7ffff73cdf18 constant 1>
>             align 8 symtab 0 alias set -1 canonical type 0x7ffff73e5f18
>  precision 8 min <integer_cst 0x7ffff73dd2e8 0> max <integer_cst
>  0x7ffff73dd2b8 255> pointer_to_this <pointer_type 0x7ffff71f17e0>>
>  no-force-blk BLK size <integer_cst 0x7ffff73cddc8 constant 32>
>         unit size <integer_cst 0x7ffff73cdde0 constant 4>
>         align 8 symtab 0 alias set -1 canonical type 0x7ffff71f1738
>         domain <integer_type 0x7ffff71f1690 type <integer_type
>  0x7ffff73d10a8 sizetype> public SI size <integer_cst 0x7ffff73cddc8
>  32> unit size <integer_cst 0x7ffff73cdde0 4> align 32 symtab 0 alias
>  set -1 canonical type 0x7ffff71f1690 precision 32 min <integer_cst
>  0x7ffff73cddf8 0> max <integer_cst 0x7ffff71e34c8 3>> pointer_to_this
>  <pointer_type 0x7ffff71fb2a0>> addressable used ignored regdecl BLK
>  file
> /home/build/gdc-build-configs/configs/x86_64-linux-gnu/gcc-snapshot/arm-gdcproject-linux-gnueabi/.build/src/gcc-custom/libphobos/src/std/digest/crc.d
>  line 10 col 14 size <integer_cst 0x7ffff73cddc8 32> unit size
>  <integer_cst 0x7ffff73cdde0 4> align 8 context <function_decl
>  0x7ffff71f4000 finish> (reg:SI 110 [ <retval> ])>
> --------------------------------------------------------
>
>
> The wrong RTL is generated in function.c:expand_function_start. expand_function_start checks aggregate_value_p (DECL_RESULT (subr), subr). aggregate_value_p does not look at TREE_ADDRESSABLE of the result decl, it only looks at TREE_ADDRESSABLE for the result type and the function type. (which could be a gcc bug?)
>
> Because of this we might have to set DECL_BY_REFERENCE for the return value as well. Will test this tomorrow.
>
> X86 might avoid this issue because of this:
> if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
> or this
> if (targetm.calls.return_in_memory (type, fntype))
>
>
>
> However, writing this down and thinking about it now I've got one
> fundamental question: Can we even legally do NRVO in the posted test
> case? The return types are PODs and fit in a register. So from an API
> perspective they should be returned in registers unless otherwise
> requested by the user. We can probably set DECL_REFERENCE on the result
> to force this. But if we do this we change the ABI (register vs memory
> pointer) so we also need to be able to determine whether this function
> returns in memory when calling it. I don't see anything
> special about the function signature in this example which would allow
> this.
>
> Could it be possible the
> else if (TREE_CODE (return_type) == ARRAY_TYPE) ...BLKmode
> check isn't doing what it's supposed to do?
> Maybe we have to call if (targetm.calls.return_in_memory (type, fntype))
> instead, like aggregate_value_p does.
>
> Also why is the extra array check necessary? Shouldn't aggregate_value_p handle arrays as well?
>

For aggregate_value_p to work correctly, I think the function type should be marked as addressable.

I was just building gdc in a qemu raspbian chroot for other reasons, and have just hit this.  Will have a look.

Iain.

On 18 June 2015 at 23:07, Iain Buclaw <ibuclaw@gdcproject.org> wrote:

> On 18 June 2015 at 22:18, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:
>
>> Am Tue, 16 Jun 2015 22:05:44 +0200
>> schrieb Johannes Pfau <nospam@example.com>:
>>
>> > Am Thu, 11 Jun 2015 22:30:39 +0200
>> > schrieb "Iain Buclaw via D.gnu" <d.gnu@puremagic.com>:
>> >
>> > > On 11 June 2015 at 20:27, Johannes Pfau via D.gnu <d.gnu@puremagic.com> wrote:
>> > >
>> > > > Some recent change in GDC broke ARM cross compiler builds. Same error with gcc-5.1 and gcc-4.9:
>> > > >
>> > > > https://gist.github.com/jpf91/1de81d6ff55587d702ae
>> > > >
>> > > > I'm not sure if this only happens for ARM cross compilers or for all cross compilers. But it doesn't seem to happen with native builds (or maybe it's target specific and it doesn't happen for x86 builds).
>> > > >
>> > > >
>> > > > Iain, any clue what could cause this? Otherwise I'll have to debug this later on.
>> > > >
>> > >
>> > > The comment above says:
>> > >
>> > >  If the DECL isn't in memory, then the DECL wasn't properly
>> > >  marked TREE_ADDRESSABLE, which will be either a front-end
>> > >  or a tree optimizer bug.
>> > >
>> > > Peek returns a ubyte[4] - so my initial guess would be the recent NRVO change.  Perhaps it wasn't quite as bullet-proof as I hoped.
>> > >
>> >
>> > Small update: It's indeed the NRVO changes, reduced test case:
>> > ------------------
>> > private union EndianSwapper
>> > {
>> >     uint value;
>> >     ubyte[4] array;
>> > }
>> >
>> > struct CRC32
>> > {
>> >     public:
>> >         @trusted pure nothrow finish()
>> >         {
>> >             auto tmp = peek();
>> >             return tmp;
>> >         }
>> >
>> >         @trusted pure nothrow peek()
>> >         {
>> >             EndianSwapper es;
>> >             es.value = 0;
>> >             return es.array;
>> >         }
>> > }
>> > ------------------
>> >
>>
>> @Iain please read my questions at the bottom of this message, the first (debugging) part probably isn't important.
>>
>>
>> Debugging showed that the backend assigns a REG:SI rtx for tmp. However, tmp does have the ADDRESSABLE flag set:
>>
>> --------------------------------------------------------
>>  <result_decl 0x7ffff7ff5078 tmp
>>     type <array_type 0x7ffff71f1738
>>         type <integer_type 0x7ffff73e5f18 ubyte public unsigned QI
>>             size <integer_cst 0x7ffff73cdf00 constant 8>
>>             unit size <integer_cst 0x7ffff73cdf18 constant 1>
>>             align 8 symtab 0 alias set -1 canonical type 0x7ffff73e5f18
>>  precision 8 min <integer_cst 0x7ffff73dd2e8 0> max <integer_cst
>>  0x7ffff73dd2b8 255> pointer_to_this <pointer_type 0x7ffff71f17e0>>
>>  no-force-blk BLK size <integer_cst 0x7ffff73cddc8 constant 32>
>>         unit size <integer_cst 0x7ffff73cdde0 constant 4>
>>         align 8 symtab 0 alias set -1 canonical type 0x7ffff71f1738
>>         domain <integer_type 0x7ffff71f1690 type <integer_type
>>  0x7ffff73d10a8 sizetype> public SI size <integer_cst 0x7ffff73cddc8
>>  32> unit size <integer_cst 0x7ffff73cdde0 4> align 32 symtab 0 alias
>>  set -1 canonical type 0x7ffff71f1690 precision 32 min <integer_cst
>>  0x7ffff73cddf8 0> max <integer_cst 0x7ffff71e34c8 3>> pointer_to_this
>>  <pointer_type 0x7ffff71fb2a0>> addressable used ignored regdecl BLK
>>  file
>> /home/build/gdc-build-configs/configs/x86_64-linux-gnu/gcc-snapshot/arm-gdcproject-linux-gnueabi/.build/src/gcc-custom/libphobos/src/std/digest/crc.d
>>  line 10 col 14 size <integer_cst 0x7ffff73cddc8 32> unit size
>>  <integer_cst 0x7ffff73cdde0 4> align 8 context <function_decl
>>  0x7ffff71f4000 finish> (reg:SI 110 [ <retval> ])>
>> --------------------------------------------------------
>>
>>
>> The wrong RTL is generated in function.c:expand_function_start. expand_function_start checks aggregate_value_p (DECL_RESULT (subr), subr). aggregate_value_p does not look at TREE_ADDRESSABLE of the result decl, it only looks at TREE_ADDRESSABLE for the result type and the function type. (which could be a gcc bug?)
>>
>> Because of this we might have to set DECL_BY_REFERENCE for the return value as well. Will test this tomorrow.
>>
>> X86 might avoid this issue because of this:
>> if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
>> or this
>> if (targetm.calls.return_in_memory (type, fntype))
>>
>>
>>
>> However, writing this down and thinking about it now I've got one
>> fundamental question: Can we even legally do NRVO in the posted test
>> case? The return types are PODs and fit in a register. So from an API
>> perspective they should be returned in registers unless otherwise
>> requested by the user. We can probably set DECL_REFERENCE on the result
>> to force this. But if we do this we change the ABI (register vs memory
>> pointer) so we also need to be able to determine whether this function
>> returns in memory when calling it. I don't see anything
>> special about the function signature in this example which would allow
>> this.
>>
>> Could it be possible the
>> else if (TREE_CODE (return_type) == ARRAY_TYPE) ...BLKmode
>> check isn't doing what it's supposed to do?
>> Maybe we have to call if (targetm.calls.return_in_memory (type, fntype))
>> instead, like aggregate_value_p does.
>>
>> Also why is the extra array check necessary? Shouldn't aggregate_value_p handle arrays as well?
>>
>
> For aggregate_value_p to work correctly, I think the function type should be marked as addressable.
>
> I was just building gdc in a qemu raspbian chroot for other reasons, and have just hit this.  Will have a look.
>
> Iain.
>
>
This is what I've settled with: https://github.com/D-Programming-GDC/GDC/pull/107

I've fixed the one failing testsuite test because GDC actually returns int[3] in registers (on 64bit at least).

Iain