Please upload a pre-processed testcase

Created attachment 45222 [details]
Preprocessed sample

g++ -c -O2 -x c++ prep

produces the shown code.

$ g++ -v
Using built-in specs.
COLLECT_GCC=/home/sr/gcc8/bin/g++
COLLECT_LTO_WRAPPER=/home/sr/gcc8/libexec/gcc/armv5tel-unknown-linux-gnueabi/8.2.0/lto-wrapper
Target: armv5tel-unknown-linux-gnueabi
Configured with: ../gcc-8.2.0/configure --build=armv5tel-unknown-linux-gnueabi --prefix=/home/sr/gcc8 --enable-languages=c,c++ --with-arch=armv5te --with-mode=arm --disable-nls
Thread model: posix
gcc version 8.2.0 (GCC)

Sigh, this is a PCS layout bug for argument marshalling.  I think it can only happen when a struct inherits 64-bit alignment from a bit-field.  In this case, the alignment on the bit-field is 1, but it's real alignment for the purposes of type-layout is derived from the bit-field's type.

I have a simple fix, but I need to think a bit more about how we might need to warn about this change occurring - made more complicated by the fact that we already have code to warn about a previous ABI bug in this area.

Simpler test-case.

struct x
{
  long long a : 61;
};

void bar (int, struct x);

int foo (int a, int b, int c, struct x d)
{
  bar (a, d);
  return 2;
}

This does not seem to generate ldrd, but does show that 'd' is passed partially in r3 and partially on the stack, when it should be entirely on the stack.

Correct code should be something like:

        push    {r4, lr}
        ldrd    r2, [sp, #8]
        bl      bar
        mov     r0, #2
        pop     {r4, pc}

But we currently generate:

        sub     sp, sp, #8
        push    {r4, lr}
        ldr     r2, [sp, #16]
        mov     r1, r3
        str     r3, [sp, #12]
        bl      bar
        pop     {r4, lr}
        mov     r0, #2
        add     sp, sp, #8
        bx      lr

Tentative patch posted here.

https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01231.html

Author: rearnsha
Date: Tue Jan 22 14:03:22 2019
New Revision: 268151

URL: https://gcc.gnu.org/viewcvs?rev=268151&root=gcc&view=rev
Log:
[arm] PR target/88469 fix incorrect argument passing with 64-bit bitfields

Unfortunately another PCS bug has come to light with the layout of
structs whose alignment is dominated by a 64-bit bitfield element.
Such fields in the type list appear to have alignment 1, but in
reality, for the purposes of alignment of the underlying structure,
the alignment is derived from the underlying bitfield's type.  We've
been getting this wrong since support for over-aligned record types
was added several releases back.  Worse still, the existing code may
generate unaligned memory accesses that may fault on some versions of
the architecture.

I've taken the opportunity to add a few more tests that check the
passing arguments with overalignment in the PCS.  Looking through the
existing tests it looked like they were really only checking
self-consistency and not the precise location of the arguments.

PR target/88469

gcc:
	* config/arm/arm.c (arm_needs_doubleword_align): Return 2 if a record's
	alignment is dominated by a bitfield with 64-bit aligned base type.
	(arm_function_arg): Emit a warning if the alignment has changed since
	earlier GCC releases.
	(arm_function_arg_boundary): Likewise.
	(arm_setup_incoming_varargs): Likewise.

gcc/testsuite:
	* gcc.target/arm/aapcs/bitfield1.c: New test.
	* gcc.target/arm/aapcs/overalign_rec1.c: New test.
	* gcc.target/arm/aapcs/overalign_rec2.c: New test.
	* gcc.target/arm/aapcs/overalign_rec3.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/arm/aapcs/bitfield1.c
    trunk/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec1.c
    trunk/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec2.c
    trunk/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog

Fixed on trunk.  Still need mitigation for gcc-7/8 and to deal with boostrapping gcc-9 with gcc-6/7/8.

Author: rearnsha
Date: Tue Jan 22 17:56:02 2019
New Revision: 268160

URL: https://gcc.gnu.org/viewcvs?rev=268160&root=gcc&view=rev
Log:
[arm] Further fixes for PR88469

A bitfield that is exactly the same size as an integral type and
naturally aligned will have DECL_BIT_FIELD cleared.  So we need to
check DECL_BIT_FIELD_TYPE to be sure whether or not the underlying
type was declared with a bitfield declaration.

I've also added a test for bitfields that are based on overaligned types.

	PR target/88469
gcc:
	* config/arm/arm.c (arm_needs_double_word_align): Check
	DECL_BIT_FIELD_TYPE.

gcc/testsuite:
	* gcc.target/arm/aapcs/bitfield2.c: New test.
	* gcc.target/arm/aapcs/bitfield3.c: New test.


Added:
    trunk/gcc/testsuite/gcc.target/arm/aapcs/bitfield2.c
    trunk/gcc/testsuite/gcc.target/arm/aapcs/bitfield3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog

Author: rearnsha
Date: Thu Jan 24 16:06:34 2019
New Revision: 268240

URL: https://gcc.gnu.org/viewcvs?rev=268240&root=gcc&view=rev
Log:
Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8

This patch, for gcc 8/9 is a mitigation patch for PR target/88469
where gcc-6/7/8 miscompile a structure whose alignment is dominated by
a 64-bit bitfield member.  Since the PCS rules for such a type must
ignore any overalignment of the base type we cannot address this by
simply adding a larger alignment to the class.  We can, however, force
the alignment of the bit-field itself and GCC will handle that as
desired.

	PR target/88469
	* profile-count.h (profile_count): On ARM systems using GCC 6/7/8
	force the alignment of m_val.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/profile-count.h

Author: rearnsha
Date: Thu Jan 24 16:10:06 2019
New Revision: 268241

URL: https://gcc.gnu.org/viewcvs?rev=268241&root=gcc&view=rev
Log:
Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8

This patch, for gcc 8/9 is a mitigation patch for PR target/88469
where gcc-6/7/8 miscompile a structure whose alignment is dominated by
a 64-bit bitfield member.  Since the PCS rules for such a type must
ignore any overalignment of the base type we cannot address this by
simply adding a larger alignment to the class.  We can, however, force
the alignment of the bit-field itself and GCC will handle that as
desired.

	PR target/88469
	* profile-count.h (profile_count): On ARM systems using GCC 6/7/8
	force the alignment of m_val.


Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/profile-count.h

Author: rearnsha
Date: Fri Jan 25 17:09:33 2019
New Revision: 268273

URL: https://gcc.gnu.org/viewcvs?rev=268273&root=gcc&view=rev
Log:
This is pretty unlikely in real code, but similar to Arm, the AArch64
ABI has a bug with the handling of 128-bit bit-fields, where if the
bit-field dominates the overall alignment the back-end code may end up
passing the argument correctly.  This is a regression that started in
gcc-6 when the ABI support code was updated to support overaligned
types.  The fix is very similar in concept to the Arm fix.  128-bit
bit-fields are fortunately extremely rare, so I'd be very surprised if
anyone has been bitten by this.

PR target/88469
gcc/
	* config/aarch64/aarch64.c (aarch64_function_arg_alignment): Add new
	argument ABI_BREAK.  Set to true if the calculated alignment has
	changed in gcc-9.  Check bit-fields for their base type alignment.
	(aarch64_layout_arg): Warn if argument passing has changed in gcc-9.
	(aarch64_function_arg_boundary): Likewise.
	(aarch64_gimplify_va_arg_expr): Likewise.

gcc/testsuite/
	* gcc.target/aarch64/aapcs64/test_align-10.c: New test.
	* gcc.target/aarch64/aapcs64/test_align-11.c: New test.
	* gcc.target/aarch64/aapcs64/test_align-12.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/aarch64/aapcs64/test_align-10.c
    trunk/gcc/testsuite/gcc.target/aarch64/aapcs64/test_align-11.c
    trunk/gcc/testsuite/gcc.target/aarch64/aapcs64/test_align-12.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.c
    trunk/gcc/testsuite/ChangeLog

Unfortunately my armv5 device has died in the meantime, so I cannot verify my original use case. The behavior is indeed different on armv7. It does not trap, even for the original misaligned code. And contrary to x86, where the alignment check flag can be changed by user space, this is a privileged operation on arm, so I cannot even selectively enable it.

(In reply to Stefan Ring from comment #12)
> Unfortunately my armv5 device has died in the meantime, so I cannot verify
> my original use case. The behavior is indeed different on armv7. It does not
> trap, even for the original misaligned code. And contrary to x86, where the
> alignment check flag can be changed by user space, this is a privileged
> operation on arm, so I cannot even selectively enable it.

Note that if you have root access on your board you can modify the kernel's behaviour for various unaligned accesses by changing /proc/cpu/alignment (see Documentation/arm/mem_alignment in the kernel sources).  You might want to try setting this to 3 to get the kernel to report (but fix up) any misaligned accesses).

(In reply to Richard Earnshaw from comment #13)
> Note that if you have root access on your board you can modify the kernel's
> behaviour for various unaligned accesses by changing /proc/cpu/alignment
> (see Documentation/arm/mem_alignment in the kernel sources).  You might want
> to try setting this to 3 to get the kernel to report (but fix up) any
> misaligned accesses).

I know that, but armv7 does not trap at all for misaligned ldrd, at the hardware level. It does trap if it’s not even 32bit-aligned, but that’s a different matter.

The GCC 7 branch is being closed, re-targeting to GCC 8.4.

This was already fixed by the commits in comments #8-#11.