[aarch64-port-dev ] RFR: 8209413: AArch64: NPE in clhsdb jstack command

[Nick Gasson (Arm Technology China)]

Hi Andrew,

Please take a look at this updated patch:

http://cr.openjdk.java.net/~ngasson/8209413/webrev.03/

It contains your suggested change below, as well as an extra check in 
the AARCH64Frame two-argument constructor that the PC from SP[-1] is 
valid. Also changed SharedRuntime::generate_native_wrapper and 
TemplateInterpreterGenerator::generate_native_entry to save an accurate 
return PC in the frame anchor.

There's a FIXME in set_last_Java_frame like this:

   } else {
     // FIXME: This is almost never correct.  We should delete all
     // cases of set_last_Java_frame with last_java_pc=NULL and use the
     // correct return address instead.
     adr(scratch, pc());
   }

Should we do this now? There's only two places I can see: 
StubGenerator::generate_throw_exception and the set_last_Java_frame 
overload that takes a Label.

Thanks,
Nick

On 12/02/2019 23:32, Andrew Haley wrote:
> What's the status of this? Are you still looking at it?
> 
> On 2/7/19 3:39 PM, Andrew Haley wrote:
>> On 2/7/19 2:36 PM, Nick Gasson (Arm Technology China) wrote:
>>>
>>> Yeah I tried this method too (see the end of my first email). It
>>> works fine except the PC is off by a few instructions. That can be
>>> fixed quite easily though if required, and it always points into the
>>> right method.
>>
>> We should fix those few cases.
>>
>>> I ended up doing the stack scanning thing because as far as I can
>>> tell that constructor is only used when we need to find the return
>>> PC from a native frame
>>
>> I don't think that ever happens. In the code I modified we know that
>> we have a good PC in the frame anchor. In the cases where we have a
>> native frame on the stack and nothing in the frame anchor we give
>> up. At least, I can see no counter-cases.
>>
>>> (I might have been wrong about this), so I couldn't see how it would
>>> ever get the right value. If we check vm.isJavaPCDbg() before we use
>>> the SP[-1] value that will make the remaining callers a bit safer
>>> (and set this.pc to null otherwise).
>>
>> That would be safe, for sure.
>>
> 
>