RFR: 8209758: 2 classes with same name G1PrintCollectionSetClosure cause crash when logging is enabled

[Kim Barrett]

> On Aug 21, 2018, at 4:27 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Mon, 2018-08-20 at 15:25 -0700, Leonid Mesnik wrote:
>> Hi
>> 
>> Could you please review small fix which rename
>> class G1PrintCollectionSetClosure in g1CollectionSet.cpp.
>> 
>> While I was developing test for JDK-8209150 [TESTBUG] Add logging to
>> verify JDK-8197901 to a different test 
>> I observed SEGV on macosx. Actually a lot of tests fail because of
>> this same issue when logging is enabled.
>> 
>> 
> [...]
>> because class 'G1PrintCollectionSetClosure' is used from
>> http://hg.openjdk.java.net/jdk/jdk/file/d96e6839e83d/src/hotspot/shar
>> e/gc/g1/g1CollectionSet.cpp#l331
>> instead of 
>> http://hg.openjdk.java.net/jdk/jdk/file/d96e6839e83d/src/hotspot/shar
>> e/gc/g1/g1CollectedHeap.cpp#l2751
>> so it tries to do completely wrong thing… 

Good find.  I remember helping someone a while back track down one of
these; it was a rather hair pulling experience.

>> I think it would be enough just to rename one of classes to avoid
>> confusion.
> 
> Although I think this is a clang compiler bug, let's rename it…

This is an ODR violation, which is UB and no diagnostic required.
I think there’s a gcc compiler/linker option to check for such, but we’re not
turning it on.  Use of anonymous namespaces to limit the scope of such
“local” classes would also avoid such problems, but there are apparently
other problems with those.

>> webrev: http://cr.openjdk.java.net/~lmesnik/8209758/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8209758
>> I run gc & serviceability tests with enabled logging to verify that
>> they work fine now.
> 
> 
> ... however I would prefer if the closure were named
> G1PrintCollectionSetDetailClosure. While not a lot better, the proposed
> name seems awkward to read to me.

That seems like a (slightly) better name to me too.

> No need for re-review if you change that.
> 
> Looks good.

Looks good to me too.

> Thanks,
>  Thomas