[pypy-commit] pypy faster-rstruct-2: delegate the alignment check to Buffer.typed_{read, write}, to take in account also the SubBuffer's offset. Moreover, reuse the logic in rawstorage to determine whether it is fine to do an unaligned access, depending on the CPU

[antocuni]

Author: Antonio Cuni <anto.cuni at gmail.com>
Branch: faster-rstruct-2
Changeset: r91335:7d8f1bbfc0b1
Date: 2017-05-18 17:12 +0200
http://bitbucket.org/pypy/pypy/changeset/7d8f1bbfc0b1/

Log:	delegate the alignment check to Buffer.typed_{read,write}, to take
	in account also the SubBuffer's offset. Moreover, reuse the logic in
	rawstorage to determine whether it is fine to do an unaligned
	access, depending on the CPU

diff --git a/rpython/rlib/buffer.py b/rpython/rlib/buffer.py
--- a/rpython/rlib/buffer.py
+++ b/rpython/rlib/buffer.py
@@ -13,6 +13,21 @@
                               ll_for_resizable_list)
 from rpython.rlib.signature import signature
 from rpython.rlib import types
+from rpython.rlib import rawstorage
+
+ALLOW_UNALIGNED_ACCESS = rawstorage.misaligned_is_fine
+
+ at specialize.ll()
+def is_alignment_correct(TYPE, index):
+    if ALLOW_UNALIGNED_ACCESS:
+        return True
+    try:
+        rawstorage._check_alignment(TYPE, index)
+    except rawstorage.AlignmentError:
+        return False
+    else:
+        return True
+
 
 class CannotRead(Exception):
     """
@@ -117,6 +132,8 @@
         """
         Read the value of type TP starting at byte_offset. No bounds checks
         """
+        if not is_alignment_correct(TP, byte_offset):
+            raise CannotRead
         ptr = self.get_raw_address()
         return llop.raw_load(TP, ptr, byte_offset)
 
@@ -125,7 +142,7 @@
         """
         Write the value of type TP at byte_offset. No bounds checks
         """
-        if self.readonly:
+        if self.readonly or not is_alignment_correct(TP, byte_offset):
             raise CannotWrite
         ptr = self.get_raw_address()
         value = lltype.cast_primitive(TP, value)
@@ -158,6 +175,8 @@
 
         @specialize.ll_and_arg(1)
         def typed_read(self, TP, byte_offset):
+            if not is_alignment_correct(TP, byte_offset):
+                raise CannotRead
             lldata = self._get_gc_data()
             byte_offset += self._get_gc_data_extra_offset()
             return llop.gc_load_indexed(TP, lldata, byte_offset,
@@ -165,7 +184,7 @@
 
         @specialize.ll_and_arg(1)
         def typed_write(self, TP, byte_offset, value):
-            if self.readonly:
+            if self.readonly or not is_alignment_correct(TP, byte_offset):
                 raise CannotWrite
             lldata = self._get_gc_data()
             byte_offset += self._get_gc_data_extra_offset()
diff --git a/rpython/rlib/rstruct/standardfmttable.py b/rpython/rlib/rstruct/standardfmttable.py
--- a/rpython/rlib/rstruct/standardfmttable.py
+++ b/rpython/rlib/rstruct/standardfmttable.py
@@ -33,17 +33,17 @@
     @specialize.argtype(0)
     def do_pack_fastpath(fmtiter, value):
         size = rffi.sizeof(TYPE)
-        pos = fmtiter.pos
         if (not USE_FASTPATH or
             fmtiter.bigendian != native_is_bigendian or
-            not native_is_ieee754 or
-            pos % size != 0):
+            not native_is_ieee754):
             raise CannotWrite
         #
-        if not ALLOW_FASTPATH:
-            raise ValueError("fastpath not allowed :(")
         # typed_write() might raise CannotWrite
         fmtiter.wbuf.typed_write(TYPE, fmtiter.pos, value)
+        if not ALLOW_FASTPATH:
+            # if we are here it means that typed_write did not raise, and thus
+            # the fast path was actually taken
+            raise ValueError("fastpath not allowed :(")
         fmtiter.advance(size)
     #
     @specialize.argtype(0)
@@ -203,12 +203,7 @@
     def do_unpack_fastpath(fmtiter):
         size = rffi.sizeof(TYPE)
         buf, pos = fmtiter.get_buffer_and_pos()
-        if pos % size != 0 or not USE_FASTPATH:
-            # XXX: maybe we are too conservative here? On most architectures,
-            # it is possible to read the data even if pos is not
-            # aligned. Also, probably it should responsibility of
-            # buf.typed_read to raise CannotRead in case it is not aligned
-            # *and* it is not supported.
+        if not USE_FASTPATH:
             raise CannotRead
         #
         if not ALLOW_FASTPATH:
diff --git a/rpython/rlib/rstruct/test/test_pack.py b/rpython/rlib/rstruct/test/test_pack.py
--- a/rpython/rlib/rstruct/test/test_pack.py
+++ b/rpython/rlib/rstruct/test/test_pack.py
@@ -2,8 +2,10 @@
 from rpython.rlib.rarithmetic import r_ulonglong
 from rpython.rlib.rstruct import standardfmttable, nativefmttable
 from rpython.rlib.rstruct.error import StructOverflowError
+from rpython.rlib import buffer
 from rpython.rlib.buffer import SubBuffer
 from rpython.rlib.mutbuffer import MutableStringBuffer
+from rpython.rlib import rawstorage
 import struct
 
 class FakeFormatIter(object):
@@ -40,16 +42,19 @@
     USE_FASTPATH = True
     ALLOW_SLOWPATH = True
     ALLOW_FASTPATH = True
+    ALLOW_UNALIGNED_ACCESS = rawstorage.misaligned_is_fine
     
     def setup_method(self, meth):
         standardfmttable.USE_FASTPATH = self.USE_FASTPATH
         standardfmttable.ALLOW_SLOWPATH = self.ALLOW_SLOWPATH
         standardfmttable.ALLOW_FASTPATH = self.ALLOW_FASTPATH
+        buffer.ALLOW_UNALIGNED_ACCESS = self.ALLOW_UNALIGNED_ACCESS
 
     def teardown_method(self, meth):
         standardfmttable.USE_FASTPATH = True
         standardfmttable.ALLOW_SLOWPATH = True
         standardfmttable.ALLOW_FASTPATH = True
+        buffer.ALLOW_UNALIGNED_ACCESS = rawstorage.misaligned_is_fine
 
     def mypack(self, fmt, value):
         size = struct.calcsize(fmt)
@@ -202,6 +207,7 @@
 
 class TestUnaligned(PackSupport):
     ALLOW_FASTPATH = False
+    ALLOW_UNALIGNED_ACCESS = False
     bigendian = nativefmttable.native_is_bigendian
     fmttable = nativefmttable.native_fmttable
 
@@ -227,6 +233,6 @@
         wbuf.setitem(0, chr(0xAB))
         wbuf.setitem(1, chr(0xCD))
         fake_fmtiter = self.mypack_into('i', wsubbuf, 0x1234)
-        assert fake_fmtiter.pos == wbuf.getlength()
+        assert fake_fmtiter.pos == wbuf.getlength()-2 # -2 since it's a SubBuffer
         got = wbuf.finish()
         assert got == expected