[11]RFR: 8203030: Zero s390 31 bit size_t type conflicts in shared code

Tue Jun 19 14:58:01 UTC 2018

Hi Per,
Thanks!

On 19/06/18 10:41 AM, Per Liden wrote:
> Hi Chris,
>
> On 06/19/2018 03:49 PM, Chris Phillips wrote:
>> Hi Per
>> Thanks!
>> New webrev : http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.3
>
> Looks good to me!
>
> /Per
>
>> All suggested changes made (see below inline).
>>
>> On 18/06/18 05:55 AM, Per Liden wrote:
>>> On 06/14/2018 05:01 PM, Chris Phillips wrote:
>>>> Hi
>>>> Any further comments or changes?
>>>> On 06/06/18 05:56 PM, Chris Phillips wrote:
>>>>> Hi Per,
>>>>>
>>>>> On 06/06/18 05:48 PM, Per Liden wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> On 06/06/2018 11:15 PM, Chris Phillips wrote:
>>>>>>> Hi Per,
>>>>>>>
>>>>>>> On 06/06/18 04:47 PM, Per Liden wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> On 06/06/2018 09:36 PM, Chris Phillips wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 06/06/18 02:23 PM, Per Liden wrote:
>>>>>>>>>> On 2018-06-06 18:29, Andrew Haley wrote:
>>>>>>>>>>> On 06/06/2018 04:47 PM, Chris Phillips wrote:
>>>>>>>>>>>> Please review this set of changes to shared code
>>>>>>>>>>>> related to S390 (31bit) Zero self-build type mis-match 
>>>>>>>>>>>> failures.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203030
>>>>>>>>>>>> webrev: 
>>>>>>>>>>>> http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.0
>>>>>>>>>>>
>>>>>>>>>>> Can you explain this a little more?  What is the type of 
>>>>>>>>>>> size_t on
>>>>>>>>>>> s390x?  What is the type of uintptr_t?  What are the errors?
>>>>>>>>>>
>>>>>>>>>> I would like to understand this too.
>>>>>>>>>>
>>>>>>>>>> cheers,
>>>>>>>>>> Per
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Quoting from the original bug  review request:
>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-June/014254.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> "This
>>>>>>>>> is a problem when one parameter is of size_t type and the 
>>>>>>>>> second of
>>>>>>>>> uintx type and the platform has size_t defined as eg. unsigned
>>>>>>>>> long as
>>>>>>>>> on s390 (32-bit)."
>>>>>>>>
>>>>>>>> Please clarify what the sizes of uintx (i.e. uintptr_t) and size_t
>>>>>>>> are
>>>>>>>> on s390?
>>>>>>> See Dan's explanation.
>>>>>>>>
>>>>>>>> I fail to see how any of this matters to _entries here? What am I
>>>>>>>> missing?
>>>>>>>>
>>>>>>>
>>>>>>> By changing the type, to its actual usage, we avoid the
>>>>>>> necessity of patching in
>>>>>>> src/hotspot/share/gc/g1/g1StringDedupTable.cpp
>>>>>>> around line 617, since its consistent usage and local I patched 
>>>>>>> at the
>>>>>>> definition.
>>>>>>>
>>>>>>> - _table->_entries, percent_of(_table->_entries, _table->_size),
>>>>>>> _entry_cache->size(), _entries_added, _entries_removed);
>>>>>>> + _table->_entries, percent_of( (size_t)(_table->_entries),
>>>>>>> _table->_size), _entry_cache->size(), _entries_added,
>>>>>>> _entries_removed);
>>>>>>>
>>>>>>> percent_of will complain about types otherwise.
>>>>>>
>>>>>> Ok, so why don't you just cast it in the call to percent_of? Your
>>>>>> current patch has ripple effects that you fail to take into account.
>>>>>> For
>>>>>> example, _entries is still printed using UINTX_FORMAT and compared
>>>>>> against other uintx variables. You're now mixing types in an unsound
>>>>>> way.
>>>>>
>>>>> Hmm missed that, so will do the cast instead as you suggest.
>>>>> (Fixing at the defn is what was suggested the last time around so I
>>>>> tried to do that where it was consistent, obviously this is not.
>>>>> Thanks.
>>>>>
>>>>>> cheers,
>>>>>> Per
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> src/hotspot/share/gc/g1/g1StringDedupTable.hpp
>>>>>>>> @@ -120,11 +120,11 @@
>>>>>>>>       // Cache for reuse and fast alloc/free of table entries.
>>>>>>>>       static G1StringDedupEntryCache* _entry_cache;
>>>>>>>>
>>>>>>>>       G1StringDedupEntry**            _buckets;
>>>>>>>>       size_t                          _size;
>>>>>>>> -  uintx                           _entries;
>>>>>>>> +  size_t                          _entries;
>>>>>>>>       uintx _shrink_threshold;
>>>>>>>>       uintx _grow_threshold;
>>>>>>>>       bool _rehash_needed;
>>>>>>>>
>>>>>>>> cheers,
>>>>>>>> Per
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hope that helps,
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> (I'll answer further if needed but the info is in the bugs and
>>>>>>>>> review thread mostly)
>>>>>>>>> See:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8203030
>>>>>>>>> and:
>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-June/014254.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046938
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8074459
>>>>>>>>> For more info.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>> Cheers!
>>>>> Chris
>>>>>
>>>>>
>>>>>
>>>>
>>>> Finally through testing and submit run again after Per's requested
>>>> change, here's the knew webrev:
>>>> http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.2
>>>> attached is the passing run fron the submit queue.
>>>>
>>>> Please review...
>>>
>>>
>>> src/hotspot/share/gc/cms/cms_globals.hpp
>>> ----------------------------------------
>>> Instead of changing the type of ParGCDesiredObjsFromOverflowList, I'd
>>> suggest you just change the single place where you need a cast, in
>>> ParScanThreadState::take_from_overflow_stack().
>>>
>>> If you change the type of ParGCDesiredObjsFromOverflowList, but you
>>> otherwise have to clean up a number of places where it's already
>>> explicitly cast to size_t in concurrentMaskSweepGeneration.cpp.
>>>
>> Done
>>
>>>
>>> src/hotspot/share/gc/parallel/parallel_globals.hpp
>>> --------------------------------------------------
>>> Please also change to type of ParallelOldDeadWoodLimiterMean to size_t.
>>>
>>>
>> Done
>>
>>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>>> ---------------------------------------------------
>>> No need to cast ParallelOldDeadWoodLimiterStdDev, you're already 
>>> changed
>>> its type. And if you change ParallelOldDeadWoodLimiterMean to also 
>>> being
>>> size_t you don't need to touch this file at all.
>>>
>>>
>> Done
>>
>>> src/hotspot/share/runtime/globals.hpp
>>> -------------------------------------
>>> -define_pd_global(uintx,  InitialCodeCacheSize,       160*K);
>>> -define_pd_global(uintx,  ReservedCodeCacheSize,      32*M);
>>> +define_pd_global(size_t,  InitialCodeCacheSize,       160*K);
>>> +define_pd_global(size_t,  ReservedCodeCacheSize,      32*M);
>>>
>>> I would avoid changing these types, otherwise you need to go around and
>>> clean up a number of other places where it's says it's an uintx, 
>>> like here:
>>>
>>> 1909   product_pd(uintx, InitialCodeCacheSize,         \
>>> 1910           "Initial code cache size (in bytes)")         \
>>> 1911           range(os::vm_page_size(), max_uintx)         \
>>>
>>> Also, it seems you've already added the cast you need for
>>> InitialCodeCacheSize in codeCache.cpp, so that type change looks
>>> unnecessary.
>>>
>> Done
>>
>>
>>> Btw, patch no longer applies to the latest jdk/jdk.
>>
>> Should work now, again.
>> Tested on s390 31 bit and x86_64  64 bit, testing on s390x underway.
>>>
>>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> Chris
>>>>
>>>
>>>
>>>
>>
>> Cheers!
>> Chris
>>
>
>
>
Cheers!

Chris