RFR: tune native call site check
yangyongyong
yangyongyong at huawei.com
Mon Sep 12 13:37:48 UTC 2016
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
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
> 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