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

[Thomas Schatzl]

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.

> 
> 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