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

Tue Jun 19 15:21:31 UTC 2018

Hi Thomas
Thanks for your comment!

On 18/06/18 08:20 AM, Thomas Stüfe wrote:
> 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

Agreed , the POSIX definition of size_t is "the result of a sizeof op"[1].
But I think that's a different issue than the current bug
(just making types cop-ascetic for s390 31 bit where they made size_t
unsigned long int  which is technically compatible but causes issues with
compilers).

Do you want to file a bug for that?

Cheers, Chris

[1]

      <stddef.h> - The Open Group Library
      <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stddef.h.html>

pubs.opengroup.org/onlinepubs/9699919799/basedefs/stddef.h.html
This volume of /POSIX/.1-2017 defers to the ISO C standard. [Option 
End]. The <stddef.h> ... /size_t/: Unsigned integer type of the result 
of the /sizeof/ operator.
>
> 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
>>
>