[11]RFR: 8203030: Zero s390 31 bit size_t type conflicts in shared code
David Holmes
david.holmes at oracle.com
Wed Jun 20 11:15:55 UTC 2018
Hi Chris,
Reviewed.
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.
Thanks,
David
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