[aarch64-port-dev ] RFR: 8148783: aarch64: SEGV running SpecJBB2013

[Andrew Haley]

Hi,

On 02/01/2016 08:33 PM, Edward Nevill wrote:

> JIRA Issue: https://bugs.openjdk.java.net/browse/JDK-8148783
> 
> The bug is explained in some detail in the JIRA issue.
> 
> The problem is that the sign is not preserved in the following code
> from adrp(...)
> 
>     long offset = dest_page - pc_page;
>     offset = (offset & ((1<<20)-1)) << 12;
> 
> This generally works because the following movk overwrites bits 32..47
> 
> However on larger memory systems of 256 Gb it could happen that the
> PC address was
> 
> 0x0000ffffXXXXXXXX
> 
> in which case the falsely positive offset could wrap to
> 
> 0x00010000XXXXXXXX
> 
> Bit 48 does not get overwritten by the following movk, hence forming
> an invalid address.
> 
> The solution is to use int32_t for offset instead of long, so it
> gets sign extended correctly when added to the pc().

I can't accept that patch because the overflowing assignment from long
to int32_t is undefined behaviour.  It is also very obscure code.

Can you test the patch I've appended instead?  It tiptoes around the UB
and should be OK.

Thanks,

Andrew.


diff --git a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
--- a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
+++ b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
@@ -3980,6 +3980,14 @@
   return inst_mark();
 }

+int64_t MacroAssembler::truncate_signed_bitfield(int64_t n, int width) {
+  // Left shifts of a signed integer are UB in Standard C++ but
+  // well-defined in GNU C++.
+  n <<= 64 - width;
+  n >>= 64 - width;
+  return n;
+}
+
 void MacroAssembler::adrp(Register reg1, const Address &dest, unsigned long &byte_offset) {
   relocInfo::relocType rtype = dest.rspec().reloc()->type();
   unsigned long low_page = (unsigned long)CodeCache::low_bound() >> 12;
@@ -3999,8 +4007,18 @@
     _adrp(reg1, dest.target());
   } else {
     unsigned long pc_page = (unsigned long)pc() >> 12;
-    long offset = dest_page - pc_page;
-    offset = (offset & ((1<<20)-1)) << 12;
+    unsigned long page_offset = dest_page - pc_page;
+
+    // The signed offset (in 4k pages) from PC to dest page.  We use a
+    // reference in order to avoid UB when converting from unsigned to
+    // signed.
+    long offset = reinterpret_cast<long&>(page_offset);
+
+    // The signed offset (in bytes) from the PC to the destination
+    // page.  We only want the 32 LSBs of the offset because the range
+    // of ADRP is +-2G, i.e. 32 bits.
+    offset = truncate_signed_bitfield(offset << 12, 32);
+
     _adrp(reg1, pc()+offset);
     movk(reg1, (unsigned long)dest.target() >> 32, 32);
   }
diff --git a/src/cpu/aarch64/vm/macroAssembler_aarch64.hpp b/src/cpu/aarch64/vm/macroAssembler_aarch64.hpp
--- a/src/cpu/aarch64/vm/macroAssembler_aarch64.hpp
+++ b/src/cpu/aarch64/vm/macroAssembler_aarch64.hpp
@@ -85,9 +85,10 @@

   void call_VM_helper(Register oop_result, address entry_point, int number_of_arguments, bool check_exceptions = true);

-  // Maximum size of class area in Metaspace when compressed
   uint64_t use_XOR_for_compressed_class_base;

+  int64_t truncate_signed_bitfield(int64_t n, int width);
+
  public:
   MacroAssembler(CodeBuffer* code) : Assembler(code) {
     use_XOR_for_compressed_class_base