Opened 9 months ago

Closed 5 months ago

#14244 closed bug (fixed)

ghc-prim: hs_atomicread* and hs_atomicwrite* missing barriers

Reported by: trommler Owned by:
Priority: highest Milestone: 8.4.1
Component: Prelude Version: 8.2.1
Keywords: Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: #12537 Differential Rev(s): Phab:D4009
Wiki Page:

Description

The comments in compiler/prelude/primops.txt.pp for both operations say "... Implies a full memory barrier."

The implementation does not issue any barriers as exemplified by the 32-bit variants as can be seen in the excerpts from libraries/ghc-prim/cbits/atomic.c.

StgWord
hs_atomicread32(StgWord x)
{
  return *(volatile StgWord32 *) x;
}

and

void
hs_atomicwrite32(StgWord x, StgWord val)
{
  *(volatile StgWord32 *) x = (StgWord32) val;
}

The native code generator for X86/amd64 and the LLVM backend do not generate calls to these functions but generate the necessary barrier (mfence) directly and thus are not affected by this issue.

There are no gcc __sync_* intrinsics for the two operations. The new __atomic_* intrinisics have the required operations but require gcc 4.7 or later.

Change History (15)

comment:1 Changed 9 months ago by bgamari

Milestone: 8.4.1
Priority: normalhigh

This is quite bad; we should likely just require GCC 4.7 and use the new intrinsics in this case.

comment:2 Changed 9 months ago by trommler

Owner: set to trommler

I'll prepare a patch.

comment:3 Changed 9 months ago by trommler

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

comment:4 Changed 8 months ago by Ben Gamari <ben@…>

In bd765f4/ghc:

Fix atomicread/write operations

In `libraries/ghc-prim/cbits/atomic.c` no barriers were issued for
atomic read and write operations. Starting with gcc 4.7 compiler
intrinsics are offered. The atomic intrinisics are also available in
clang. Use these to implement `hs_atomicread*` and `hs_atomicwrite`.

Test Plan: validate on OSX and Windows

Reviewers: austin, bgamari, simonmar, hvr, erikd, dfeuer

Reviewed By: bgamari

Subscribers: dfeuer, rwbarton, thomie

GHC Trac Issues: #14244

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

comment:5 Changed 7 months ago by Ben Gamari <ben@…>

In 59de2909/ghc:

Update autoconf test for gcc to require 4.7 and up

Fixing #14244 required the newer gcc atomic built-ins that are provided
from 4.7 and up. This updates the test to check for minimum gcc version
4.7.

The version tests for 3.4 (!), 4.4, and 4.6 are no longer needed and can
be removed. This makes the build system simpler.

Test Plan: validate

Reviewers: austin, bgamari, hvr, simonmar

Reviewed By: bgamari

Subscribers: rwbarton, thomie, erikd

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

comment:6 Changed 7 months ago by bgamari

Resolution: fixed
Status: patchclosed

comment:7 Changed 5 months ago by YitzGale

Hmm, by requiring GCC 4.7, this bans GHC from RHEL/CentOS 6, which uses GCC 4.4. That OS will be supported until the end of 2020, and will continue to be used fairly widely in the enterprise until close to then.

Can this requirement be avoided at least for x86, amd64, and llvm builds, where it isn't actually needed?

comment:8 Changed 5 months ago by trommler

If that helps, GCC 4.7 and up is only a build-time requirement. There are newer versions of GCC available for RHEL and CentOS as part of the Red Hat Developer Toolset for instance.

In the case of the LLVM-backend you need a fairly recent (and IIRC an exact) version both at build-time and at run-time. If you are going to install LLVM then you could use clang that comes with it.

comment:9 Changed 5 months ago by bgamari

Owner: trommler deleted
Priority: highhighest
Resolution: fixed
Status: closednew

Reopening to ensure I don't forget to fix the compatibility issue here.

Last edited 5 months ago by bgamari (previous) (diff)

comment:10 Changed 5 months ago by allbery_b

This doesn't consider platforms that use clang as their compiler. And there's been at least one person trying to use icc recently, which iirc used to work.

comment:11 Changed 5 months ago by allbery_b

uh, I should specify that I mean the configure test doesn't. (I have no idea if icc even has these primitives, or whether they look like gcc's.)

comment:12 Changed 5 months ago by bgamari

uh, I should specify that I mean the configure test doesn't.

What precisely is the problem?

As far as I can tell Clang has supported both __atomic_* and __sync_* builtins at least as far back as LLVM 3.4. I'm fairly certain that __sync_* are supported by icc as I have read that this builtins were stolen by gcc from icc.

Last edited 5 months ago by bgamari (previous) (diff)

comment:13 Changed 5 months ago by bgamari

Status: newpatch

comment:14 Changed 5 months ago by Ben Gamari <ben@…>

In 217e417/ghc:

ghc-prim: Emulate C11 atomics when not available

GCC's __sync primitives apparently "usually" imply a full barrier,
meaning they can be used to emulate the more precise C11 atomics albeit
with a loss of efficiency. This restores compatibility with GCC 4.4.

This partially reverts commit 59de290928e6903337f31c1f8107ac8a98ea145d.

Test Plan: Validate on Centos

Reviewers: hvr, simonmar, trommler

Subscribers: rwbarton, thomie, erikd, carter

GHC Trac Issues: #14244

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

comment:15 Changed 5 months ago by bgamari

Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.