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

[Mikael Gerdin]

Hi Volker, Johannes

On 2015-01-09 19:23, Volker Simonis wrote:
> 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/

Good catch!

> https://bugs.openjdk.java.net/browse/JDK-8068739
>
> I've already review the change and think it's good.

+1, Reviewed.

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

Indeed. Unfortunately I'm not surprised, a lot of G1 code is still very 
messy and full of strange hacks which have no clear explanation.

>
> 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 sort of disagree here, I'd prefer to keep initializations in 
initializer lists where it's possible since it implies that there are no 
special dependencies between the initializations. If I find 
initializations in the constructor body I expect to find some sort of 
conditionals or error handling code.

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

Indeed. Can you please file an issue on JDK 9 for that and assign it to 
yourself (or the appropriate SAP engineer) if you guys plan to work on it?

/Mikael


>
> Regards,
> Volker
>