[Bug libstdc++/54005] New: Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[Bug libstdc++/54005] New: Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

             Bug #: 54005
           Summary: Use __atomic_always_lock_free in libstdc++
                    is_lock_free instead of __atomic_is_lock_free
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: hp@gcc.gnu.org
                CC: amacleod@redhat.com
        Depends on: 54004


According to http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2992.htm,
the is_lock_free property goes for all instances of a given type.  So,
libstdc++ should use __atomic_always_lock_free instead of __atomic_is_lock_free
as it does now, at r189499, libstdc++-v3/include/bits/atomic_base.h.  But
first, better fix the depended bugs.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[bkoz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #1 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-08-07 23:04:05 UTC ---
Author: bkoz
Date: Tue Aug  7 23:03:55 2012
New Revision: 190216

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190216
Log:
2012-08-07  Benjamin Kosnik  <bkoz@redhat.com>

    PR libstdc++/54005
    * include/std/atomic: Use __atomic_always_lock_free.
    * include/bits/atomic_base.h: Same.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/atomic_base.h
    trunk/libstdc++-v3/include/std/atomic

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #2 from Andrew Macleod <amacleod at redhat dot com> 2012-08-08 15:04:21 UTC ---
I don't think that is right.


A type may not be always lock free from the compiler's perspective, so
__atomic_always_lock_free() will return false.  

The compiler will issue a call to libatomic to implement any atomic operations,
and libatomic may provide a lock-free implementation for the type/size.  So
these need to remain runtime calls to   __atomic_is_lock_free () since the
question cannot always be answered at compile time.

In cases where __atomic_always_lock_free() is true at compile time,
__atomic_is_lock_free() will return a compile time true.  The runtime call is
only made when __atomic_always_lock_free is false.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bkoz at gcc dot gnu.org

--- Comment #3 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-08-14 02:46:23 UTC ---
(In reply to comment #2)
> I don't think that is right.

See the reference.  The sites changed are where semantics has changed to be
per-type rather than per-object.  See the referenced URL: "Edit paragraph 2 as
follows. The function atomic_is_lock_free (29.6) indicates whether the type is
lock-free. In any given program execution, the result of the lock-free query
shall be consistent for all pointers of the same type."

Adding the committer to CC for further comments.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #4 from Andrew Macleod <amacleod at redhat dot com> 2012-08-14 03:20:09 UTC ---

A GCC port is incorrect if it is issuing any kind of lock.  GCC is only allowed
to issue a lock free sequence of some sort.  If a lock is required, then a call
to libatomic must be made.

So if there is a type which is sometimes lock free and sometimes not due to
alignment or anything else,   __atomic_always_lock_free should return FALSE for
that type.  I think your current problem is that the infrastructure change for
__atomic_always_lock_free to handle alignment issues is not present yet. So it
erroneously says TRUE for this type when in fact it shouldn't.   

I don't think this will be resolved properly in GCC 4.7 since c++11 is still
experimental (I could be wrong, we'll see how invasive the support changes
are).


Its important that the libstdc++ routines call the __atomic_is_lock_free() so
that the answer can be determined at runtime if libatomic is utilized for any
atomic sequences. 

__atomic_always_lock_free() is *always* resolved to a 1 or a 0 at compile time.
The compiler is only capable of answering the question, "Does the compiler
always generate a lock free sequence".    

__atomic_is_lock_free() will return true if __atomic_always_lock_free() returns
true. Otherwise it is left as a runtime call into libatomic so it can answer
the q.  libatomic is

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #5 from Andrew Macleod <amacleod at redhat dot com> 2012-08-14 03:27:28 UTC ---
huh, somehow this got submitted before I finished. :-P

For libstdc++, the macros SHOULD use __atomic_always_lock_free() since they are
intended to be used in #ifdef's and such and we want those resolved at compile
time for use with compiler sequences.   

The is_lock_free() method should continue to call the __atomic_is_lock_free().
That question may not be answerable without asking whatever libatomic is linked
into the executable.  Just because the compiler doesn't know a type is lock
free doesn't mean the library doesn't have a lock free implementation.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crowl at gcc dot gnu.org

--- Comment #6 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-08-14 03:52:07 UTC ---
(In reply to comment #4)
> A GCC port is incorrect if it is issuing any kind of lock.  GCC is only allowed
> to issue a lock free sequence of some sort.  If a lock is required, then a call
> to libatomic must be made.
> 
> So if there is a type which is sometimes lock free and sometimes not due to
> alignment or anything else,   __atomic_always_lock_free should return FALSE for
> that type.

Yes...

>  I think your current problem is that the infrastructure change for
> __atomic_always_lock_free to handle alignment issues is not present yet. So it
> erroneously says TRUE for this type when in fact it shouldn't.

This PR is about the libstdc++ library is_lock_free function using the
*per-object* builtin query when it should use the *per-type* query as per the
referenced discussion.  And that's been corrected.

It's separate to my other target woes; an incidental observation.  I agree the
function has a name intuitively leading to thinking it should be per-object,
but it isn't.

> Its important that the libstdc++ routines call the __atomic_is_lock_free() so
> that the answer can be determined at runtime if libatomic is utilized for any
> atomic sequences.

Maybe elsewhere, but not this particular code.  If your argument is that
is_lock_free should be per-object, then you'll have to argue with the
standard-guys; I'm just quoting the reference above.

> __atomic_always_lock_free() is *always* resolved to a 1 or a 0 at compile time.
> The compiler is only capable of answering the question, "Does the compiler
> always generate a lock free sequence".

Which is exactly the kind of answer sought here.  Is the n2992 reference
incorrect or am I (and bkoz) misinterpreting it?  Are things about to change
again standard-wise?  Let's try and ask mr. Crowl.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #7 from Andrew Macleod <amacleod at redhat dot com> 2012-08-14 13:45:50 UTC ---
If NULL is passed as the second parameter, *both* functions are per type.  They
only become per-object based when an object is actually passed as the second
parameter.   libstdc++ could simply pass NULL as the second parameter rather
than the object, but it shouldn't matter since the C11/C++11 types will have
the atomic attribute set and thus will always be either lock free or not
anyway.  We don't even look at the object pointer in this case. 

Type and size are combined by the first parameter, so the meaning is now
changed to whether something {is|always} lock free for a type of SIZE with the
new atomic attribute (which is to be added soonish).  Since both C++11 and C11
will have the atomic attribute set on memory for their atomic types, this will
work fine.  

If the memory does not have the atomic attribute, then the answer can only be
resolved by using the (second parameter) pointer to the actual object. We'll
look at the type at compile time to see if the size/alignment of that type is
always good, and proceed from there to either generate instructions or call
into libatomic.

__atomic_is_lock_free() is the compiler implementation of the standard method.
This is the one which *must* be used since then answer cannot always be
provided at compile time.  The library MAY provide a lock free implementation
for a type which the compiler cannot, and thus a type may be always lock free
at runtime even if it isn't at compile time.  Thats why the query exists. 
Think of an old binary running on new hardware that provides new lock free
instructions which are utilized in a new library.  __atomic_always_lock_free()
would return the incorrect result and lock free algorithms wouldn't be able to
utilize the new instructions.

__atomic_always_lock_free() is a compiler creation to answer the question of
whether we know at compile time if a type/object is always lock free or not.
This is then resolved early in compilation so the preprocessor can resolve the
set of predefined ATOMIC_*_LOCK_FREE macros.  It is *identical* to
__atomic_is_lock_free except resolves to a FALSE at compile time rather than
turning into a call into libatomic.  

In fact, the compiler implements __atomic_is_lock_free() by (paraphrased):

  if (__atomic_always_lock_free (size, ptr))
    return TRUE;
  else
    emit_call_to_atomic_is_lock_free(size, ptr)



So if a code change is desired (but it isn't required), the 2nd parameter could
be passed as NULL to __atomic_is_lock_free().

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #8 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-08-14 22:16:00 UTC ---
(In reply to comment #7)
> ,,,
> In fact, the compiler implements __atomic_is_lock_free() by (paraphrased):

ITYM *will* implement. :)  Right now we still have PR54004.

> So if a code change is desired (but it isn't required), the 2nd parameter could
> be passed as NULL to __atomic_is_lock_free().

Since a lot of code changes will happen in this area soonish (hopefully), I
guess it's no actual use quoting current documentation or implementation.  I'll
leave it to you and bkoz to fight out whether you want his change reverted
before that happens.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #9 from Andrew Macleod <amacleod at redhat dot com> 2012-08-14 22:44:53 UTC ---
Actually, that's the way __atomic_is_lock_free()  has always been implemented
(even in 4.7).  

The change is simply wrong and needs to be reverted.  __atomic_is_lock_free()
*must* call the library routine of the same name if the compiler can't generate
the lock free sequence.  That's the way the 2 routines were designed to
inter-operate. The current code introduces an new upgrade path bug, so its
fixing a problem in the wrong place and introducing another.

And yes, 54004 is legitimate and separate from this issue. Fixing that by
giving targets a say in what atomic alignment is will resolve this one without
any changes.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #10 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-08-16 22:23:32 UTC ---
(In reply to comment #9)
> Actually, that's the way __atomic_is_lock_free()  has always been implemented
> (even in 4.7).  

There's miscommunication here.  The point of this PR is, the code for
is_lock_free must be per-type, agreed?  It used __atomic_is_lock_free() with a
non-null pointer, hence it was per-object, a bug, agreed?  (Mr. Crowl asserts
on IRC what can be understood from the referenced URL; that the function
started as per-object but semantics later changed to be per-type.)

The committed changes were to make it use __atomic_always_lock_free() so
per-type.
IIUC you mean it should instead use __atomic_is_lock_free() with the object
pointer changed to NULL.  That might be: whether that is preferred to r190216 I
can't say, in particular with a pending rewrite.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[bkoz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

Benjamin Kosnik <bkoz at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |bkoz at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #11 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-08-28 18:10:09 UTC ---

Andrew I'll revert

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #12 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-08-28 21:59:55 UTC ---
(In reply to comment #11)
> Andrew I'll revert

Then please set the object pointer to NULL in the __atomic_is_lock_free
 call.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[bkoz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #13 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-08-30 19:25:03 UTC ---
Author: bkoz
Date: Thu Aug 30 19:24:58 2012
New Revision: 190810

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190810
Log:
2012-08-30  Benjamin Kosnik  <bkoz@redhat.com>

    PR libstdc++/54005 continued
    * include/std/atomic: Use __atomic_lock_free with
    * include/bits/atomic_base.h: Same.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/atomic_base.h
    trunk/libstdc++-v3/include/std/atomic

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2015-03-25
     Ever confirmed|0                           |1

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Can we close this?

(N.B. https://gcc.gnu.org/ml/libstdc++/2015-02/msg00040.html will make further
changes to these functions to fix PR65033, but only to make them account for
mis-aligned types, they will still be per-type not per-object)

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #15 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #14)
> Can we close this?

I can't say now, sorry, but will be back on this in a week.

[Bug libstdc++/54005] Use __atomic_always_lock_free in libstdc++ is_lock_free instead of __atomic_is_lock_free

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54005

--- Comment #16 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #14)
> Can we close this?

No.  IIUC, we're still/again using __atomic_is_lock_free with alignment deduced
from the current object rather than the type (even though it's now a
proxy-object; the faked pointer is constructed from the alignment of the
current object).

So, r221701 was wrong to change from null to the alignment-deduced
fake-pointer.