gdc master is broken for arm cross-compilers

[Iain Buclaw via D.gnu]

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

> On 18 June 2015 at 22:18, Johannes Pfau via D.gnu <d.gnu at puremagic.com>
> wrote:
>
>> Am Tue, 16 Jun 2015 22:05:44 +0200
>> schrieb Johannes Pfau <nospam at example.com>:
>>
>> > Am Thu, 11 Jun 2015 22:30:39 +0200
>> > schrieb "Iain Buclaw via D.gnu" <d.gnu at puremagic.com>:
>> >
>> > > On 11 June 2015 at 20:27, Johannes Pfau via D.gnu
>> > > <d.gnu at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/d.gnu/attachments/20150619/67e1b460/attachment.html>