RFR: tune native call site check
Anton Kozlov
akozlov at azul.com
Wed Nov 2 14:23:10 UTC 2016
Thanks for feedback.
I totally forgot it's not commited yet. Could someone fix that?
Anton
________________________________________
From: yangyongyong [yangyongyong at huawei.com]
Sent: Monday, October 17, 2016 9:52 AM
To: Anton Kozlov; aarch32-port-dev at openjdk.java.net
Subject: RE: RFR: tune native call site check
Hi, Anton, very sorry for late reply.
The second version seems ok to me.
Best regards.
-----Original Message-----
From: Anton Kozlov [mailto:akozlov at azul.com]
Sent: Monday, September 12, 2016 11:50 PM
To: yangyongyong; aarch32-port-dev at openjdk.java.net
Subject: Re: RFR: tune native call site check
Oh, I see. Agree, is_call_before is incorrect same way as nativeCall_before prior 1st patch; and if no single instruction will be found on offset (-4), it will try to find 3-instruction sequence there, possibly going beyound CodeCache boundary.
Thanks! I suggest second version of patch. Yang, could you check it still works for you?
Anton
***** patch start *****
diff --git a/src/cpu/aarch32/vm/nativeInst_aarch32.cpp b/src/cpu/aarch32/vm/nativeInst_aarch32.cpp
--- a/src/cpu/aarch32/vm/nativeInst_aarch32.cpp
+++ b/src/cpu/aarch32/vm/nativeInst_aarch32.cpp
@@ -104,8 +104,16 @@
}
bool NativeCall::is_call_before(address return_address) {
- return is_at(return_address - NativeImmCall::instruction_size) ||
- is_at(return_address - NativeCall::instruction_size);
+ if (NativeTrampolineCall::is_at(return_address -
+NativeCall::instruction_size)) {
+ return true;
+ } else if (NativeMovConstReg::is_at(return_address -
+NativeCall::instruction_size)) {
+ NativeMovConstReg *nm = NativeMovConstReg::from(return_address -
+NativeCall::instruction_size);
+ address next_instr = nm->next_instruction_address();
+ return NativeRegCall::is_at(next_instr) &&
+NativeRegCall::from(next_instr)->destination() == nm->destination();
+ } else if (NativeImmCall::is_at(return_address -
+NativeBranchType::instruction_size)) {
+ return true;
+ }
+ return false;
}
address NativeCall::next_instruction_address() const { diff --git a/src/cpu/aarch32/vm/nativeInst_aarch32.hpp b/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
--- a/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
+++ b/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
@@ -303,20 +303,6 @@
return NativeCall::from(address);
}
-inline NativeCall* nativeCall_before(address return_address) {
- address call_addr = NULL;
- if (NativeCall::is_at(return_address - NativeBranchType::instruction_size)) {
- call_addr = return_address - NativeBranchType::instruction_size;
- } else if (NativeCall::is_at(return_address - NativeCall::instruction_size)) {
- call_addr = return_address - NativeCall::instruction_size;
- } else {
- ShouldNotReachHere();
- }
-
- return NativeCall::from(call_addr);
-}
-
-
// An interface for accessing/manipulating native moves of the form:
// mov[b/w/l/q] [reg + offset], reg (instruction_code_reg2mem)
// mov[b/w/l/q] reg, [reg+offset] (instruction_code_mem2reg @@ -505,4 +491,27 @@
inline bool NativeInstruction::is_imm_jump() const { return NativeImmJump::is_at(addr()); }
inline bool NativeInstruction::is_reg_jump() const { return NativeRegJump::is_at(addr()); }
+inline NativeCall* nativeCall_before(address return_address) {
+ address call_addr = NULL;
+ if (NativeTrampolineCall::is_at(return_address -
+NativeCall::instruction_size)) {
+ call_addr = return_address - NativeCall::instruction_size;
+ } else if (NativeMovConstReg::is_at(return_address -
+NativeCall::instruction_size)) {
+ NativeMovConstReg *nm = NativeMovConstReg::from(return_address -
+NativeCall::instruction_size);
+ address next_instr = nm->next_instruction_address();
+ if (NativeRegCall::is_at(next_instr) &&
+ NativeRegCall::from(next_instr)->destination() ==
+nm->destination()) {
+ call_addr = return_address - NativeCall::instruction_size;
+ }
+ }
+ if (!call_addr) {
+ if (NativeImmCall::is_at(return_address -
+NativeBranchType::instruction_size)) {
+ call_addr = return_address - NativeBranchType::instruction_size;
+ } else {
+ ShouldNotReachHere();
+ }
+ }
+
+ return NativeCall::from(call_addr);
+}
+
#endif // CPU_AARCH32_VM_NATIVEINST_AARCH32_HPP
***** patch end ******
On Mon, 2016-09-12 at 13:37 +0000, yangyongyong wrote:
> Yes, please. Yang is my surname and of course you can call me that.
>
> I think NativeCall* nativeCall_before() is ok now.
> However it seems that NativeCall::is_call_before() would incur a
> similar crash as you mentioned in an earlier email.
>
> Regards.
>
> -----Original Message-----
> From: Anton Kozlov [mailto:akozlov at azul.com]
> Sent: Monday, September 12, 2016 8:34 PM
> To: yangyongyong; aarch32-port-dev at openjdk.java.net<mailto:aarch32-port-dev at openjdk.java.net>
> Subject: Re: RFR: tune native call site check
>
> Hi, Yang (sorry, is it correct?),
>
> I can't remember well, it reproduced on some jtreg test. But it also
> should be reproduced in release mode by specifying small values for
> InitialCodeCacheSize and CodeCacheExpansionSize JVM options, like 4K.
>
> We've prepared another patch after your email with suggestions
> implemented (Andrey Petushkov), in attach and below.
>
> I think that nativeCall_before should be enough. The problem is that
> nativeCall_before is not sure where instruction sequence is started,
> but it calls NativeCall::is_at() anyway. NativeCall::is_at is supposed
> to be called on start of instruction sequence; with this precondition,
> I see no possibilities for is_at to misdecode target address. Please,
> correct me if I'm wrong.
>
> Thanks,
> Anton
>
> ***** patch begin ******
> diff --git a/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> b/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> --- a/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> +++ b/src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> @@ -303,20 +303,6 @@
> return NativeCall::from(address);
> }
>
> -inline NativeCall* nativeCall_before(address return_address) {
> - address call_addr = NULL;
> - if (NativeCall::is_at(return_address -
> NativeBranchType::instruction_size))
> {
> - call_addr = return_address - NativeBranchType::instruction_size;
> - } else if (NativeCall::is_at(return_address -
> NativeCall::instruction_size)) {
> - call_addr = return_address - NativeCall::instruction_size;
> - } else {
> - ShouldNotReachHere();
> - }
> -
> - return NativeCall::from(call_addr); -}
> -
> -
> // An interface for accessing/manipulating native moves of the form:
> // mov[b/w/l/q] [reg + offset], reg (instruction_code_reg2mem)
> // mov[b/w/l/q] reg, [reg+offset] (instruction_code_mem2reg
> @@
> -505,4 +491,27 @@
> inline bool NativeInstruction::is_imm_jump() const { return
> NativeImmJump::is_at(addr()); }
> inline bool NativeInstruction::is_reg_jump() const { return
> NativeRegJump::is_at(addr()); }
>
> +inline NativeCall* nativeCall_before(address return_address) {
> + address call_addr = NULL;
> + if (NativeTrampolineCall::is_at(return_address -
> +NativeCall::instruction_size)) {
> + call_addr = return_address - NativeCall::instruction_size;
> + } else if (NativeMovConstReg::is_at(return_address -
> +NativeCall::instruction_size)) {
> + NativeMovConstReg *nm = NativeMovConstReg::from(return_address -
> +NativeCall::instruction_size);
> + address next_instr = nm->next_instruction_address();
> + if (NativeRegCall::is_at(next_instr) &&
> + NativeRegCall::from(next_instr)->destination() ==
> +nm->destination()) {
> + call_addr = return_address - NativeCall::instruction_size;
> + }
> + }
> + if (!call_addr) {
> + if (NativeImmCall::is_at(return_address -
> +NativeBranchType::instruction_size)) {
> + call_addr = return_address -
> +NativeBranchType::instruction_size;
> + } else {
> + ShouldNotReachHere();
> + }
> + }
> +
> + return NativeCall::from(call_addr); }
> +
> #endif // CPU_AARCH32_VM_NATIVEINST_AARCH32_HPP
>
> ***** patch end ******
>
> On Sat, 2016-09-10 at 07:14 +0000, yangyongyong wrote:
> >
> > Thank you, Anton,
> >
> > My patch *improves* the check logic a little but it does not handle
> > the situation you mentioned.
> > How is it reproduced? Do you have a patch at hand already?
> >
> > Your suggestion seems more rational. The check logic should better
> > not mix up the situations of offset -4 and -12.
> >
> > Meanwhile, please note that NativeCall::is_at() is invoked somewhere
> > else, so perhaps some minor modification of nativeCall_before() is
> > not enough.
> >
> > -----Original Message-----
> > From: Anton Kozlov [mailto:akozlov at azul.com]
> > Sent: Friday, September 09, 2016 8:35 PM
> > To: yangyongyong; aarch32-port-dev at openjdk.java.net<mailto:aarch32-port-dev at openjdk.java.net>
> > Subject: Re: RFR: tune native call site check
> >
> > Hi!
> >
> > Thanks for finding this! You're right, this is a bug.
> >
> > However, if NativeCall::is_at will be fixed like suggested, then the
> > long instruction sequence will be checked before short one. And now
> > consider situation, when this call is the last instruction of last
> > method in CodeCache, unluckily placed at end of page. "Long" check
> > will try to read next page of CodeCache (forbidden for r/w/x) and
> > will trigger segfault, which is not expected and not handled by JVM.
> > Actually, we've seen that =)
> >
> > There is a suggestion to fix only nativeCall_before, by explicitly
> > checking NativeImmCall, NativeTrampolineCall, NativeRegCall there
> > with appropriate offsets. Actually, it looks more correct instead of
> > trying to find any of these
> > 3 Native* at offset 4 and then at offset 12.
> >
> > Thanks,
> > Anton
> >
> > On Fri, 2016-09-09 at 10:57 +0000, yangyongyong wrote:
> > >
> > >
> > > Roughly there are 3 kinds of native call site checked by
> > > nativeCall_before():
> > >
> > > 1. Native-Trampoline-Call takes the form of:
> > >
> > > add lr, pc, #4
> > > ldr pc, [pc, -4]
> > > 0x???????? # call_destination
> > > #return_address:
> > > Some_insn
> > >
> > >
> > > 2. Native-Imm-Call
> > >
> > > bl imm # call_destination
> > > #return_address:
> > > Some_insn
> > >
> > >
> > > 3. Native-Reg-Call
> > >
> > >
> > > movw regx, #call_destination
> > >
> > > movt regx, #call_destination
> > > bl regx # call destination
> > > #return_address:
> > > Some_insn
> > >
> > > Current check logic incurs a problem if the encoding of the call
> > > destination of type 1 can by any chance be disassembled as a “bl imm”
> > > instruction.
> > > And thus the call address is calculated improperly and then the
> > > relocation info will not be found, which causes assert failure in
> > > CompiledIC::CompiledIC(nmethod* nm, NativeCall* call):
> > > assert(ret == true, "relocInfo must exist at this address");
> > >
> > > This failure can be reproduced simply by “java -Xcomp
> > > -XX:ReservedCodeCacheSize=xx”. Please tune xx and make sure some
> > > of the JITed code is placed at address 0x?B??????.
> > > The attached patch improves the check logic:
> > > ----------------- patch begins ---------------- diff -r
> > > 80b7b526cffb src/cpu/aarch32/vm/nativeInst_aarch32.cpp
> > > --- a/src/cpu/aarch32/vm/nativeInst_aarch32.cpp Sun Sep 04
> > > 20:52:43
> > > 2016 +0100
> > > +++ b/src/cpu/aarch32/vm/nativeInst_aarch32.cpp Fri Sep 09
> > > +++ 17:38:34 2016
> > > +0800
> > > @@ -132,14 +132,14 @@
> > > }
> > > bool NativeCall::is_at(address addr) {
> > > - if (NativeImmCall::is_at(addr)) {
> > > + if (NativeTrampolineCall::is_at(addr)) {
> > > return true;
> > > } else if (NativeMovConstReg::is_at(addr)) {
> > > NativeMovConstReg *nm = NativeMovConstReg::from(addr);
> > > address next_instr = nm->next_instruction_address();
> > > return NativeRegCall::is_at(next_instr) &&
> > > NativeRegCall::from(next_instr)->destination() ==
> > > nm->destination();
> > > - } else if (NativeTrampolineCall::is_at(addr)) {
> > > + } else if (NativeImmCall::is_at(addr)) {
> > > return true;
> > > }
> > > return false;
> > > diff -r 80b7b526cffb src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> > > --- a/src/cpu/aarch32/vm/nativeInst_aarch32.hpp Sun Sep 04
> > > 20:52:43
> > > 2016 +0100
> > > +++ b/src/cpu/aarch32/vm/nativeInst_aarch32.hpp Fri Sep 09
> > > +++ 17:38:34 2016
> > > +0800
> > > @@ -306,10 +306,10 @@
> > > inline NativeCall* nativeCall_before(address return_address) {
> > > address call_addr = NULL;
> > > - if (NativeCall::is_at(return_address -
> > > NativeBranchType::instruction_size))
> > > {
> > > + if (NativeCall::is_at(return_address -
> > > +NativeCall::instruction_size)) {
> > > + call_addr = return_address - NativeCall::instruction_size;
> > > + } else if (NativeCall::is_at(return_address -
> > > NativeBranchType::instruction_size)) {
> > > call_addr = return_address -
> > > NativeBranchType::instruction_size;
> > > - } else if (NativeCall::is_at(return_address -
> > > NativeCall::instruction_size)) {
> > > - call_addr = return_address - NativeCall::instruction_size;
> > > } else {
> > > ShouldNotReachHere();
> > > }
> > > ----------------- patch ends ----------------
> > >
More information about the aarch32-port-dev
mailing list