[8u] RFR: Delete incorrect NativeTrampolineCall::instruction_size

Anton Kozlov akozlov at azul.com
Thu Dec 1 15:42:56 UTC 2016


Hi!

Could you review a patch, that deletes NativeTrampolineCall::instruction_size?

It's redundant and never used, because shared code in some sitations assumes
NativeTrampolineCall to be same size as NativeCall. However, they are different
sizes (3 and 5 words on ARMv6), so first one is padded with nops up to second. 

Presence of NativeTrampolineCall::instruction_size encourage it's use and thus 
introducing bugs. Example is nativeCall_before:

inline NativeCall* nativeCall_before(address return_address) {
  if (NativeTrampolineCall::is_at(return_address - NativeCall::instruction_size)) {
    ...
  } 
  ...
}

It will be incorrect if `NativeTrampolineCall::instruction_size` used here, but
looking good.

NativeCall::instruction_size is not C++ constant and computed at VM start after
determing weither it's running on ARMv6 or ARMv7. 
Making NativeTrampolineCall::instruction_size to be always equal 
NativeCall::instruction_size implies making one not constant too and adding 
relevant initializing code.

I suggest just to remove NativeTrampolineCall::instruction_size and use 
NativeCall::instruction_size instead.

Thanks,
Anton

diff --git a/src/cpu/aarch32/vm/macroAssembler_aarch32.cpp b/src/cpu/aarch32/vm/macroAssembler_aarch32.cpp
--- a/src/cpu/aarch32/vm/macroAssembler_aarch32.cpp
+++ b/src/cpu/aarch32/vm/macroAssembler_aarch32.cpp
@@ -625,12 +625,15 @@
     // Have make trampline such way: destination address should be raw 4 byte value,
     // so it's patching could be done atomically.
     relocate(entry.rspec());
+    address start = pc();
     add(lr, r15_pc, NativeCall::instruction_size - 2 * NativeInstruction::arm_insn_sz);
     ldr(r15_pc, Address(r15_pc, 4));
     emit_int32((uintptr_t) entry.target());
     // possibly pad the call to the NativeCall size to make patching happy
-    for (int i = NativeCall::instruction_size; i > 3 * NativeInstruction::arm_insn_sz; i -= NativeInstruction::arm_insn_sz)
+    while (pc() - start < NativeCall::instruction_size) {
       nop();
+    }
+    assert(pc() - start == NativeCall::instruction_size, "fix NativeTrampolineCall::instruction_size!");
   } else {
     bl(entry);
   }
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
@@ -177,7 +177,7 @@
 }
 
 bool NativeTrampolineCall::is_at(address addr) {
-  return (as_uint(addr    ) & ~0xffu) == 0xe28fe000  // add     lr, pc, #disp
+  return (as_uint(addr    ) & ~0xffu) == 0xe28fe000 // add     lr, pc, #disp
        && as_uint(addr + 4)          == 0xe51ff004; // ldr     pc, [pc, -4]
 }
 
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
@@ -211,11 +211,9 @@
   return NativeMovConstReg::from(address);
 }
 
-class NativeTrampolineCall: public NativeBranchType {
+class NativeTrampolineCall: public NativeInstruction {
  public:
-  enum {
-    instruction_size = 3 * arm_insn_sz
-  };
+  // NativeTrampolineCall size is always equal to NativeCall::instruction_size
   address destination() const;
   void set_destination(address dest);
   void set_destination_mt_safe(address dest, bool assert_lock = true);
@@ -252,7 +250,6 @@
 
   static int instruction_size;
 #ifdef ASSERT
-  StaticAssert<(int) NativeTrampolineCall::instruction_size <= (int) max_instruction_size> dummy1;
   StaticAssert<NativeMovConstReg::movw_movt_pair_sz
       + NativeRegCall::instruction_size <= (int) max_instruction_size> dummy2;
   StaticAssert<NativeMovConstReg::mov_n_three_orr_sz


More information about the aarch32-port-dev mailing list