RFR(S): 8196401: PPC64+s390: get_frame_at_stack_banging_point uses wrong PC

[Lindenmaier, Goetz]

Hi Martin,

the change looks good. 

I wondered why this never matters in the test, but that's probably 
because the method annotated itself stack overflows before doing
critical stuff.

Best regards,
  Goetz.


> -----Original Message-----
> From: Doerr, Martin
> Sent: Mittwoch, 31. Januar 2018 10:15
> To: Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>; Schmidt, Lutz <lutz.schmidt at sap.com>
> Subject: RE: RFR(S): 8196401: PPC64+s390:
> get_frame_at_stack_banging_point uses wrong PC
> 
> Hi Thomas,
> 
> 
> 
> thanks for clarification. Yes, I had thought in terms of storage duration. I
> agree.
> 
> Thanks again for reviewing.
> 
> 
> 
> Best regards,
> 
> Martin
> 
> 
> 
> 
> 
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Mittwoch, 31. Januar 2018 08:59
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>; Schmidt, Lutz <lutz.schmidt at sap.com>
> Subject: Re: RFR(S): 8196401: PPC64+s390:
> get_frame_at_stack_banging_point uses wrong PC
> 
> 
> 
> Hi Martin,
> 
> 
> 
> On Tue, Jan 30, 2018 at 4:45 PM, Doerr, Martin <martin.doerr at sap.com
> <mailto:martin.doerr at sap.com> > wrote:
> 
> 	Hi Thomas,
> 
> 
> 
> 	the storage class „static” is used by default, so the 2 versions are
> equivalent.
> 
> 
> 
> We misunderstood each other. Default storage class for global functions is
> "external". static is needed to avoid symbol export.
> 
> 
> 
> 	But I’m ok with using the keyword to make it explicitly static.
> 
> 
> 
> 	I also need to update copyright information.
> 
> 
> 
> 	New webrev:
> 
> 	http://cr.openjdk.java.net/~mdoerr/8196401_ppc64_s390_stack_ba
> ng_pc/webrev.01/
> 
> 
> 
> 
> 
> Looks fine to me. Thanks for fixing.
> 
> 
> 
> ..Thomas
> 
> 
> 
> 	Best regards,
> 
> 	Martin
> 
> 
> 
> 
> 
> 	From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com> ]
> 	Sent: Dienstag, 30. Januar 2018 16:04
> 
> 
> 	To: Doerr, Martin <martin.doerr at sap.com
> <mailto:martin.doerr at sap.com> >
> 	Cc: hotspot-compiler-dev at openjdk.java.net <mailto:hotspot-
> compiler-dev at openjdk.java.net> ; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com> >;
> Schmidt, Lutz <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com> >
> 	Subject: Re: RFR(S): 8196401: PPC64+s390:
> get_frame_at_stack_banging_point uses wrong PC
> 
> 
> 
> 	Hi Martin,
> 
> 
> 
> 	On Tue, Jan 30, 2018 at 3:57 PM, Doerr, Martin
> <martin.doerr at sap.com <mailto:martin.doerr at sap.com> > wrote:
> 
> 		Hi Thomas,
> 
> 
> 
> 		thanks for reviewing.
> 
> 
> 
> 		> Could you please move ucontext_get_lr() to the Aix resp
> Linux namespaces, like the other ucontext_xxx functions, or at least make
> them static file scope ?
> 
> 
> 
> 		This could be done on AIX, but I don’t see how it could be
> done on Linux without touching shared linux code. I don’t want to change
> non-ppc/s390 files because the PC retrieval is specific to these platforms.
> 
> 
> 
> 	I see what you mean. I am fine with keeping it file static.
> 
> 
> 
> 		The new functions are already static file scope. Or did you
> mean something else?
> 
> 
> 
> 
> 	I mean:
> 
> 	-address ucontext_get_lr(const ucontext_t * uc) {
> 	+static address ucontext_get_lr(const ucontext_t * uc) {
> 
> 
> 
> 	Best Regards, Thomas
> 
> 
> 
> 		Best regards,
> 
> 		Martin
> 
> 
> 
> 
> 
> 		From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com> ]
> 		Sent: Dienstag, 30. Januar 2018 15:42
> 		To: Doerr, Martin <martin.doerr at sap.com
> <mailto:martin.doerr at sap.com> >
> 		Cc: hotspot-compiler-dev at openjdk.java.net
> <mailto:hotspot-compiler-dev at openjdk.java.net> ; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com> >;
> Schmidt, Lutz <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com> >
> 		Subject: Re: RFR(S): 8196401: PPC64+s390:
> get_frame_at_stack_banging_point uses wrong PC
> 
> 
> 
> 		Hi Martin,
> 
> 
> 
> 		Could you please move ucontext_get_lr() to the Aix resp
> Linux namespaces, like the other ucontext_xxx functions, or at least make
> them static file scope ?
> 
> 
> 
> 		Otherwise, this looks good.
> 
> 
> 
> 		Regards, Thomas
> 
> 
> 
> 		On Tue, Jan 30, 2018 at 2:47 PM, Doerr, Martin
> <martin.doerr at sap.com <mailto:martin.doerr at sap.com> > wrote:
> 
> 			Hi,
> 
> 
> 
> 			we have found a bug in
> get_frame_at_stack_banging_point on PPC64 and s390 by reading code.
> 
> 
> 
> 			C1 and C2 JIT compilers generate stack banging code
> before the code for saving the PC on PPC64 and s390.
> 
> 			Hence, the PC needs to be picked from the signal
> context (LR on PPC64 and R14 on s390) to create a correct frame object.
> 
> 
> 
> 			Currently, the top frame can not be checked for
> reserved stack annotation because the frame doesn't get a PC which points
> into the nmethod.
> 
> 
> 
> 			Proposed fix:
> 
> 
> 	http://cr.openjdk.java.net/~mdoerr/8196401_ppc64_s390_stack_ba
> ng_pc/webrev.00/
> 
> 
> 
> 			Please review.
> 
> 
> 
> 			Best regards,
> 
> 			Martin
> 
> 
> 
> 
> 
> 
> 
>