RFR: tune native call site check

Anton Kozlov akozlov at azul.com
Mon Sep 12 12:33:53 UTC 2016


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 ----------------
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nativeInstr_before.patch
Type: text/x-patch
Size: 2197 bytes
Desc: nativeInstr_before.patch
URL: <http://mail.openjdk.java.net/pipermail/aarch32-port-dev/attachments/20160912/a49a0d46/nativeInstr_before-0001.patch>


More information about the aarch32-port-dev mailing list