RFR: tune native call site check

yangyongyong yangyongyong at huawei.com
Sat Sep 10 07:14:45 UTC 2016


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