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

[Chris Phillips]

On 14/06/18 11:01 AM, 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...
>
> Chris
>
Hi
Please may I have another review
and someone to push ?

Thanks!
Chris

Hmm attachments stripped...

Here it is inline:

Build Details: 2018-06-14-1347454.chrisphi.source
0 Failed Tests
Mach5 Tasks Results Summary

    PASSED: 75
    KILLED: 0
    FAILED: 0
    UNABLE_TO_RUN: 0
    EXECUTED_WITH_FAILURE: 0
    NA: 0