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

Doerr, Martin martin.doerr at sap.com
Tue Oct 2 12:58:18 UTC 2018


Hi Coleen,

I've created a rebased webrev after 8209645:
http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.03/

It contains minor improvements (added missing include and type cast).
I think I should reuse the code from VirtualSpaceList::contains. Changed.

I have tested most cases by inserting e.g.
"if (UseNewCode) __ code()->insts()->emit_int8((unsigned char) 0x37 /* aaa */);"
at different places. I got the expected output.

Please take a look.

Thanks and best regards,
Martin


-----Original Message-----
From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net> On Behalf Of Doerr, Martin
Sent: Freitag, 28. September 2018 17:27
To: coleen.phillimore at oracle.com; Thomas Stüfe <thomas.stuefe at gmail.com>
Cc: hotspot-runtime-dev at openjdk.java.net
Subject: [CAUTION] RE: RFR(M): 8210754: print_location is not reliable enough (printing register info)

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



More information about the hotspot-runtime-dev mailing list