RFR(M): 8210754: print_location is not reliable enough (printing register info)

[Doerr, Martin]

Hi Coleen,

thank you for taking time to review my proposal in detail. I have fixed a few things in place and uploaded a new webrev which moves functions into their classes:
http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.02


> Hi, I am sorry for the delay getting to this.
No problem. I also needed some time to get my build working again. I had to build a gcc 7.3.


> http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.01/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
> Can you assert is_error_reported() and add a comment that this is only to be called during error reporting?  I share David's concern about walking CLDG graph without a lock but I think the only place where we unlink items is limited and the timing window where you'll get into trouble is very limited.
Done (in place).


>  105 bool vmErrorHelper::is_valid_symbol(Symbol* s) {
> This is actually wrong. The Symbol is not in metaspace.  I agree with Thomas's suggestion of testing validity and moving to symbol.hpp/cpp.
Right, I had broken it in webrev.01. Fixed in place.


>  122   return is_readable_range(bytes, bytes + len);
>
> I kind of wonder if this is going too far.   Why not just ask os::is_readable_pointer?
Hmm... Should usually be sufficient. But is there a disadvantage of using the range version? Performance shouldn't be a concern when iterating pagewise.


> +    } else if (Method::has_method_vptr((const void*)addr)) {
> This should be changed to is_valid_method().  is_valid_klass() should be written similarly.
Done (in place).


> Isn't if (Metaspace::contains_non_shared(m))  a sufficient check for the Klass pointer, ie, like the code in Method::is_valid_method()?
Can't we have Klasses in shared space for CDS? I guess this would break it for CDS.
I think checking if at least the common Klass part is completely contained in one metaspace region is better. Do you agree?


>  141 bool vmErrorHelper::is_valid_oop(oop obj) {
> I expected is_valid_oop to be the top level verification and to load the klass pointer and ask is_valid_klass.  verify_oop does that.  Maybe the CLDG iteration is warranted to see if the klass is still loaded.   I think using just using Universe::heap()->is_in_reserved() should be sufficient and you should avoid using block_start() and other things that might be broken with a crash.
Removing the block_start part would degrade the functionality. The current implementation tries to find the start address of a Java object when addr points into it.


> I've always wanted the code blob printing code to be in codeBlob.cpp.
I agree. Done.


> I appreciate the hardening of error reporting.  It's long overdue!
Thanks.


I'll try to find some time for testing various cases next week.

Best regards,
Martin