Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#10244 closed bug (fixed)

"memory barriers unimplemented on this architecture" on ARM pre-ARMv7

Reported by: rwbarton Owned by: simonmar
Priority: normal Milestone: 7.10.2
Component: Runtime System Version: 7.10.1
Keywords: Cc: simonmar, eridk, cjwatson, slyfox
Operating System: Unknown/Multiple Architecture: arm
Type of failure: Building GHC failed Test Case:
Blocked By: Blocking:
Related Tickets: #10433 Differential Rev(s): Phab:D894
Wiki Page:

Description

Phab:D33 broke the build on ARM pre-ARMv7, because there is no definition of store_load_barrier or load_load_barrier for those platforms. (Granted, the old fall-back behavior of doing nothing was almost certainly incorrect.)

I don't know whether we need CPU-level barriers here, or whether they are available. At a minimum, we need a compiler-level barrier. If we need a CPU-level barrier and it isn't provided by the instruction set, then I guess we should disable SMP for these platforms in mk/config.mk.in.

Attachments (1)

0001-includes-stg-SMP.h-implement-load_-store_load_barrie.patch (1.3 KB) - added by slyfox 3 years ago.
0001-includes-stg-SMP.h-implement-load_-store_load_barrie.patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by nomeata

Cc: eridk cjwatson added
Milestone: 7.10.2

This obviously affects distributions. If someone finds a fix, can we backport this to 7.10?

(Setting milestone to 7.10.2, but 7.10.3 would be fine with me as well.)

Adding Erik and Colin to the CC list, they might know more about this.

comment:2 Changed 3 years ago by slyfox

Cc: slyfox added

Do you have a particular build error? I wonder what else is visible there.

From what I see in the includes/stg/SMP.h it's just omission in implementation.

There is properly-looking write_barrier

EXTERN_INLINE void
write_barrier(void) {
...
#elif arm_HOST_ARCH && defined(arm_HOST_ARCH_PRE_ARMv7)
    __asm__ __volatile__ ("" : : : "memory");
#elif (arm_HOST_ARCH && !defined(arm_HOST_ARCH_PRE_ARMv7)) || aarch64_HOST_ARCH
    __asm__ __volatile__ ("dmb  st" : : : "memory");
...

but not load_load:

EXTERN_INLINE void
load_load_barrier(void) {
...
#elif arm_HOST_ARCH && !defined(arm_HOST_ARCH_PRE_ARMv7)
__asm__ __volatile__ ("dmb" : : : "memory");
...

I think stubbing out with

__asm__ __volatile__ ("" : : : "memory");

is best we can do there.

Changed 3 years ago by slyfox

0001-includes-stg-SMP.h-implement-load_-store_load_barrie.patch

comment:3 Changed 3 years ago by slyfox

Can you try this patch on a real host to see if it's the only omission?

If it's fine I'll make a proper Phab DR.

comment:4 Changed 3 years ago by cjwatson

You can do a little better for ARMv6 with mcr p15: the Linux kernel has some examples. I don't know if this is worth bothering with in practice though.

comment:5 Changed 3 years ago by nomeata

I’ll simply apply the patch to the Debian package and upload to Debian experimental.

comment:6 Changed 3 years ago by slyfox

Differential Rev(s): Phab:D894
Status: newpatch

Created DR with a more detailed comment

https://phabricator.haskell.org/D894

comment:7 Changed 3 years ago by Sergei Trofimovich <siarheit@…>

In eaaa38ba24d5152623cb202a98f71ed09deef0bb/ghc:

includes/stg/SMP.h: implement simple load_/store_load_barrier on armv6 and older

Assuming there is no real SMP systems on these CPUs
I've added only compiler barrier (otherwise write_barrier
and friends need to be fixed as well).

Patch also fixes build breakage reported in #10244.

Signed-off-by: Sergei Trofimovich <siarheit@google.com>

Reviewers: rwbarton, nomeata, austin

Reviewed By: nomeata, austin

Subscribers: bgamari, thomie

Differential Revision: https://phabricator.haskell.org/D894

GHC Trac Issues: #10244

comment:8 Changed 3 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

I've merged this to ghc-7.10 - also, Reid, I committed 753b156dc6b0c38b106c390952750fb800bf27e7 copying your comments from Phab:D894 - so we can hopefully fix the problem you pointed out in the future. We should probably open a bug for this, though...

comment:9 Changed 3 years ago by thoughtpolice

Note: I've filed #10433 as a follow up.

comment:10 Changed 3 years ago by rwbarton

Looks good, thanks!

Note: See TracTickets for help on using tickets.