[11]RFR: 8203030: Zero s390 31 bit size_t type conflicts in shared code
Chris Phillips
ChrisPhi at LGonQn.Org
Wed Jun 20 20:24:42 UTC 2018
Hi David,
On 20/06/18 07:15 AM, David Holmes wrote:
> Hi Chris,
>
> Reviewed.
>
Thanks!
> I think there is an underlying problem in mixing uintx types and size_t
> types, but that's not your problem to fix here. The casts deal with the
> immediate issue.
> I agree, thanks again.
> Thanks,
> David
Cheers!
Chris
>
> On 20/06/2018 7:38 PM, Chris Phillips wrote:
>>
>> Hi
>> Another reviewer?
>>
>> On 19/06/18 10:58 AM, "Chris Phillips"@T O wrote:
>>> 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
>>>
>>>
>>>
>>>
>>
>> Ready to go :
>>
>> http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.4
>>
>> Build Details: 2018-06-19-1559053.chrisphi.source
>> 0 Failed Tests
>> Mach5 Tasks Results Summary
>>
>> FAILED: 0
>> KILLED: 0
>> UNABLE_TO_RUN: 0
>> NA: 0
>> PASSED: 75
>> EXECUTED_WITH_FAILURE: 0
>>
>>
>> Chris
>>
>
>
>
More information about the hotspot-dev
mailing list