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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Oct 2 23:19:58 UTC 2018


http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.03/src/hotspot/share/oops/klass.cpp.frames.html

In is_valid_klass() can you check for the vtable before calling 
class_loader_data() or getting the Symbol?  There's also a 'valid' bit 
on a lot of the metadata that you might want to check in debug mode.

http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.03/src/hotspot/share/oops/symbol.cpp.frames.html

The Symbols might be allocated in metaspace for CDS, or considered to be 
allocated there by the metaspace range matching code, so this might not 
be quite right to return false if they're in metaspace. I think I've 
contradicted my earlier comments (sorry about that).  I think you can't 
have that conditional.

http://cr.openjdk.java.net/~mdoerr/8210754_print_location/webrev.03/src/hotspot/share/runtime/os.cpp.frames.html

1121   // Check if in metaspace and print types that have vptrs (only method now)

1123 if (Klass::is_valid((Klass*)addr)) {


This comment meant that we only print method but Klass has a vptr too.

I think this looks like a good improvement to error handling.  Thank you 
for taking up my suggestions and merging with the classLoaderDataGraph 
change.

Coleen


On 10/2/18 8:58 AM, Doerr, Martin wrote:
> 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