Bug 80082 - [5/6 regression] GCC incorrectly assumes Cortex-r[578] have 64-bit single-copy atomic LDRD
Summary: [5/6 regression] GCC incorrectly assumes Cortex-r[578] have 64-bit single-cop...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 7.0.1
: P2 normal
Target Milestone: 5.5
Assignee: Thomas Preud'homme
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-03-17 13:53 UTC by Prakhar Bahuguna
Modified: 2017-04-12 10:57 UTC (History)
1 user (show)

See Also:
Host:
Target: arm
Build:
Known to work: 6.3.1, 7.0
Known to fail: 5.4.1, 6.3.0
Last reconfirmed: 2017-03-17 00:00:00


Attachments
Atomic load reproducer (81 bytes, text/x-csrc)
2017-03-17 13:53 UTC, Prakhar Bahuguna
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Prakhar Bahuguna 2017-03-17 13:53:48 UTC
Created attachment 40992 [details]
Atomic load reproducer

GCC currently assumes that if your armv7* architecture has both arm and Thumb division instructions then you have the LPAE extension. This causes the compiler to assume that LDRD is single-copy atomic.

Cortex-R cores with ARM division are thus incorrectly treated as having LPAE. This is a bug that leads to incorrect code that would likely to be difficult to track down in the field (potential data corruption during contention).

Steps to reproduce:
* Compile atomic_load.c with arm-none-eabi-gcc -mcpu=cortex-r5 atomic_load.c -S -O2 -o -
* Note that in the output, LDRD is used to load x instead of LDREXD, despite LDRD not being single-copy atomic on Cortex-R profile. The same issue can be reproduced for -mcpu=cortex-r7 and cortex-r8.
Comment 1 Richard Earnshaw 2017-03-17 14:06:23 UTC
Introduced by r233658; cortex-r5 support pre-dates that, so a regression in gcc-6 and -7.
Comment 2 Richard Earnshaw 2017-03-17 14:14:50 UTC
It looks as though this patch was then backported onto older releases as well, so GCC-5 also regressed.
Comment 3 ktkachov 2017-03-17 16:01:07 UTC
Confirmed.
The problem is that these Cortex-R cores combined with the bit_adiv isa bit end up having the same feature set as armv7ve as far as GCC can tell.

I think the best approach here is to create an isa_bit_lpae that's unique to armv7ve (and higher A-profile arches)
Comment 4 ktkachov 2017-03-17 16:02:55 UTC
P2 as the bug appears in a released compiler
Comment 5 Richard Biener 2017-03-20 09:02:56 UTC
If backports were not yet released this would be a P1 (no known-to-work/fail specified though...)
Comment 6 ktkachov 2017-03-20 09:11:08 UTC
I believe Prakhar is working on a fix
Comment 7 Thomas Preud'homme 2017-03-22 11:35:47 UTC
Author: thopre01
Date: Wed Mar 22 11:35:15 2017
New Revision: 246365

URL: https://gcc.gnu.org/viewcvs?rev=246365&root=gcc&view=rev
Log:
Fix PR80082: LDRD erronously used for 64bit load on ARMv7-R

2017-03-22  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    gcc/
    PR target/80082
    * config/arm/arm-isa.h (isa_bit_lpae): New feature bit.
    (ISA_ARMv7ve): Add isa_bit_lpae to the definition.
    * config/arm/arm-protos.h (arm_arch7ve): Rename into ...
    (arm_arch_lpae): This.
    * config/arm/arm.c (arm_arch7ve): Rename into ...
    (arm_arch_lpae): This.  Define it in term of isa_bit_lpae.
    * config/arm/arm.h (TARGET_HAVE_LPAE): Redefine in term of
    arm_arch_lpae.

    gcc/testsuite/
    PR target/80082
    * gcc.target/arm/atomic_loaddi_10.c: New testcase.
    * gcc.target/arm/atomic_loaddi_11.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.target/arm/atomic_loaddi_10.c
    trunk/gcc/testsuite/gcc.target/arm/atomic_loaddi_11.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm-isa.h
    trunk/gcc/config/arm/arm-protos.h
    trunk/gcc/config/arm/arm.c
    trunk/gcc/config/arm/arm.h
    trunk/gcc/testsuite/ChangeLog
Comment 8 Thomas Preud'homme 2017-03-22 11:38:41 UTC
Updating known to work and known to fail fields.
Comment 9 Thomas Preud'homme 2017-04-06 14:53:54 UTC
Author: thopre01
Date: Thu Apr  6 14:53:22 2017
New Revision: 246733

URL: https://gcc.gnu.org/viewcvs?rev=246733&root=gcc&view=rev
Log:
[ARM] Compile atomic_loaddi_11 for Cortex-R5

2017-04-06  Thomas Preud'homme  <thomas.preudhomme@arm.com

    gcc/testsuite/
    PR target/80082
    * gcc.target/arm/atomic_loaddi_11.c: Target Cortex-R5 instead of
    ARMv7-R.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/arm/atomic_loaddi_11.c
Comment 10 Thomas Preud'homme 2017-04-06 16:27:11 UTC
Author: thopre01
Date: Thu Apr  6 16:26:39 2017
New Revision: 246734

URL: https://gcc.gnu.org/viewcvs?rev=246734&root=gcc&view=rev
Log:
2017-04-06  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    gcc/
    PR target/80082
    * config/arm/arm-protos.h (FL_LPAE): Define macro.
    (FL_FOR_ARCH7VE): Add FL_LPAE.
    (arm_arch_lpae): Declare extern.
    * config/arm/arm.c (arm_arch_lpae): Declare.
    (arm_option_override): Define arm_arch_lpae.
    * config/arm/arm.h (TARGET_HAVE_LPAE): Redefine in term of
    arm_arch_lpae.

    gcc/testsuite/
    PR target/80082
    * gcc.target/arm/atomic_loaddi_10.c: New testcase.
    * gcc.target/arm/atomic_loaddi_11.c: Likewise.

Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.target/arm/atomic_loaddi_10.c
    branches/gcc-6-branch/gcc/testsuite/gcc.target/arm/atomic_loaddi_11.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/config/arm/arm-protos.h
    branches/gcc-6-branch/gcc/config/arm/arm.c
    branches/gcc-6-branch/gcc/config/arm/arm.h
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 11 Thomas Preud'homme 2017-04-11 15:26:52 UTC
Author: thopre01
Date: Tue Apr 11 15:26:20 2017
New Revision: 246844

URL: https://gcc.gnu.org/viewcvs?rev=246844&root=gcc&view=rev
Log:
Fix PR80082: LDRD erronously used for 64bit load on ARMv7-R

2017-04-11  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    Backport from GCC 6
    2017-04-06  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    gcc/
    PR target/80082
    * config/arm/arm-protos.h (FL_LPAE): Define macro.
    (FL_FOR_ARCH7VE): Add FL_LPAE.
    (arm_arch_lpae): Declare extern.
    * config/arm/arm.c (arm_arch_lpae): Declare.
    (arm_option_override): Define arm_arch_lpae.
    * config/arm/arm.h (TARGET_HAVE_LPAE): Redefine in term of
    arm_arch_lpae.

    gcc/testsuite/
    PR target/80082
    * gcc.target/arm/atomic_loaddi_10.c: New testcase.
    * gcc.target/arm/atomic_loaddi_11.c: Likewise.

Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.target/arm/atomic_loaddi_10.c
    branches/gcc-5-branch/gcc/testsuite/gcc.target/arm/atomic_loaddi_11.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/arm/arm-protos.h
    branches/gcc-5-branch/gcc/config/arm/arm.c
    branches/gcc-5-branch/gcc/config/arm/arm.h
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 12 Ramana Radhakrishnan 2017-04-12 10:57:51 UTC
Fixed .