diff mbox

Remove incorrect default implementation of atomics.

Message ID 1418059612.25868.84.camel@triegel.csb
State New
Headers show

Commit Message

Torvald Riegel Dec. 8, 2014, 5:26 p.m. UTC 
There's a default implementation of atomic operations that just uses
sequential code.  This is incorrect because it at least would need
proper compiler barriers, unless the arch using this somehow (can)
make(s) assumptions about the compiler's optimizations too.

I don't think it's useful to keep this.  Every arch should have a
correct implementation of atomics; having an incorrect default
implementation is just error-prone.


2014-12-09  Torvald Riegel  <triegel@redhat.com>

	* bits/atomic.h: Remove file.

Comments

Ondřej Bílka Dec. 8, 2014, 8:30 p.m. UTC | #1 
On Mon, Dec 08, 2014 at 06:26:52PM +0100, Torvald Riegel wrote:
> There's a default implementation of atomic operations that just uses
> sequential code.  This is incorrect because it at least would need
> proper compiler barriers, unless the arch using this somehow (can)
> make(s) assumptions about the compiler's optimizations too.
> 
> I don't think it's useful to keep this.  Every arch should have a
> correct implementation of atomics; having an incorrect default
> implementation is just error-prone.
>
Its dead header, As every supported gcc has __sync builin one could also
use it instead of this.
> 
> 2014-12-09  Torvald Riegel  <triegel@redhat.com>
> 
> 	* bits/atomic.h: Remove file.
Roland McGrath Dec. 8, 2014, 9:13 p.m. UTC | #2 
Stub headers like that exist to document the internal API for that sysdeps header.
Torvald Riegel Dec. 9, 2014, 9:40 a.m. UTC | #3 
On Mon, 2014-12-08 at 13:13 -0800, Roland McGrath wrote:
> Stub headers like that exist to document the internal API for that sysdeps header.

But that's not true in this case, at least not with the code we have.
It doesn't define any of the barriers, nor has a full set of operations,
nor explains the actual synchronization semantics.
The only useful documentation that we have is in include/atomic.h, which
uses bits/atomic.h or its arch-specific variants.  The archs with weak
memory models (ARM, Power, ...) are also a good source of information.
But bits/atomics.h is definitely not a documentation of an internal API.

For the newer C11-like atomics, documentation is in the wiki and the C11
standard.  In the long run, we'll have just this as internal API.

Thus, I think it doesn't serve any actual purpose.  If you want the
internal API of the old-style atomics to be documented, I can
additionally prepare a patch with that for include/atomic.h.

Sounds good?
Roland McGrath Dec. 9, 2014, 11:27 p.m. UTC | #4 
My point is that removing the header entirely is not good.  When generic
code requires that some sysdeps file exist, then we always have a generic
or stub version of that sysdeps file that tells someone about how to go
about filling in that requirement in a new port.  If there is really
nothing useful for the placeholder file to contain, it can contain an
#error line and a comment referring to where the relevant details are to be
found.  But in most cases, we have found it most useful to write a skeleton
file (maybe still with some #error or other obviously non-compiling syntax)
that has at least substantial comments about what a real file would do.
diff mbox

Patch

commit eb1dc68327c0d8215b0c26419317a2022337d125
Author: Torvald Riegel <triegel@redhat.com>
Date:   Mon Dec 8 17:47:55 2014 +0100

    Remove incorrect default implementation of atomics.

diff --git a/bits/atomic.h b/bits/atomic.h
deleted file mode 100644
index a3d1fa1..0000000
--- a/bits/atomic.h
+++ /dev/null
@@ -1,42 +0,0 @@ 
-/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#ifndef _BITS_ATOMIC_H
-#define _BITS_ATOMIC_H	1
-
-/* We have by default no support for atomic operations.  So define
-   them non-atomic.  If this is a problem somebody will have to come
-   up with real definitions.  */
-
-/* The only basic operation needed is compare and exchange.  */
-#define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \
-  ({ __typeof (mem) __gmemp = (mem);				      \
-     __typeof (*mem) __gret = *__gmemp;				      \
-     __typeof (*mem) __gnewval = (newval);			      \
-								      \
-     if (__gret == (oldval))					      \
-       *__gmemp = __gnewval;					      \
-     __gret; })
-
-#define atomic_compare_and_exchange_bool_acq(mem, newval, oldval) \
-  ({ __typeof (mem) __gmemp = (mem);				      \
-     __typeof (*mem) __gnewval = (newval);			      \
-								      \
-     *__gmemp == (oldval) ? (*__gmemp = __gnewval, 0) : 1; })
-
-#endif	/* bits/atomic.h */