Opened 2 months ago

Closed 3 weeks ago

#14244 closed bug (fixed)

ghc-prim: hs_atomicread* and hs_atomicwrite* missing barriers

Reported by: trommler Owned by: trommler
Priority: high 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 (6)

comment:1 Changed 2 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 2 months ago by trommler

Owner: set to trommler

I'll prepare a patch.

comment:3 Changed 2 months ago by trommler

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

comment:4 Changed 3 weeks 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 3 weeks 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 3 weeks ago by bgamari

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