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

[Chris Phillips]

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