RFR(xxxs) (jdk10): 8168542: os::realloc should return a valid pointer for input size=0

[David Holmes]

Hi Thomas,

On 6/03/2017 8:58 PM, Thomas Schatzl wrote:
> Hi all,
>
>   some comments:
>
> On Wed, 2017-03-01 at 14:11 +1000, David Holmes wrote:
>> Hi Thomas,
>>
>> Thanks for all the work you have done on this.
>>
>> I'm disappointed that the assertions on size==0 yielded so many
>> problem areas. I think in many cases these do indicate actual bugs in
>> the logic - eg I'm not sure what a GrowableArray of size 0 is
>> supposed to represent! But not sure it is worth spending the cycles
>> trying to address this.
>>
>> BTW the handling of G1ConcRefinementThreads=0 is definitely a bug in
>> the argument processing logic. 0 means to set ergonomically, so it
>> should either be handled thusly if explicitly set or else explicitly
>> disallowed
>> - the code currently passes through the 0 as the number of threads!
>
> No. Explicitly setting it to zero means zero refinement threads as of
> current.
>
> Refinement is an optimization, as in "G1 does not absolutely need
> refinement threads for operation", and this switch is part of the
> incantation to disable them. The mutators will do the refinement
> themselves.
>
> There may be an inconsistency with the meaning of "0" when set
> explicitly for this kind of threads compared to others.

It is very bad form to have 0 as a default mean one thing, and 0 as an 
explicit value meaning something else IMHO. If 0 is a legal value then 
-1, or some other value should have been chosen to mean "use ergonomics".

David
-----

>>
>> I do not have an issue with free(NULL) as that has always been
>> well-defined and avoids checking the pointer twice when not-NULL. I
>> prefer to see:
>>
>> free(p);
>>
>> rather than:
>>
>> if (p) free(p);
>>
>> So ... I'll concede to go with the first patch.
>>
>> Thanks,
>> David
>> -----
>>
>> On 27/02/2017 9:20 PM, Thomas Stüfe wrote:
>>>
>>> Hi all,
>>>
>>> thanks for all your input, and sorry for the delay!
>>>
> [...]
>>> Here are some examples of asserts:
>>>
>>> --
>>>
>>> 1) 33 V  [libjvm.so+0xb17625]
>>>  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(u
>>> nsigned long)+0xbb 34 V  [libjvm.so+0xb16588]
>>>  G1CollectionSet::verify_young_cset_indices() const+0x1a8
>>>
>>> Happens in a lot of tests.
>>> G1CollectionSet::verify_young_cset_indices()
>>> is called when G1CollectionSet::_collection_set_cur_length is zero.
>>> I assume this is fine, verification could just be skipped at this
>>> point, so the fix would be trivial.
>
> Verification also does not do anything in this case, i.e. the code then
> iterates over zero regions.
>
> There is another related situation with G1RemSetSummary::initialize().
> In case of zero refinement threads, the code also allocates a zero-
> sized array to avoid special casing this situation throughout. The code
> is correct.
>
>>> --
>>>
>>> gc/arguments/TestG1ConcRefinementThreads: VM is called with
>>> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>>
>>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long,
>>> MemoryType,
>>> NativeCallStack const&,
>>> AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>>>  33 V  [libjvm.so+0x954689]
>>>  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>>>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>>>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>>
>>> Not sure at which layer one would handle this. If
>>> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should
>>> this setting not just be forbidden?
>
> See above.
>
> Imho (and just rambling without considering any consequences of an
> implementation):
>
> Somewhere in this thread there has been a comment to let malloc(0)
> return a sentinel value that can be checked for - that seems to be most
> preferable to me.
> The reason for the malloc(0) call is that all of these violations seem
> to be about allocating zero-sized arrays, and AllocateHeap et al. just
> pass on whatever they get as input values. I do not see zero-sized
> arrays to be something wrong.
>
> Or just add a thin "array" abstraction in hotspot that allows that.
>
> Practical considerations like catching potential errors or performance
> might make that one undesired though.
>
> Thanks,
>   Thomas
>