[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

--- Comment #37 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #36)
> Either I goofed on the ChangeLog markup

I think it's because you use "PR libstdc++-v3/n" not "PR libstdc++/n"

> As the tests I wrote have no bearing on the code change, I did not commit
> them.
> (If you still want them committed, please tell.)

No need. Thanks.

[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

Hans-Peter Nilsson  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED
   Target Milestone|--- |9.0

--- Comment #36 from Hans-Peter Nilsson  ---
Either I goofed on the ChangeLog markup or the hook posting bugzilla notes from
svn commits has taken some time off.  Anyway, I committed the change approved
in comment #35 as r266018 (and a changelog goof in r266019).  It was also
posted in .

As the tests I wrote have no bearing on the code change, I did not commit them.
(If you still want them committed, please tell.)

[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

--- Comment #35 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #34)
> Can I at least change 
> - reinterpret_cast(-__alignof(_M_i)));
> + reinterpret_cast(-_S_alignment));
> ?

Yes, OK.

[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 #34 from Hans-Peter Nilsson  ---
(In reply to Jonathan Wakely from comment #32)
> I would prefer a function template instead of the LF macro e.g.

That will lose the __LINE__ information from the supposed error message in
VERIFY, i.e. it'll point at the check function rather than the unique invoking
line.

> So the premise that "is_lock_free() is per-type implies it's the same as
> always_lock_free" is wrong.

Ouch.  That again exposes the __is_lock_free() flaws.  But that's for another
bug.

Can I at least change 
-   reinterpret_cast(-__alignof(_M_i)));
+   reinterpret_cast(-_S_alignment));
?

[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

--- Comment #33 from Jonathan Wakely  ---
std::atomic already meets the "same for all objects of the type" guarantee
by fixing the alignment of its member and by using a fake pointer that has the
same value for all objects of the type.

[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

--- Comment #32 from Jonathan Wakely  ---
Comment on attachment 44851
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=44851
patch as per directions

That looks like sufficient variety of types tested.

I would prefer a function template instead of the LF macro e.g.

template
void check()
{
  std::atomic a;
  VERIFY( a.is_lock_free() || !a.is_always_lock_free );
}

and then:

  check< Foo * >();
  check< Bar * >();

etc.

But I'm losing confidence in this change being correct, after asking some
questions of the C++ committee. The intent seems to be that is_lock_free() can
indeed give a runtime answer, based on properties which might not have been
known at compile-time. For example, code compiled with -march=i386 will say
is_always_lock_free is false (correctly) but if at runtime it links to
libatomic compiled for i686 and it runs on i686 then is_lock_free could return
true. So the link failure in comment 30 is right, and I was wrong to say:

> The result should be the same as ax.is_always_lock_free which is a constant.

This is the point Andrew made all the way back in Comment 2. At compile-time we
might not be able to guarantee lock-freedom, but at run-time libatomic might be
able to **and that will be true for all suitably-aligned objects of that
type**.

So the premise that "is_lock_free() is per-type implies it's the same as
always_lock_free" is wrong.

[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 #31 from Hans-Peter Nilsson  ---
Created attachment 44851
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=44851=edit
patch as per directions

Thanks.  I also adjusted include/std/atomic, without which the struct X
test-case would not be impacted; only "base integral types".  I hope 54005.cc
contains sufficient variety, please advise if not.

Will post this to lists after testing.

[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  changed:

   What|Removed |Added

 Status|WAITING |ASSIGNED
   Assignee|unassigned at gcc dot gnu.org  |hp at gcc dot gnu.org

--- Comment #30 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #26)
> I went by the documentation, which says at r264855 for __atomic_is_lock_free
> that "If the built-in function is not known to be lock-free, a call is made
> to a runtime routine named @code{__atomic_is_lock_free}."  It certainly
> seems to be that way too (builtins.c):
> 
> static tree
> fold_builtin_atomic_is_lock_free (tree arg0, tree arg1)
> {
>   if (!flag_inline_atomics)
> return NULL_TREE;
>   
>   /* If it isn't always lock free, don't generate a result.  */
>   if (fold_builtin_atomic_always_lock_free (arg0, arg1) == boolean_true_node)
> return boolean_true_node;
> 
>   return NULL_TREE;
> }
> 
> ISTM that this will not "inline" a return of "false".

Indeed. This fails to link without -latomic:

#include 

struct X {
  alignas(64) char x;
};

int main()
{
  std::atomic ax;
  return ax.is_lock_free();
}

So even if the call in libatomic would give the right answer, depending on
libatomic is unnecessary. The result should be the same as
ax.is_always_lock_free which is a constant.

So let's go with your patch to  to change to:

return __atomic_always_lock_free(sizeof(_M_i),
reinterpret_cast(-_S_alignment));

Please also add some tests to the libstdc++ testsuite confirming that
a.is_lock_free() is consistent with a.is_always_lock_free for a variety of
types with different sizes and alignments.

[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

--- Comment #29 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #27)
> (In reply to Jonathan Wakely from comment #24)
> > (In reply to Hans-Peter Nilsson from comment #22)
> > > Or do I misread that?  Are __alignof(x) and the result of alignas(x)
> > > in the declaration guaranteed to always be the same here?
> > 
> > Yes.
> 
> For the combination of alignof and alignas in *this* code it's not
> obvious to me.  I can imagine that (for example) the alignment of a
> container

What do you mean by container? The std::atomic class template?


> can affect the __alignof(x) such that it's (for example)
> higher than the specifically alignas declaration of x, likely by bug,

Yes, that would be a compiler bug.

> less likely by intent.  IOW, to me, this isn't the alignas(type) ===
> alignas(alignof(type)) in
> .

Because we're not relying on that. We're using GCC's __alignof extension to ask
for the alignment of an lvalue object, not a type.

https://gcc.gnu.org/onlinedocs/gcc/Alignment.html

That page should probably be amended to also mention 'alignas' when it says
"taking into account any minimum alignment specified with GCC’s __attribute__
extension".

The alignas(_S_alignment) specifies the alignment for the _M_i member, and
__alignof gives that same value back. We could verify that with:

  static_assert(__alignof(_M_i) == _S_alignment, "sanity check");

Given the history of bugs in this code, I'm reluctant to make any larger
changes here without a test showing an actual bug.

[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

--- Comment #28 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #27)
> BTW, why use __alignof and not alignof?

Because alignof can only be used with a type and we use it with _M_i.

[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 #27 from Hans-Peter Nilsson  ---
(In reply to Jonathan Wakely from comment #24)
> (In reply to Hans-Peter Nilsson from comment #22)
> > Or do I misread that?  Are __alignof(x) and the result of alignas(x)
> > in the declaration guaranteed to always be the same here?
> 
> Yes.

For the combination of alignof and alignas in *this* code it's not
obvious to me.  I can imagine that (for example) the alignment of a
container can affect the __alignof(x) such that it's (for example)
higher than the specifically alignas declaration of x, likely by bug,
less likely by intent.  IOW, to me, this isn't the alignas(type) ===
alignas(alignof(type)) in
.

Either way, whether the guarantee is by C++ standard wording or by gcc
internals documentation carries not more value than the documentation
for __atomic_is_lock_free when it comes to maintainer reading, heh...

So, what I'd like to see is either:

- This change in atomic_base.h, avoiding use of the specific object in
is_lock_free and changing to __atomic_always_lock_free:

Index: atomic_base.h
===
--- atomic_base.h   (revision 264855)
+++ atomic_base.h   (working copy)
@@ -355,7 +355,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
// Use a fake, minimally aligned pointer.
return __atomic_is_lock_free(sizeof(_M_i),
-   reinterpret_cast(-__alignof(_M_i)));
+   reinterpret_cast(-_S_alignment));
   }

   bool
@@ -363,7 +363,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
// Use a fake, minimally aligned pointer.
return __atomic_always_lock_free(sizeof(_M_i),
-   reinterpret_cast(-__alignof(_M_i)));
+   reinterpret_cast(-_S_alignment));
   }

   _GLIBCXX_ALWAYS_INLINE void

(Note the change to __atomic_always_lock_free.  BTW, why use __alignof and not
alignof?  No underscores in the _S_alignment expression!)

-or-

- at least a test in the C++ test-suite with at least a construct of
the exact same form as the one in atomic_base.h but with an overaligned
container and an assert that the alignof the inner object equals the
alignas expression, as you asserted above.  I know that'd cover only
the first case of alignment-bleed that comes to mind, but it'd
indicate an intent to future hackers.

- optionally: wording in the gcc documentation to the effect of the
__alignof === alignas guarantee.  (Still, a test-case is more reliable
than gcc documentation.)  But, there's probably sufficient wording in the
standard.

Maybe some of that is already in place; if it isn't, I'll produce
patches (and CC you, hoping for speedier review).

Do we have common ground here?

[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 #26 from Hans-Peter Nilsson  ---
(In reply to Jonathan Wakely from comment #25)
> (In reply to Hans-Peter Nilsson from comment #23)
> > ...and also, a call might be generated as the result of using
> > __atomic_is_lock_free (instead of __atomic_always_lock_free), so the target
> > may change its mind.  Not good.
> 
> That should have been fixed by r227878 for Bug 65913 so that for these cases
> no call is generated.

I went by the documentation, which says at r264855 for __atomic_is_lock_free
that "If the built-in function is not known to be lock-free, a call is made to
a runtime routine named @code{__atomic_is_lock_free}."  It certainly seems to
be that way too (builtins.c):

static tree
fold_builtin_atomic_is_lock_free (tree arg0, tree arg1)
{
  if (!flag_inline_atomics)
return NULL_TREE;

  /* If it isn't always lock free, don't generate a result.  */
  if (fold_builtin_atomic_always_lock_free (arg0, arg1) == boolean_true_node)
return boolean_true_node;

  return NULL_TREE;
}

ISTM that this will not "inline" a return of "false".

[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

--- Comment #25 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #23)
> ...and also, a call might be generated as the result of using
> __atomic_is_lock_free (instead of __atomic_always_lock_free), so the target
> may change its mind.  Not good.

That should have been fixed by r227878 for Bug 65913 so that for these cases no
call is generated.

Without a testcase showing the wrong thing happening, I still think the right
thing happens here.

[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

--- Comment #24 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #22)
> Or do I misread that?  Are __alignof(x) and the result of alignas(x)
> in the declaration guaranteed to always be the same here?

Yes.

[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 #23 from Hans-Peter Nilsson  ---
(In reply to Hans-Peter Nilsson from comment #22)
> Anyway, as per the arguments in n2992.html in the link above, I (still)
> think __atomic_*always*_lock free should be used instead; this'd make the
> intent clear.

...and also, a call might be generated as the result of using
__atomic_is_lock_free (instead of __atomic_always_lock_free), so the target may
change its mind.  Not good.

[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 #22 from Hans-Peter Nilsson  ---
The issue again, is using properties of the actual object in the value of
is_lock_free().
AFAICS, that still happens (at r264855)

libstdc++-v3/include/bits/atomic_base.h:246
  static constexpr int _S_alignment =
sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);

  alignas(_S_alignment) __int_type _M_i;

libstdc++-v3/include/bits/atomic_base.h:353
  bool
  is_lock_free() const noexcept
  {
// Use a fake, minimally aligned pointer.
return __atomic_is_lock_free(sizeof(_M_i),
reinterpret_cast(-__alignof(_M_i)));
  }

To me, is_lock_free doesn't seem to be constant over all objects of
the same type, when the expression to __atomic_is_lock_free is based
on the particular object; the fake-pointer (void*) -__alignof(_M_i).

Or do I misread that?  Are __alignof(x) and the result of alignas(x)
in the declaration guaranteed to always be the same here?  This seems
to be the crux.  (If so, can we add an assert somehow to make this
clearer?)

Anyway, as per the arguments in n2992.html in the link above, I (still) think
__atomic_*always*_lock free should be used instead; this'd make the intent
clear.

[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  changed:

   What|Removed |Added

   Assignee|bkoz at gcc dot gnu.org|unassigned at gcc dot 
gnu.org

--- Comment #21 from Jonathan Wakely  ---
(In reply to Eric Gallager from comment #19)
> (In reply to Jonathan Wakely from comment #18)
> > I still want to close this.
> 
> With what resolution? FIXED? WORKSFORME?

FIXED, I think, as we do the right thing now.

But there's no rush, I'd rather wait and get HP's input.

[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 #20 from Hans-Peter Nilsson  ---
Please allow a couple of days so I can catch up. 
Thanks.

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

[egallager at gcc dot gnu.org]

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

--- Comment #19 from Eric Gallager  ---
(In reply to Jonathan Wakely from comment #18)
> (In reply to Hans-Peter Nilsson from comment #16)
> > (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).
> 
> It's constructed from __alignof(_M_i) and since r221945 that is given a
> fixed alignment:
> 
>   static constexpr int _S_alignment =
>   sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);
> 
>   alignas(_S_alignment) __int_type _M_i;
> 
> So it will be the same for all objects of the type.
> 
> There was an additional fix in r227878 for Bug 65913.
> 
> > So, r221701 was wrong to change from null to the alignment-deduced
> > fake-pointer.
> 
> I think the current code is right, and per-type.
> 
> I still want to close this.

With what resolution? FIXED? WORKSFORME?

[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

--- Comment #18 from Jonathan Wakely  ---
(In reply to Hans-Peter Nilsson from comment #16)
> (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).

It's constructed from __alignof(_M_i) and since r221945 that is given a fixed
alignment:

  static constexpr int _S_alignment =
sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);

  alignas(_S_alignment) __int_type _M_i;

So it will be the same for all objects of the type.

There was an additional fix in r227878 for Bug 65913.

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

I think the current code is right, and per-type.

I still want to close this.

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

[egallager at gcc dot gnu.org]

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

Eric Gallager  changed:

   What|Removed |Added

 CC||egallager at gcc dot gnu.org

--- Comment #17 from Eric Gallager  ---
(In reply to Hans-Peter Nilsson from comment #16)
> (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.

So, if we can't close, does it make sense to still leave it in WAITING? WAITING
implies a threat to close if what's being waited upon isn't produced.

[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.

[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

[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

[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=gccview=revrev=190810
Log:
2012-08-30  Benjamin Kosnik  b...@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

[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

[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

[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

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 #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

[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=gccview=revrev=190216
Log:
2012-08-07  Benjamin Kosnik  b...@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