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

[Thomas Stüfe]

Hi Chris,

it may be just me, but I dislike a bit the usage of "size_t" for
"number of things". size_t, to me, will always mean a memory range.

Best Regards, Thomas

On Fri, Jun 15, 2018 at 5:36 PM, Chris Phillips <ChrisPhi at lgonqn.org> wrote:
> 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
>