[Bug bootstrap/109510] New: [13 Regression] bootstrap with Ada broken on aarch64

[Bug bootstrap/109510] New: [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

            Bug ID: 109510
           Summary: [13 Regression] bootstrap with Ada broken on aarch64
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: bootstrap
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

The fix for PR108910 reportedly breaks bootstrap with Ada enabled

/opt/gcc/gcc-20230414/Build/./gcc/xgcc -B/opt/gcc/gcc-20230414/Build/./gcc/
-B/usr/aarch64-suse-linux/bin/ -B/usr/aarch64-suse-linux/lib/ -isystem
/usr/aarch64-suse-linux/include -isystem /usr/aarch64-suse-linux/sys-include   
-c -g -O2  -fPIC -fno-lto  -W -Wall -gnatpg -nostdinc   s-bitfie.ads -o
s-bitfie.o
+===========================GNAT BUG DETECTED==============================+
| 13.0.1 20230414 (experimental) [master 48d0244b605] (aarch64-suse-linux) GCC
error:|
| in aarch64_function_arg_alignment, at config/aarch64/aarch64.cc:7498     |
| Error detected at s-bituti.adb:195:7 [s-bitfie.ads:55:4]                 |
| Compiling s-bitfie.ads                                                   |

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0
           Keywords|                            |build
           Priority|P3                          |P1
             Target|                            |aarch64

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Maybe Eric can clarify which type kinds in Ada can have TYPE_USER_ALIGN and
_not_ a TYPE_MAIN_VARIANT without.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-04-14
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Maybe Eric can clarify which type kinds in Ada can have TYPE_USER_ALIGN and
> _not_ a TYPE_MAIN_VARIANT without.

All of them, TYPE_USER_ALIGN is supposed to be orthogonal to type variants.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #2)
> > Maybe Eric can clarify which type kinds in Ada can have TYPE_USER_ALIGN and
> > _not_ a TYPE_MAIN_VARIANT without.
> 
> All of them, TYPE_USER_ALIGN is supposed to be orthogonal to type variants.

How do you get at the alignment the type would have when the user didn't
specify it?  That's what the call ABI is supposed to look at.

/* 1 if the alignment for this type was requested by "aligned" attribute,
   0 if it is the default for this type.  */
#define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->base.u.bits.user_align)

so what's the "default" alignment?

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> (In reply to Eric Botcazou from comment #2)
> > > Maybe Eric can clarify which type kinds in Ada can have TYPE_USER_ALIGN and
> > > _not_ a TYPE_MAIN_VARIANT without.
> > 
> > All of them, TYPE_USER_ALIGN is supposed to be orthogonal to type variants.
> 
> How do you get at the alignment the type would have when the user didn't
> specify it?  That's what the call ABI is supposed to look at.
> 
> /* 1 if the alignment for this type was requested by "aligned" attribute,
>    0 if it is the default for this type.  */
> #define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->base.u.bits.user_align)
> 
> so what's the "default" alignment?

Btw, we're talking about non-aggregate types here.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> (In reply to Eric Botcazou from comment #2)
> > > Maybe Eric can clarify which type kinds in Ada can have TYPE_USER_ALIGN and
> > > _not_ a TYPE_MAIN_VARIANT without.
> > 
> > All of them, TYPE_USER_ALIGN is supposed to be orthogonal to type variants.
> 
> How do you get at the alignment the type would have when the user didn't
> specify it?  That's what the call ABI is supposed to look at.
> 
> /* 1 if the alignment for this type was requested by "aligned" attribute,
>    0 if it is the default for this type.  */
> #define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->base.u.bits.user_align)
> 
> so what's the "default" alignment?

What stor-layout.cc sets it to.
For scalar types on targets which don't define ROUND_TYPE_ALIGN, I think it is
usually
(see stor-layout.cc (finalize_type_size)) GET_MODE_ALIGNMENT (TYPE_MODE
(type)),
for vector types targetm.vector_alignment (type).
AGGREGATE_TYPE_Ps should have their specified user alignment honored.

The reason why we shouldn't take user alignment into account for ABI argument
and return value passing is e.g. because useless_type_conversion_p doesn't
really take that user alignment on the scalar types into account.
E.g. integer type conversions are considered equal if they have same mode, same
signedness and same precision, scalar floats if they have same mode,
pointers/references indeed same mode and same address space, vectors have some
extra rules as well, but none
of that takes into account the alignment.  So, what exact type you get in the
call vs. what exact type the callee expects might have different alignment, and
it is the default alignment that should matter there (solely).

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #6 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Btw, we're talking about non-aggregate types here.

Right, I agree that this is unexpected for them, let me investigate.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
In patch form what I wrote above (completely untested):
--- gcc/config/aarch64/aarch64.cc.jj    2023-04-14 09:15:08.470312336 +0200
+++ gcc/config/aarch64/aarch64.cc       2023-04-14 12:08:59.785137542 +0200
@@ -7459,6 +7459,8 @@ aarch64_vfp_is_call_candidate (cumulativ
                                                  nregs, NULL, pcum->silent_p);
 }

+static HOST_WIDE_INT aarch64_simd_vector_alignment (const_tree);
+
 /* Given MODE and TYPE of a function argument, return the alignment in
    bits.  The idea is to suppress any stronger alignment requested by
    the user and opt for the natural alignment (specified in AAPCS64 \S
@@ -7487,16 +7489,16 @@ aarch64_function_arg_alignment (machine_
     {
       /* The ABI alignment is the natural alignment of the type, without
         any attributes applied.  Normally this is the alignment of the
-        TYPE_MAIN_VARIANT, but not always; see PR108910 for a counterexample.
-        For now we just handle the known exceptions explicitly.  */
+        TYPE_MAIN_VARIANT, but not always; see PR108910 for a
+        counterexample.  */
+      unsigned int ret;
       type = TYPE_MAIN_VARIANT (type);
-      if (POINTER_TYPE_P (type))
-       {
-         gcc_assert (known_eq (POINTER_SIZE, GET_MODE_BITSIZE (mode)));
-         return POINTER_SIZE;
-       }
-      gcc_assert (!TYPE_USER_ALIGN (type));
-      return TYPE_ALIGN (type);
+      if (VECTOR_TYPE_P (type))
+       ret = aarch64_simd_vector_alignment (type);
+      else
+       ret = GET_MODE_ALIGNMENT (TYPE_MODE (type));
+      gcc_assert (TYPE_USER_ALIGN (type) || ret == TYPE_ALIGN (type));
+      return ret;
     }

   if (TREE_CODE (type) == ARRAY_TYPE)

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #8 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> In patch form what I wrote above (completely untested):
Sorry in advance for the overly verbose comment, but the timeline here was
that:

- PR108411 fixed an ABI problem exposed by struct-layout-1 trying something
new.

- The fix for PR108411 also included some extra asserts to try to prevent
similar such problems being silent failures in future.

- The breakage in PR108910 was caused by the extra asserts rather than the
change in behaviour (which meant that the asserts were doing their job :)).  I
think it would be possible to construct a wrong-code bug for GCC 11 based on
this.

- The fix for PR108910 handled that case, but in a way that is intended to be
backwards-compatible, since the bug was really an internal inconsistency rather
than a specification vs. implementation discrepancy.

- The fix for PR108910 also included extra asserts, because I can't help
myself.

- This PR was again caused by the extra asserts, rather than by the code
change.

My laboured point being, it's only really the first bullet point that is (or
was intended to be) a change in the implemented ABI.  Otherwise we've tried to
keep the implementation (and implementation method) the same as far as
possible.  The extra asserts are just to make sure that we know what's going
on.

Because of that, I'm wary of changing the general way that we calculate the
alignment.  These asserts have been firing because of unexpected corner cases. 
At least as things stand, we'll err on the side of doing what earlier compilers
did in the face of those corner cases, including those where TYPE_USER_ALIGN is
set.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> How do you get at the alignment the type would have when the user didn't
> specify it?  That's what the call ABI is supposed to look at.
> 
> /* 1 if the alignment for this type was requested by "aligned" attribute,
>    0 if it is the default for this type.  */
> #define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->base.u.bits.user_align)
> 
> so what's the "default" alignment?

Note that the wording is ambiguous if the "aligned" attribute requests the same
alignment as the default for the type, which is the case here:

   type Val is mod 2**Val_Bits with Alignment => Val_Bytes;

The alignment clause is confirming in Ada parlance, which means that it does
not change anything.  We set TYPE_USER_ALIGN in this case, but I agree that we
might as well not set it.  Tentative patch to be attached.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Created attachment 54859
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54859&action=edit
Tentative fix

To be tested.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
It might be possible to re-write the aarch64 assert to be instead

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index f4ef22ce02f..9da46a5e45d 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -7489,13 +7489,15 @@ aarch64_function_arg_alignment (machine_mode mode,
const_tree type,
         any attributes applied.  Normally this is the alignment of the
         TYPE_MAIN_VARIANT, but not always; see PR108910 for a counterexample.
         For now we just handle the known exceptions explicitly.  */
+      tree orig_type = type;
       type = TYPE_MAIN_VARIANT (type);
       if (POINTER_TYPE_P (type))
        {
          gcc_assert (known_eq (POINTER_SIZE, GET_MODE_BITSIZE (mode)));
          return POINTER_SIZE;
        }
-      gcc_assert (!TYPE_USER_ALIGN (type));
+      gcc_assert (TYPE_ALIGN (type) == TYPE_ALIGN (orig_type)
+                 || TYPE_USER_ALIGN (orig_type));
       return TYPE_ALIGN (type);
     }

I'm not sure if that captures the original intent of the assert though.
Maybe instead || !TYPE_USER_ALIGN (type)?

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #12 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #10)
> Created attachment 54859 [details]
> Tentative fix
> 
> To be tested.
Thanks!  This works for me locally and gives clean Ada test results.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:94a21e008c4778e446321b1355f61abc75a076be

commit r13-7187-g94a21e008c4778e446321b1355f61abc75a076be
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Apr 14 20:14:07 2023 +0200

    Fix build failure of Ada runtime for Aarch64 targets

    The Aarch64 back-end now asserts that the main variant of scalar types
    has TYPE_USER_ALIGN cleared, and that's not the case for scalar types
    declared with a confirming alignment clause in Ada.

    gcc/ada/
            PR bootstrap/109510
            * gcc-interface/decl.cc (gnat_to_gnu_entity) <types>: Reset align
            to zero if its value is equal to TYPE_ALIGN and the type is scalar.
            Set TYPE_USER_ALIGN on the type only if align is positive.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Fix applied.

[Bug bootstrap/109510] [13 Regression] bootstrap with Ada broken on aarch64

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109510

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:2e2b6ec156e3035297bd76edfd462d68d1f87314

commit r13-7195-g2e2b6ec156e3035297bd76edfd462d68d1f87314
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Sat Apr 15 19:35:02 2023 +0200

    Fix fallout of previous change on x86/Linux

    gcc/ada/
            PR bootstrap/109510
            * gcc-interface/decl.cc (gnat_to_gnu_entity) <types>: Do not reset
            align to zero in any case.  Set TYPE_USER_ALIGN on the type only if
            it is an aggregate type, or else a type whose default alignment is
            specifically capped on selected platforms.