RFR: 8229278: Improve hs_err location printing to assume less about GC internals

[Erik Osterlund]

Hi Kim,

Thanks for reviewing this.

On 28 Aug 2019, at 01:07, Kim Barrett <kim.barrett at oracle.com> wrote:

>> On Aug 27, 2019, at 10:49 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> 
>> Hi Stefan,
>> 
>> Thank you for the review. I like your cleanups. I folded them in to the next webrev: http://cr.openjdk.java.net/~eosterlund/8229278/webrev.01/
> 
> ------------------------------------------------------------------------------
> 
> It's kind of annoying that we have both locationPrinter.hpp and
> locationPrinter.inline.hpp.  I think locationPrinter.hpp probably
> isn't useful without the .inline file also being included.
> 
> I was going to suggest merging into the .hpp and eliminating the
> .inline.hpp, but noticed the use of compressedOops.inline.hpp.
> Maybe go the other way, no .hpp file?  That's a little odd, but I
> think there are some other places where that's been done.
> 
> Your call on doing anything with this comment.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/collectedHeap.hpp
> 
> Removed pure virtual block_start and block_is_obj.  However, some of
> the derived classes still (now unnecessarily) declare their
> implementations virtual.  (Too bad they aren't declared using the
> C++11 "override" virtual specifier.)

Ahhh yes. I will remove the unnecessary virtual specifiers.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/z/zCollectedHeap.cpp
> 352 bool ZCollectedHeap::print_location(outputStream* st, void* addr) const {
> 353   if (LocationPrinter::is_valid_obj(addr)) {
> ...
> 358   }
> 359   return false;
> 360 }
> 
> ZGC won't attempt to determine whether addr is a pointer into the
> middle of an object and print something useful in that case, correct?

Correct.

> I'm not sure whether it was previously attempting to do so either; I
> don't really want to study the code being deleted. :)

No it failed to do that in the past as well.

> But that seems like a loss of useful functionality compared to the
> other collectors, assuming it's actually possible to do.  Which I'm
> guessing it isn't?

Possible is a relative thing. ;)

The ZGC heap isn’t parsable, and we don’t have block offset tables. That makes it pretty hard to find the surrounding object of an arbitrary address in the heap. When we have to walk the heap to find things, we use tracing instead of iterative scanning due to the lack of parsability. It might be possible to sometimes find the base pointer using tracing, assuming you can find it in the object graph. But then again, we just crashed when we get here, so the tracing might crash too, and interesting values might be hidden in garbage.

So while it would be nice to improve heap location printing in crash reports, the solution is unfortunately not clear yet.

> ------------------------------------------------------------------------------
> 
> This looks good, except for the now extraneous virtual specifiers.  I
> don't need a new webrev for that.

Thanks for the review!

/Erik