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

[Leonid Mesnik]

Kim, Thomas

Thank you for review and comments. I updated name of class as you suggested and pushing fix.

Leonid

> On Aug 21, 2018, at 9:06 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> 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/ <http://cr.openjdk.java.net/~lmesnik/8209758/webrev.00/>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209758 <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180821/f94e3252/attachment.htm>