Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 6 November 2015 at 16:59, Jakub Jelinek wrote: > > In any case, to manually reproduce, compile > gnatmake -g -gnatws macrosub.adb > with GCC 5.1.1 (before the ARM changes) and then try to run that process > against > GCC 5.2.1 (after the ARM changes) libgnat-5.so, which is what make check > does (it uses host_gnatmake to compile the support stuff, so ideally the > processes built by host gcc/gnatmake should not be run with the > LD_LIBRARY_PATH=$ADA_INCLUDE_PATH:$BASE:$LD_LIBRARY_PATH > in the environment, and others should). > In macrosub in particular, the problem is in: > WHILE NOT END_OF_FILE (INFILE1) LOOP >GET_LINE (INFILE1, A_LINE, A_LENGTH); > in FILL_TABLE, where A_LINE'First is 0 and A_LINE'Last is 400 (if I remember > right), but if you step into GET_LINE compiled by GCC 5.2.1, Item'First > and Item'Last don't match that. Ok, I see the mismatch now. However, to get there, I had to use my 5.1 gnatmake -g -gnatws macrosub.ads --rts=/path/to/5.2/arm-none-linux-gnueabihf/libada, as if I ran 5.1 gnatmake without that flag, I did not manage to get the wrong value passed/received with LD_LIBRARY_PATH set to any of build-5.2/gcc/ada/rts, build-5.2/arm-none-linux-gnueabihf/libada, build-5.2/arm-none-linux-gnueabihf/libada/adalib (any further suggestions?). [Also I note 'LD_DEBUG=all ./macrosub' does not show libgnat being loaded that way.] With 5.1 gnatmake -g -gnatws macrosub.ads --rts=/path/to/5.2/arm-none-linux-gnueabihf/libada : $ gdb ./macrosub GNU gdb (Ubuntu 7.7-0ubuntu3) 7.7 [snip] Reading symbols from ./macrosub...done. (gdb) break get_line Breakpoint 1 at 0x1aeec: get_line. (4 locations) (gdb) run Starting program: /home/alalaw01/build-5.1.0/gcc/testsuite/ada/acats/support/macrosub BEGINNING MACRO SUBSTITUTIONS. Breakpoint 1, ada.text_io.get_line (item=...) at a-tigeli.adb:41 41 procedure Get_Line (gdb) print item'first $1 = -443273216 (gdb) print item'last $2 = -514850813 (gdb) n 146FIO.Check_Read_Status (AP (File)); (gdb) n 152if Item'First > Item'Last then (gdb) print item'first $3 = 1 (gdb) print item'last $4 = 0 (gdb) up #1 0x0001f34c in getsubs.fill_table () at getsubs.adb:122 122GET_LINE (INFILE1, A_LINE, A_LENGTH); (gdb) print a_line'first $5 = 1 (gdb) print a_line'last $6 = 400 So yes, we have an ABI change; which is not entirely unexpected. So, questions (1) Why does LD_LIBRARY_PATH affect your system, not mine (i.e. if this is because my gnatmake is building with static linking, then why). This is maybe the least interesting question so I'm leaving it for now... (2) If/when LD_LIBRARY_PATH does have an effect - as you say, things compiled with host gnatmake, should be run against host libraries, not against target libraries. Otherwise, potentially *any* gcc ABI change can break the build process, right? So I think this is of interest regardless of the ARM AAPCS change, but I will be slightly presumptious and hope that the Adacore folk will pick this up...[CC Eric] (3) Has the ARM AAPCS had an effect that we didn't mean it to? I don't see any evidence so far that this is _necessarily_ the case, but I will look into this, bearing Florian's advice in mind (thanks!)... --Alan
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 26 November 2015 at 14:00, Alan Lawrence wrote: > On 6 November 2015 at 16:59, Jakub Jelinek wrote: >> >> In any case, to manually reproduce, compile >> gnatmake -g -gnatws macrosub.adb >> with GCC 5.1.1 (before the ARM changes) and then try to run that process >> against >> GCC 5.2.1 (after the ARM changes) libgnat-5.so, which is what make check >> does (it uses host_gnatmake to compile the support stuff, so ideally the >> processes built by host gcc/gnatmake should not be run with the >> LD_LIBRARY_PATH=$ADA_INCLUDE_PATH:$BASE:$LD_LIBRARY_PATH >> in the environment, and others should). >> In macrosub in particular, the problem is in: >> WHILE NOT END_OF_FILE (INFILE1) LOOP >>GET_LINE (INFILE1, A_LINE, A_LENGTH); >> in FILL_TABLE, where A_LINE'First is 0 and A_LINE'Last is 400 (if I remember >> right), but if you step into GET_LINE compiled by GCC 5.2.1, Item'First >> and Item'Last don't match that. > > Ok, I see the mismatch now. The type affected in Jakub's case here is an Ada String, which looks like this: constant 64> unit size constant 8> align 64 symtab -151604912 alias set -1 canonical type 0xf7569720 fields asm_written unsigned SI size unit size align 32 symtab -151604672 alias set -1 canonical type 0xf756a2a0> unsigned nonaddressable SI file line 0 col 0 size unit size align 32 offset_align 64 offset bit offset context chain visited unsigned nonaddressable SI file line 0 col 0 size unit size align 32 offset_align 64 offset bit offset context >> context unconstrained array BLK align 8 symtab 0 alias set -1 canonical type 0xf7569c00 context pointer_to_this reference_to_this chain > chain > i.e. a 64-bit DImode struct, with alignment set to 64, containing P_ARRAY a 32-bit pointer with alignment 32, and P_BOUNDS a 32-bit pointer with alignment 32, pointing to a record (of size 64, alignment 32, containing two 32-bit ints LB0 and UB0). AFAICT, in the fill_table/get_line case, the first parameter to get_line is a file, a simple pointer; then we have a string. So *fill_table (compiled with 5.1, doubleword aligned) should pass the string P_ARRAY in r2 and P_BOUNDS in r3. 0x1f334movt r3, #2 0x1f338strr3, [r11, #-504]; 0x 0x1f33csubr3, r11, #508 ; 0x1fc 0x1f340ldrd r2, [r3] 0x1f344movr0, r1 0x1f348bl 0x1aee4
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
> So, I'm not familiar with Ada 'fat pointers' but if that is one - > well, it's a record, with an alignment that the 'new' AAPCS now > ignores, so yes the ABI has changed between gcc 5.1 and 5.2, rather > more significantly for Ada than for C. Yes, XUP suffixed types are fat pointers and they are maximally aligned so that they can be given non-BLK mode and, consequently, live in registers. > Thoughts? There is no official ABI for Ada so I guess that's not really a problem as long as it's documented on https://gcc.gnu.org/gcc-5/changes.html. -- Eric Botcazou
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 11/27/2015 06:55 PM, Eric Botcazou wrote: > There is no official ABI for Ada so I guess that's not really a problem as > long as it's documented on https://gcc.gnu.org/gcc-5/changes.html. It's still surprising to make such a far-reaching change in a minor release, I think. Florian
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 03/07/15 16:26, Alan Lawrence wrote:
> These include tests of structs, scalars, and vectors - only
> general-purpose registers are affected by the ABI rules for alignment,
> but we can restrict the vector test to use the base AAPCS.
>
> Prior to this patch, align2.c, align3.c and align_rec1.c were failing
> (the latter showing an internal inconsistency, the first two merely that
> GCC did not obey the new ABI).
>
> With this patch, the align_rec2.c fails, and also
> gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a latent
> bug where we can emit strd/ldrd on an odd-numbered register in ARM
> state, fixed by the second patch.
>
> gcc/ChangeLog:
>
> * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
> alignment attribute, exploring one level down for aggregates.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/arm/aapcs/align1.c: New.
> * gcc.target/arm/aapcs/align_rec1.c: New.
> * gcc.target/arm/aapcs/align2.c: New.
> * gcc.target/arm/aapcs/align_rec2.c: New.
> * gcc.target/arm/aapcs/align3.c: New.
> * gcc.target/arm/aapcs/align_rec3.c: New.
> * gcc.target/arm/aapcs/align4.c: New.
> * gcc.target/arm/aapcs/align_rec4.c: New.
> * gcc.target/arm/aapcs/align_vararg1.c: New.
> * gcc.target/arm/aapcs/align_vararg2.c: New.
>
> arm_overalign_1.patch
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index
> 04663999224c8c8eb8e2d10b0ec634db6ce5027e..ee57d30617a2f7e1cd63ca013fe5655a01027581
> 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6020,8 +6020,17 @@ arm_init_cumulative_args (CUMULATIVE_ARGS *pcum, tree
> fntype,
> static bool
> arm_needs_doubleword_align (machine_mode mode, const_tree type)
> {
> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
> + if (!type)
> +return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
> +
> + if (!AGGREGATE_TYPE_P (type))
> +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
> +
> + for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> +if (DECL_ALIGN (field) > PARM_BOUNDARY)
> + return true;
> +
Technically this is incorrect since AGGREGATE_TYPE_P includes ARRAY_TYPE
and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that
case though (unless there's a language that allows passing arrays by value).
For array types I think you need to check TYPE_ALIGN (TREE_TYPE (type)).
R.
> + return false;
> }
>
>
> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align1.c
> b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
> new file mode 100644
> index
> ..8981d57c3eaf0bd89d224bec79ff8a45627a0a89
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
> @@ -0,0 +1,29 @@
> +/* Test AAPCS layout (alignment). */
> +
> +/* { dg-do run { target arm_eabi } } */
> +/* { dg-require-effective-target arm32 } */
> +/* { dg-options "-O" } */
> +
> +#ifndef IN_FRAMEWORK
> +#define TESTFILE "align1.c"
> +
> +typedef __attribute__((aligned (8))) int alignedint;
> +
> +alignedint a = 11;
> +alignedint b = 13;
> +alignedint c = 17;
> +alignedint d = 19;
> +alignedint e = 23;
> +alignedint f = 29;
> +
> +#include "abitest.h"
> +#else
> + ARG (alignedint, a, R0)
> + /* Attribute suggests R2, but we should use only natural alignment: */
> + ARG (alignedint, b, R1)
> + ARG (alignedint, c, R2)
> + ARG (alignedint, d, R3)
> + ARG (alignedint, e, STACK)
> + /* Attribute would suggest STACK + 8 but should be ignored: */
> + LAST_ARG (alignedint, f, STACK + 4)
> +#endif
> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align2.c
> b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
> new file mode 100644
> index
> ..992da53c606c793f25278152406582bb993719d2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
> @@ -0,0 +1,30 @@
> +/* Test AAPCS layout (alignment). */
> +
> +/* { dg-do run { target arm_eabi } } */
> +/* { dg-require-effective-target arm32 } */
> +/* { dg-options "-O" } */
> +
> +#ifndef IN_FRAMEWORK
> +#define TESTFILE "align2.c"
> +
> +/* The underlying struct here has alignment 4. */
> +typedef struct __attribute__((aligned (8)))
> + {
> +int x;
> +int y;
> + } overaligned;
> +
> +/* A couple of instances, at 8-byte-aligned memory locations. */
> +overaligned a = { 2, 3 };
> +overaligned b = { 5, 8 };
> +
> +#include "abitest.h"
> +#else
> + ARG (int, 7, R0)
> + /* Alignment should be 4. */
> + ARG (overaligned, a, R1)
> + ARG (int, 9, R3)
> + ARG (int, 10, STACK)
> + /* Alignment should be 4. */
> + LAST_ARG (overaligned, b, STACK + 4)
> +#endif
> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align3.c
> b/gcc/testsuite/gcc.target/arm/aapcs/align3.c
> new file mode 100644
> index
> ..81ad3f587a95aae52ec601ce5a60b198e5351edf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.tar
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On Fri, Jul 03, 2015 at 04:26:02PM +0100, Alan Lawrence wrote: > These include tests of structs, scalars, and vectors - only general-purpose > registers are affected by the ABI rules for alignment, but we can restrict > the vector test to use the base AAPCS. > > Prior to this patch, align2.c, align3.c and align_rec1.c were failing (the > latter showing an internal inconsistency, the first two merely that GCC did > not obey the new ABI). > > With this patch, the align_rec2.c fails, and also > gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a latent bug > where we can emit strd/ldrd on an odd-numbered register in ARM state, fixed > by the second patch. > > gcc/ChangeLog: > > * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer > alignment attribute, exploring one level down for aggregates. Can you please also add the testcase from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00278.html to your patch set? Or I can commit it separately after it is approved (if it is). Jakub
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On July 3, 2015 6:11:13 PM GMT+02:00, Richard Earnshaw
wrote:
>On 03/07/15 16:26, Alan Lawrence wrote:
>> These include tests of structs, scalars, and vectors - only
>> general-purpose registers are affected by the ABI rules for
>alignment,
>> but we can restrict the vector test to use the base AAPCS.
>>
>> Prior to this patch, align2.c, align3.c and align_rec1.c were failing
>> (the latter showing an internal inconsistency, the first two merely
>that
>> GCC did not obey the new ABI).
>>
>> With this patch, the align_rec2.c fails, and also
>> gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a
>latent
>> bug where we can emit strd/ldrd on an odd-numbered register in ARM
>> state, fixed by the second patch.
>>
>> gcc/ChangeLog:
>>
>> * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
>> alignment attribute, exploring one level down for aggregates.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/arm/aapcs/align1.c: New.
>> * gcc.target/arm/aapcs/align_rec1.c: New.
>> * gcc.target/arm/aapcs/align2.c: New.
>> * gcc.target/arm/aapcs/align_rec2.c: New.
>> * gcc.target/arm/aapcs/align3.c: New.
>> * gcc.target/arm/aapcs/align_rec3.c: New.
>> * gcc.target/arm/aapcs/align4.c: New.
>> * gcc.target/arm/aapcs/align_rec4.c: New.
>> * gcc.target/arm/aapcs/align_vararg1.c: New.
>> * gcc.target/arm/aapcs/align_vararg2.c: New.
>>
>> arm_overalign_1.patch
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index
>04663999224c8c8eb8e2d10b0ec634db6ce5027e..ee57d30617a2f7e1cd63ca013fe5655a01027581
>100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -6020,8 +6020,17 @@ arm_init_cumulative_args (CUMULATIVE_ARGS
>*pcum, tree fntype,
>> static bool
>> arm_needs_doubleword_align (machine_mode mode, const_tree type)
>> {
>> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>> + if (!type)
>> +return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
>> +
>> + if (!AGGREGATE_TYPE_P (type))
>> +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>> +
>> + for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN
>(field))
>> +if (DECL_ALIGN (field) > PARM_BOUNDARY)
>> + return true;
>> +
Is this behavior correct for unions or aggregates with record or union members?
>
>Technically this is incorrect since AGGREGATE_TYPE_P includes
>ARRAY_TYPE
>and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that
>case though (unless there's a language that allows passing arrays by
>value).
>
>For array types I think you need to check TYPE_ALIGN (TREE_TYPE
>(type)).
>
>R.
>
>> + return false;
>> }
>>
>>
>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>> new file mode 100644
>> index
>..8981d57c3eaf0bd89d224bec79ff8a45627a0a89
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>> @@ -0,0 +1,29 @@
>> +/* Test AAPCS layout (alignment). */
>> +
>> +/* { dg-do run { target arm_eabi } } */
>> +/* { dg-require-effective-target arm32 } */
>> +/* { dg-options "-O" } */
>> +
>> +#ifndef IN_FRAMEWORK
>> +#define TESTFILE "align1.c"
>> +
>> +typedef __attribute__((aligned (8))) int alignedint;
>> +
>> +alignedint a = 11;
>> +alignedint b = 13;
>> +alignedint c = 17;
>> +alignedint d = 19;
>> +alignedint e = 23;
>> +alignedint f = 29;
>> +
>> +#include "abitest.h"
>> +#else
>> + ARG (alignedint, a, R0)
>> + /* Attribute suggests R2, but we should use only natural
>alignment: */
>> + ARG (alignedint, b, R1)
>> + ARG (alignedint, c, R2)
>> + ARG (alignedint, d, R3)
>> + ARG (alignedint, e, STACK)
>> + /* Attribute would suggest STACK + 8 but should be ignored: */
>> + LAST_ARG (alignedint, f, STACK + 4)
>> +#endif
>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>> new file mode 100644
>> index
>..992da53c606c793f25278152406582bb993719d2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>> @@ -0,0 +1,30 @@
>> +/* Test AAPCS layout (alignment). */
>> +
>> +/* { dg-do run { target arm_eabi } } */
>> +/* { dg-require-effective-target arm32 } */
>> +/* { dg-options "-O" } */
>> +
>> +#ifndef IN_FRAMEWORK
>> +#define TESTFILE "align2.c"
>> +
>> +/* The underlying struct here has alignment 4. */
>> +typedef struct __attribute__((aligned (8)))
>> + {
>> +int x;
>> +int y;
>> + } overaligned;
>> +
>> +/* A couple of instances, at 8-byte-aligned memory locations. */
>> +overaligned a = { 2, 3 };
>> +overaligned b = { 5, 8 };
>> +
>> +#include "abitest.h"
>> +#else
>> + ARG (int, 7, R0)
>> + /* Alignment should be 4. */
>> + ARG (overaligned, a, R1)
>> + ARG (int, 9, R3)
>> + ARG (int, 10, STACK)
>> + /* Alignment should be 4. */
>> + LAST_ARG (overaligned, b, STACK + 4)
>>
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 03/07/15 19:24, Richard Biener wrote:
> On July 3, 2015 6:11:13 PM GMT+02:00, Richard Earnshaw
> wrote:
>> On 03/07/15 16:26, Alan Lawrence wrote:
>>> These include tests of structs, scalars, and vectors - only
>>> general-purpose registers are affected by the ABI rules for
>> alignment,
>>> but we can restrict the vector test to use the base AAPCS.
>>>
>>> Prior to this patch, align2.c, align3.c and align_rec1.c were failing
>>> (the latter showing an internal inconsistency, the first two merely
>> that
>>> GCC did not obey the new ABI).
>>>
>>> With this patch, the align_rec2.c fails, and also
>>> gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a
>> latent
>>> bug where we can emit strd/ldrd on an odd-numbered register in ARM
>>> state, fixed by the second patch.
>>>
>>> gcc/ChangeLog:
>>>
>>> * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
>>> alignment attribute, exploring one level down for aggregates.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * gcc.target/arm/aapcs/align1.c: New.
>>> * gcc.target/arm/aapcs/align_rec1.c: New.
>>> * gcc.target/arm/aapcs/align2.c: New.
>>> * gcc.target/arm/aapcs/align_rec2.c: New.
>>> * gcc.target/arm/aapcs/align3.c: New.
>>> * gcc.target/arm/aapcs/align_rec3.c: New.
>>> * gcc.target/arm/aapcs/align4.c: New.
>>> * gcc.target/arm/aapcs/align_rec4.c: New.
>>> * gcc.target/arm/aapcs/align_vararg1.c: New.
>>> * gcc.target/arm/aapcs/align_vararg2.c: New.
>>>
>>> arm_overalign_1.patch
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index
>> 04663999224c8c8eb8e2d10b0ec634db6ce5027e..ee57d30617a2f7e1cd63ca013fe5655a01027581
>> 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -6020,8 +6020,17 @@ arm_init_cumulative_args (CUMULATIVE_ARGS
>> *pcum, tree fntype,
>>> static bool
>>> arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>> {
>>> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>> + if (!type)
>>> +return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
>>> +
>>> + if (!AGGREGATE_TYPE_P (type))
>>> +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>> +
>>> + for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN
>> (field))
>>> +if (DECL_ALIGN (field) > PARM_BOUNDARY)
>>> + return true;
>>> +
>
> Is this behavior correct for unions or aggregates with record or union
> members?
Yes, at least that was my intention. It's an error in the wording of
the proposed change, which I think should say "composite types" not
"aggregate types".
R.
>
>>
>> Technically this is incorrect since AGGREGATE_TYPE_P includes
>> ARRAY_TYPE
>> and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that
>> case though (unless there's a language that allows passing arrays by
>> value).
>>
>> For array types I think you need to check TYPE_ALIGN (TREE_TYPE
>> (type)).
>>
>> R.
>>
>>> + return false;
>>> }
>>>
>>>
>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>> b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>> new file mode 100644
>>> index
>> ..8981d57c3eaf0bd89d224bec79ff8a45627a0a89
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>> @@ -0,0 +1,29 @@
>>> +/* Test AAPCS layout (alignment). */
>>> +
>>> +/* { dg-do run { target arm_eabi } } */
>>> +/* { dg-require-effective-target arm32 } */
>>> +/* { dg-options "-O" } */
>>> +
>>> +#ifndef IN_FRAMEWORK
>>> +#define TESTFILE "align1.c"
>>> +
>>> +typedef __attribute__((aligned (8))) int alignedint;
>>> +
>>> +alignedint a = 11;
>>> +alignedint b = 13;
>>> +alignedint c = 17;
>>> +alignedint d = 19;
>>> +alignedint e = 23;
>>> +alignedint f = 29;
>>> +
>>> +#include "abitest.h"
>>> +#else
>>> + ARG (alignedint, a, R0)
>>> + /* Attribute suggests R2, but we should use only natural
>> alignment: */
>>> + ARG (alignedint, b, R1)
>>> + ARG (alignedint, c, R2)
>>> + ARG (alignedint, d, R3)
>>> + ARG (alignedint, e, STACK)
>>> + /* Attribute would suggest STACK + 8 but should be ignored: */
>>> + LAST_ARG (alignedint, f, STACK + 4)
>>> +#endif
>>> diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>> b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>> new file mode 100644
>>> index
>> ..992da53c606c793f25278152406582bb993719d2
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>> @@ -0,0 +1,30 @@
>>> +/* Test AAPCS layout (alignment). */
>>> +
>>> +/* { dg-do run { target arm_eabi } } */
>>> +/* { dg-require-effective-target arm32 } */
>>> +/* { dg-options "-O" } */
>>> +
>>> +#ifndef IN_FRAMEWORK
>>> +#define TESTFILE "align2.c"
>>> +
>>> +/* The underlying struct here has alignment 4. */
>>> +typedef struct __attribute__((aligned (8)))
>>> + {
>>> +int x;
>>> +int y;
>>> + } overaligned;
>>> +
>>> +/* A couple of inst
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On July 3, 2015 10:43:30 PM GMT+02:00, Richard Earnshaw
wrote:
>On 03/07/15 19:24, Richard Biener wrote:
>> On July 3, 2015 6:11:13 PM GMT+02:00, Richard Earnshaw
> wrote:
>>> On 03/07/15 16:26, Alan Lawrence wrote:
These include tests of structs, scalars, and vectors - only
general-purpose registers are affected by the ABI rules for
>>> alignment,
but we can restrict the vector test to use the base AAPCS.
Prior to this patch, align2.c, align3.c and align_rec1.c were
>failing
(the latter showing an internal inconsistency, the first two merely
>>> that
GCC did not obey the new ABI).
With this patch, the align_rec2.c fails, and also
gcc.c-torture/execute/20040709-1.c at -O0 only, both because of a
>>> latent
bug where we can emit strd/ldrd on an odd-numbered register in ARM
state, fixed by the second patch.
gcc/ChangeLog:
* config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
alignment attribute, exploring one level down for aggregates.
gcc/testsuite/ChangeLog:
* gcc.target/arm/aapcs/align1.c: New.
* gcc.target/arm/aapcs/align_rec1.c: New.
* gcc.target/arm/aapcs/align2.c: New.
* gcc.target/arm/aapcs/align_rec2.c: New.
* gcc.target/arm/aapcs/align3.c: New.
* gcc.target/arm/aapcs/align_rec3.c: New.
* gcc.target/arm/aapcs/align4.c: New.
* gcc.target/arm/aapcs/align_rec4.c: New.
* gcc.target/arm/aapcs/align_vararg1.c: New.
* gcc.target/arm/aapcs/align_vararg2.c: New.
arm_overalign_1.patch
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index
>>>
>04663999224c8c8eb8e2d10b0ec634db6ce5027e..ee57d30617a2f7e1cd63ca013fe5655a01027581
>>> 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6020,8 +6020,17 @@ arm_init_cumulative_args (CUMULATIVE_ARGS
>>> *pcum, tree fntype,
static bool
arm_needs_doubleword_align (machine_mode mode, const_tree type)
{
- return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
-|| (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
+ if (!type)
+return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
+
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
+
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN
>>> (field))
+if (DECL_ALIGN (field) > PARM_BOUNDARY)
+ return true;
I also believe this loop is equivalent to checking TYPE_ALIGN of the aggregate
type?
I'll double check your wording in the abi document, but it seems to be unclear
whether packed and not packed structs should be passed the same (considering
layout differences). OTOH the above function is only relevant for register
passing? (Likewise the abi document changes?)
>>
>> Is this behavior correct for unions or aggregates with record or
>union members?
>
>Yes, at least that was my intention. It's an error in the wording of
>the proposed change, which I think should say "composite types" not
>"aggregate types".
>
>R.
>
>>
>>>
>>> Technically this is incorrect since AGGREGATE_TYPE_P includes
>>> ARRAY_TYPE
>>> and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach
>that
>>> case though (unless there's a language that allows passing arrays by
>>> value).
>>>
>>> For array types I think you need to check TYPE_ALIGN (TREE_TYPE
>>> (type)).
>>>
>>> R.
>>>
+ return false;
}
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align1.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
new file mode 100644
index
>>>
>..8981d57c3eaf0bd89d224bec79ff8a45627a0a89
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/align1.c
@@ -0,0 +1,29 @@
+/* Test AAPCS layout (alignment). */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "align1.c"
+
+typedef __attribute__((aligned (8))) int alignedint;
+
+alignedint a = 11;
+alignedint b = 13;
+alignedint c = 17;
+alignedint d = 19;
+alignedint e = 23;
+alignedint f = 29;
+
+#include "abitest.h"
+#else
+ ARG (alignedint, a, R0)
+ /* Attribute suggests R2, but we should use only natural
>>> alignment: */
+ ARG (alignedint, b, R1)
+ ARG (alignedint, c, R2)
+ ARG (alignedint, d, R3)
+ ARG (alignedint, e, STACK)
+ /* Attribute would suggest STACK + 8 but should be ignored: */
+ LAST_ARG (alignedint, f, STACK + 4)
+#endif
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/align2.c
>>> b/gcc/testsuite/gcc.target/arm/aapcs/align2.c
new file mode 100644
index
>>>
>..992da53c606c793f
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On Sat, Jul 04, 2015 at 12:57:36PM +0200, Richard Biener wrote:
> + if (!AGGREGATE_TYPE_P (type))
> +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
> +
> + for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN
> >>> (field))
> +if (DECL_ALIGN (field) > PARM_BOUNDARY)
> + return true;
>
> I also believe this loop is equivalent to checking TYPE_ALIGN of the
> aggregate type?
Is it?
What if you do
struct __attribute__((aligned (32))) S { char a; int b; char c; }; ?
In this case, TYPE_MAIN_VARIANT of S is S itself, and has TYPE_USER_ALIGN
and TYPE_ALIGN 256.
Jakub
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
> Technically this is incorrect since AGGREGATE_TYPE_P includes ARRAY_TYPE > and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that > case though (unless there's a language that allows passing arrays by value). Ada passes small array types by the method specified by the pass_by_reference hook (and large array types by reference). -- Eric Botcazou
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
Richard Biener wrote: I also believe this loop is equivalent to checking TYPE_ALIGN of the aggregate type? Jakub is correct: the intention is to discard any top-level alignment attribute on a struct declaration. I'll double check your wording in the abi document, but it seems to be unclear whether packed and not packed structs should be passed the same (considering layout differences). OTOH the above function is only relevant for register passing? (Likewise the abi document changes?) It also affects the alignment of things passed on the stack. 'Packed' structs are affected too: the outer 'packed' will have no effect on the position on the stack / in registers, as you say; layout will still be packed. Is this behavior correct for unions or aggregates with record or union members? To clarify Richard Earnshaw's statement: The intention is that 'member alignment' is pretty much gcc's TYPE_ALIGN (actually the source code type declaration - which is the same for for struct members, but ignoring cases where other opts like SRA figure out a larger TYPE_ALIGN). 'Natural alignment' is not directly available in GCC under all circumstances, hence having to compute it here. --Alan
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
Eric Botcazou wrote:
Technically this is incorrect since AGGREGATE_TYPE_P includes ARRAY_TYPE
and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that
case though (unless there's a language that allows passing arrays by value).
Ada passes small array types by the method specified by the pass_by_reference
hook (and large array types by reference).
Ok, thanks. Here's a revised patch that handles array types. Again I've tested
on both trunk (bootstrap + check-gcc) and gcc-5-branch (profiledbootstrap now
succeeding + check-gcc). Jakub's pr65956.c testcase also now passes.
The new code lacks a testcase; from what Eric says, it's possible we can write
one using Ada, but I don't know any Ada myself, so I think any testcase should
follow in a separate patch.
Neither have I managed to run a check-ada yet, as I don't presently have a
working Ada compiler with which to bootstrap gcc's Ada frontend. Working on this
now.
--Alan
gcc/ChangeLog:
* config/arm/arm.c (arm_needs_doubleword_align) : Drop any outer
alignment attribute, exploring one level down for records and arrays.
commit f8bd310d65f2b8fd8d7e1151a4a1f84489738029
Author: Alan Lawrence
Date: Wed Jun 3 18:22:36 2015 +0100
arm_needs_doubleword_align: explore one level for aggregates, also arrays.
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e79a369..e12198a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6151,8 +6151,23 @@ arm_init_cumulative_args (CUMULATIVE_ARGS *pcum, tree fntype,
static bool
arm_needs_doubleword_align (machine_mode mode, const_tree type)
{
- return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
- || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
+ if (!type)
+return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode);
+
+ /* Scalar and vector types: Use natural alignment, i.e. of base type. */
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
+
+ /* Array types: Use member alignment of element type. */
+ if (TREE_CODE (type) == ARRAY_TYPE)
+return TYPE_ALIGN (TREE_TYPE (type)) > PARM_BOUNDARY;
+
+ /* Record/aggregate types: Use greatest member alignment of any member. */
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+if (DECL_ALIGN (field) > PARM_BOUNDARY)
+ return true;
+
+ return false;
}
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 06/07/15 12:00, Alan Lawrence wrote: > Eric Botcazou wrote: >>> Technically this is incorrect since AGGREGATE_TYPE_P includes ARRAY_TYPE >>> and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that >>> case though (unless there's a language that allows passing arrays by value). >> >> Ada passes small array types by the method specified by the >> pass_by_reference hook (and large array types by reference). > > Ok, thanks. Here's a revised patch that handles array types. Again I've > tested on both trunk (bootstrap + check-gcc) and gcc-5-branch > (profiledbootstrap now succeeding + check-gcc). Jakub's pr65956.c testcase > also now passes. > > The new code lacks a testcase; from what Eric says, it's possible we can > write one using Ada, but I don't know any Ada myself, so I think any testcase > should follow in a separate patch. > > Neither have I managed to run a check-ada yet, as I don't presently have a > working Ada compiler with which to bootstrap gcc's Ada frontend. Working on > this now. This is OK, the ada testing can go in parallel and we should take this in to not delay rc1 any further. regards Ramana > > --Alan > > gcc/ChangeLog: > > * config/arm/arm.c (arm_needs_doubleword_align) : Drop any outer > alignment attribute, exploring one level down for records and arrays.
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
Trying to push these now (svn!), patch 2 is going first. I realize my second iteration of patch 1/2, dropped the testcases from the first version. Okay to include those as per https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ? Cheers, Alan Ramana Radhakrishnan wrote: On 06/07/15 12:00, Alan Lawrence wrote: Eric Botcazou wrote: Technically this is incorrect since AGGREGATE_TYPE_P includes ARRAY_TYPE and ARRAY_TYPE doesn't have TYPE_FIELDS. I doubt we could reach that case though (unless there's a language that allows passing arrays by value). Ada passes small array types by the method specified by the pass_by_reference hook (and large array types by reference). Ok, thanks. Here's a revised patch that handles array types. Again I've tested on both trunk (bootstrap + check-gcc) and gcc-5-branch (profiledbootstrap now succeeding + check-gcc). Jakub's pr65956.c testcase also now passes. The new code lacks a testcase; from what Eric says, it's possible we can write one using Ada, but I don't know any Ada myself, so I think any testcase should follow in a separate patch. Neither have I managed to run a check-ada yet, as I don't presently have a working Ada compiler with which to bootstrap gcc's Ada frontend. Working on this now. This is OK, the ada testing can go in parallel and we should take this in to not delay rc1 any further. regards Ramana --Alan gcc/ChangeLog: * config/arm/arm.c (arm_needs_doubleword_align) : Drop any outer alignment attribute, exploring one level down for records and arrays.
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 06/07/15 17:38, Alan Lawrence wrote: > Trying to push these now (svn!), patch 2 is going first. > > I realize my second iteration of patch 1/2, dropped the testcases from the > first version. Okay to include those as per > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ? Yeah the tests are fine to go in as long as the testcases showed no regressions ;) What about Jakub's tests ? Is he adding them in or are you considering them here ? Ramana
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
Ramana Radhakrishnan wrote: On 06/07/15 17:38, Alan Lawrence wrote: Trying to push these now (svn!), patch 2 is going first. I realize my second iteration of patch 1/2, dropped the testcases from the first version. Okay to include those as per https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ? Yeah the tests are fine to go in as long as the testcases showed no regressions ;) What about Jakub's tests ? Is he adding them in or are you considering them here ? Ramana I'll add Jakub's test, but as a separate commit, I wouldn't want to claim authorship of that one ;-) --Alan
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
Ramana Radhakrishnan wrote: This is OK, the ada testing can go in parallel and we should take this in to not delay rc1 any further. I can confirm, no regressions in check-ada (gcc/testsuite/gnats and gcc/testsuite/acats) following an ada bootstrap on cortex-a15/neon/hard-float. That's the existing tests - nothing specifically testing conformance to the AAPCS updates (wrt. arrays), of course. Cheers, Alan
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On Mon, Jul 06, 2015 at 05:38:35PM +0100, Alan Lawrence wrote: > Trying to push these now (svn!), patch 2 is going first. > > I realize my second iteration of patch 1/2, dropped the testcases from the > first version. Okay to include those as per > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ? FYI, it seems that (most likely) the PR65956 changes on gcc-5-branch broke libgnat ABI compatibility on arm - it seems that getsubs.adb from macrosub proglet (and others) are during make check compiled/linked with system gnatmake/gcc, but the program is run at runtime against the new libgnat-5.so. If I run it manually against system libgnat, it works, otherwise it hangs, when Fill_Table from getsubs.adb calls Get_Line, and indeed it looks like the argument passing for Get_Line changed and on the callee side it thinks Item (which is 400 chars string) has random (and in the hanging case negative) number of chars in it. Jakub
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 11/04/2015 02:13 PM, Jakub Jelinek wrote: > On Mon, Jul 06, 2015 at 05:38:35PM +0100, Alan Lawrence wrote: >> Trying to push these now (svn!), patch 2 is going first. >> >> I realize my second iteration of patch 1/2, dropped the testcases from the >> first version. Okay to include those as per >> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ? > > FYI, it seems that (most likely) the PR65956 changes on gcc-5-branch > broke libgnat ABI compatibility on arm - it seems that getsubs.adb > from macrosub proglet (and others) are during make check compiled/linked > with system gnatmake/gcc, but the program is run at runtime > against the new libgnat-5.so. If I run it manually against > system libgnat, it works, otherwise it hangs, when Fill_Table from > getsubs.adb calls Get_Line, and indeed it looks like the argument passing > for Get_Line changed and on the callee side it thinks Item (which is 400 > chars string) has random (and in the hanging case negative) number of chars > in it. The patch looks at TYPE_MAIN_VARIANT without checking first if the type has any qualifiers: + if (!AGGREGATE_TYPE_P (type)) +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY; I'm not sure if this is valid, and what happens here if the type refers to a fat pointer type generated by the Ada front end. Florian
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On 04/11/15 13:13, Jakub Jelinek wrote:
On Mon, Jul 06, 2015 at 05:38:35PM +0100, Alan Lawrence wrote:
Trying to push these now (svn!), patch 2 is going first.
I realize my second iteration of patch 1/2, dropped the testcases from the
first version. Okay to include those as per
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00215.html ?
FYI, it seems that (most likely) the PR65956 changes on gcc-5-branch
broke libgnat ABI compatibility on arm - it seems that getsubs.adb
from macrosub proglet (and others) are during make check compiled/linked
with system gnatmake/gcc, but the program is run at runtime
against the new libgnat-5.so. If I run it manually against
system libgnat, it works, otherwise it hangs, when Fill_Table from
getsubs.adb calls Get_Line, and indeed it looks like the argument passing
for Get_Line changed and on the callee side it thinks Item (which is 400
chars string) has random (and in the hanging case negative) number of chars
in it.
Jakub
Sorry Jakub, can you clarify please, how to reproduce this failure? I've just
bootstrapped gcc-5-branch with ada and run the Ada testsuite, which has build me
gcc/ada/rts/libgnat{.a,.so,-5.so}, and I see all tests passing. (Same with
--disable-bootstrap FWIW.)
It seems plausible that Ada would be the language affected by the ABI change,
obviously it would be somewhat ironic that we broke intercompatibility with
gcc's own libgnat but not against libgnat prior to the change...
Thanks,
Alan
Re: [PATCH 1/2][ARM] PR/65956 AAPCS update for alignment attribute
On Fri, Nov 06, 2015 at 04:48:02PM +, Alan Lawrence wrote:
> Sorry Jakub, can you clarify please, how to reproduce this failure? I've
> just bootstrapped gcc-5-branch with ada and run the Ada testsuite, which has
> build me gcc/ada/rts/libgnat{.a,.so,-5.so}, and I see all tests passing.
> (Same with --disable-bootstrap FWIW.)
I have installed a GCC 5.1.1 version including Ada on the system,
now bootstrap goes fine, but when doing make check the macrosub process just
hangs. It happened to me only on Fedora 23/24 and not on Fedora 22.
I bet the issue why it sometimes can be reproduced and sometimes can't is
that due to the register passing changes Item'First and Item'Last are
uninitialized, and it the process only hangs if you are unlucky (the
difference Item'Last - Item'First is negative).
In any case, to manually reproduce, compile
gnatmake -g -gnatws macrosub.adb
with GCC 5.1.1 (before the ARM changes) and then try to run that process against
GCC 5.2.1 (after the ARM changes) libgnat-5.so, which is what make check
does (it uses host_gnatmake to compile the support stuff, so ideally the
processes built by host gcc/gnatmake should not be run with the
LD_LIBRARY_PATH=$ADA_INCLUDE_PATH:$BASE:$LD_LIBRARY_PATH
in the environment, and others should).
In macrosub in particular, the problem is in:
WHILE NOT END_OF_FILE (INFILE1) LOOP
GET_LINE (INFILE1, A_LINE, A_LENGTH);
in FILL_TABLE, where A_LINE'First is 0 and A_LINE'Last is 400 (if I remember
right), but if you step into GET_LINE compiled by GCC 5.2.1, Item'First
and Item'Last don't match that.
Jakub
