RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot

[Kim Barrett]

> On Oct 13, 2017, at 2:34 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> Hi, Here is the version with the changes from Kim's comments that has passed at least testing with JPRT and tier1, locally.   More testing (tier2-5) is in progress.
> 
> Also includes a corrected version of Atomic::sub care of Erik Osterlund.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.kim-review-changes/webrev
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.review-comments/webrev
> 
> Full version:
> 
> http://cr.openjdk.java.net/~coleenp/8188220.03/webrev
> 
> Thanks!
> Coleen

I still dislike and disagree with what is being proposed regarding replace_if_null.

------------------------------------------------------------------------------
I forgot that I'd promised you an updated Atomic::sub definition.
Unfortunately, the new one still has problems, performing some
conversions that should not be permitted (and are disallowed by
Atomic::add).  Try this instead.  (This hasn't been tested, not even
compiled; hopefully I don't have any typos or anything.)  The intent
is that this supports the same conversions as Atomic::add.

template<typename I, typename D>
inline D Atomic::sub(I sub_value, D volatile* dest) {
  STATIC_ASSERT(IsPointer<D>::value || IsIntegral<D>::value);
  STATIC_ASSERT(IsIntegral<I>::value);
  // If D is a pointer type, use [u]intptr_t as the addend type,
  // matching signedness of I.  Otherwise, use D as the addend type.
  typedef typename Conditional<IsSigned<I>::value, intptr_t, uintptr_t>::type PI;
  typedef typename Conditional<IsPointer<D>::value, PI, D>::type AddendType;
  // Only allow conversions that can't change the value.
  STATIC_ASSERT(IsSigned<I>::value == IsSigned<AddendType>::value); 
  STATIC_ASSERT(sizeof(I) <= sizeof(AddendType));
  AddendType addend = sub_value;
  // Assumes two's complement integer representation.
  #pragma warning(suppress: 4146) // In case AddendType is not signed.
  return Atomic::add(-addend, dest);
}

>>> src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
>>> 7960   Atomic::add(-n, &_num_par_pushes);
>>> 
>>> Atomic::sub
>> 
>> fixed.

Nope, not fixed in http://cr.openjdk.java.net/~coleenp/8188220.03/webrev

>>> src/hotspot/share/gc/g1/heapRegionRemSet.cpp
>>>   200       PerRegionTable* res =
>>>   201         Atomic::cmpxchg(nxt, &_free_list, fl);
>>> 
>>> Please remove the line break, now that the code has been simplified.
>>> 
>>> But wait, doesn't this alloc exhibit classic ABA problems?  I *think*
>>> this works because alloc and bulk_free are called in different phases,
>>> never overlapping.
>> 
>> I don't know.  Do you want to file a bug to investigate this?
>> fixed.

No, I now think it’s ok, though confusing.

>>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>>   295     SparsePRT* res =
>>>   296       Atomic::cmpxchg(sprt, &_head_expanded_list, hd);
>>> and
>>>   307     SparsePRT* res =
>>>   308       Atomic::cmpxchg(next, &_head_expanded_list, hd);
>>> 
>>> I'd rather not have the line breaks in these either.
>>> 
>>> And get_from_expanded_list also appears to have classic ABA problems.
>>> I *think* this works because add_to_expanded_list and
>>> get_from_expanded_list are called in different phases, never
>>> overlapping.
>> 
>> Fixed, same question as above?  Or one bug to investigate both?

Again, I think it’s ok, though confusing.

>>> src/hotspot/share/gc/shared/taskqueue.inline.hpp
>>>   262   return (size_t) Atomic::cmpxchg((intptr_t)new_age._data,
>>>   263                                   (volatile intptr_t *)&_data,
>>>   264 (intptr_t)old_age._data);
>>> 
>>> This should be
>>> 
>>>    return Atomic::cmpxchg(new_age._data, &_data, old_age._data);
>> 
>> fixed.

Still casting the result.

>>> src/hotspot/share/oops/method.hpp
>>>   139   volatile address from_compiled_entry() const   { return OrderAccess::load_acquire(&_from_compiled_entry); }
>>>   140   volatile address from_compiled_entry_no_trampoline() const;
>>>   141   volatile address from_interpreted_entry() const{ return OrderAccess::load_acquire(&_from_interpreted_entry); }
>>> 
>>> [pre-existing]
>>> The volatile qualifiers here seem suspect to me.
>> 
>> Again much suspicion about concurrency and giant pain, which I remember, of debugging these when they were broken.

Let me be more direct: the volatile qualifiers for the function return
types are bogus and confusing, and should be removed.

>>> src/hotspot/share/prims/jni.cpp
>>> 
>>> [pre-existing]
>>> 
>>> copy_jni_function_table should be using Copy::disjoint_words_atomic.
>> 
>> yuck.

Of course, neither is entirely technically correct, since both are
treating conversion of function pointers to void* as okay in shared
code, e.g. violating some of the raison d'etre of CAST_{TO,FROM}_FN_PTR.
For way more detail than you probably care about, see the discussion
starting here:
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018578.html
through (5 messages in total)
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018623.html

Oh well.

>>> src/hotspot/share/runtime/mutex.hpp
>>> 
>>> [pre-existing]
>>> 
>>> I think the Address member of the SplitWord union is unused. Looking
>>> at AcquireOrPush (and others), I'm wondering whether it *should* be
>>> used there, or whether just using intptr_t casts and doing integral
>>> arithmetic (as is presently being done) is easier and clearer.
>>> 
>>> Also the _LSBINDEX macro probably ought to be defined in mutex.cpp
>>> rather than polluting the global namespace.  And technically, that
>>> name is reserved word.
>> 
>> I moved both this and _LBIT into the top of mutex.cpp since they are used there.

Good.

>> Cant define const intptr_t _LBIT =1; in a class in our version of C++.

Sorry, please explain?  If you tried to move it into SplitWord, that doesn’t work;
unions are not permitted to have static data members (I don’t off-hand know why,
just that it’s explicitly forbidden).

And you left the seemingly unused Address member in SplitWord.

>>> src/hotspot/share/runtime/thread.cpp
>>> 4707   intptr_t w = Atomic::cmpxchg((intptr_t)LOCKBIT, Lock, (intptr_t)0);
>>> 
>>> This and other places suggest LOCKBIT should be defined as intptr_t,
>>> rather than as an enum value.  The MuxBits enum type is unused.
>>> 
>>> And the cast of 0 is another case where implicit widening would be nice.
>> 
>> Making LOCKBIT a const intptr_t = 1 removes a lot of casts.

Because of the new definition of LOCKBIT I noticed the immediately
preceeding typedef for MutexT, which seems to be unused.

------------------------------------------------------------------------------
src/hotspot/share/oops/cpCache.cpp
 114 bool ConstantPoolCacheEntry::init_flags_atomic(intx flags) {
 115   intptr_t result = Atomic::cmpxchg(flags, &_flags, (intx)0);
 116   return (result == 0);
 117 }

[I missed this on earlier pass.]

Should be

bool ConstantPoolCacheEntry::init_flags_atomic(intx flags) {
  return Atomic::cmpxchg(flags, &_flags, (intx)0) == 0;
}

Otherwise, I end up asking why result is intptr_t when the cmpxchg is
dealing with intx.  Yeah, one's a typedef of the other, but mixing
them like that in the same expression is not helpful.