RFR(S): 8068739: G1CollectorPolicy uses uninitialized field '_sigma' in the constructor

[Volker Simonis]

Hi,

could somebody please review and sponsor the following fix which was
contributed by my colleague Johannes Scheerer:

http://cr.openjdk.java.net/~simonis/webrevs/2015/8068739/
https://bugs.openjdk.java.net/browse/JDK-8068739

I've already review the change and think it's good.

The G1CollectorPolicy constructor uses a huge initializer list. In
this list it calls "new SurvRateGroup(this, ..)" passing it a
reference to the not yet fully initialized G1CollectorPolicy instance.

Via the following call chain, the SurvRateGroup constructor calls back
to G1CollectorPolicy::get_new_prediction() which uses the uninitialzed
value of G1CollectorPolicy::_sigma:

G1CollectorPolicy::G1CollectorPolicy
SurvRateGroup::SurvRateGroup
SurvRateGroup::reset
SurvRateGroup::all_surviving_words_recorded
G1CollectorPolicy::get_new_prediction

Depending on the indefinite value of '_sigma' this can lead to
situations, where a GC is triggered before the whole system is
initialized resulting in the following crash:

Error occurred during initialization of VM
GC triggered before VM initialization completed. Try increasing
NewSize, current value 5324K.

This is a "day 1" problem in G1. The funny thing is that the Visual
Studio compiler specially warns about this dangarous usage of the
'this' pointer in the initializer list ("Compiler Warning C4355:
'this' : used in base member initializer list") but unfortunately the
warning was disable in the file.

I have re-enabled this warning for g1CollectorPolicy.cpp because after
the fix it isn't necessary any more. The call to "new
SurvRateGroup(this, ..)" has been moved from the initializer list into
the constructor body. Notice that it wouldn't have helped to
initialize '_sigma' in the initializer list before the call to "new
SurvRateGroup(this, ..)" because according to the C++ standard,
members are initialized in the order how they are defined in a class
(and not in the initializer list order).

I also can not understand why we have these huge initializer list for
many gc related class constructors. As far as I understand, this only
makes sense for class members because that saves a call to the default
constructor and one assignment. For basic and pointer types this makes
no sense however. Taking into account the maybe unexpected
initialization order of initialization lists, I would strongly
recommend to move all the member initializations into the constructor
bodies.

I also noticed that there are several other files which have disabled
warning 4355 and which use a not fully initialized 'this' pointer in
the initializer list:

src/share/vm/gc_implementation/g1/concurrentMark.cpp:#pragma warning(
disable:4355 )
src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp:#pragma warning(
disable:4355 )
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp:#pragma warning(
disable:4355 )
src/share/vm/gc_implementation/g1/satbQueue.cpp:#pragma warning( disable:4355 )
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp:#pragma
warning( disable:4355 )
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp:#pragma
warning( disable:4355 )

I think it would be good idea to review all these locations, refactor
them as suggested above and re-enable the warnings afterwards.

Regards,
Volker