RFR(xxxs) (jdk10): 8168542: os::realloc should return a valid pointer for input size=0
david.holmes at oracle.com
Mon Mar 6 11:38:18 UTC 2017
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
>> - the code currently passes through the 0 as the number of threads!
> No. Explicitly setting it to zero means zero refinement threads as of
> 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
> 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".
>> 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:
>> rather than:
>> if (p) free(p);
>> So ... I'll concede to go with the first patch.
>> 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]
>>> nsigned long)+0xbb 34 V [libjvm.so+0xb16588]
>>> G1CollectionSet::verify_young_cset_indices() const+0x1a8
>>> Happens in a lot of tests.
>>> 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,
>>> NativeCallStack const&,
>>> 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
> 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.
More information about the hotspot-runtime-dev