RFR: tune native call site check

[Anton Kozlov]

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 ----------------
>