[PATCH 3/3] ewah: support platforms that require aligned reads

[Jeff King]

From: Vicent Marti tan...@gmail.com

The caller may hand us an unaligned buffer (e.g., because it
is an mmap of a file with many ewah bitmaps). On some
platforms (like SPARC) this can cause a bus error. We can
fix it with a combination of get_be32 and moving the data
into an aligned buffer (which we would do anyway, but we can
move it before fixing the endianness).

Signed-off-by: Vicent Marti tan...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
Tested on the SPARC I have access to. Please double-check that it also
works fine on ARM.

 ewah/ewah_io.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index aed0da6..4a7fae6 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -112,23 +112,38 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
 
 int ewah_read_mmap(struct ewah_bitmap *self, void *map, size_t len)
 {
-   uint32_t *read32 = map;
-   eword_t *read64;
-   size_t i;
+   uint8_t *ptr = map;
+
+   self-bit_size = get_be32(ptr);
+   ptr += sizeof(uint32_t);
+
+   self-buffer_size = self-alloc_size = get_be32(ptr);
+   ptr += sizeof(uint32_t);
 
-   self-bit_size = ntohl(*read32++);
-   self-buffer_size = self-alloc_size = ntohl(*read32++);
self-buffer = ewah_realloc(self-buffer,
self-alloc_size * sizeof(eword_t));
 
if (!self-buffer)
return -1;
 
-   for (i = 0, read64 = (void *)read32; i  self-buffer_size; ++i)
-   self-buffer[i] = ntohll(*read64++);
+   /*
+* Copy the raw data for the bitmap as a whole chunk;
+* if we're in a little-endian platform, we'll perform
+* the endianness conversion in a separate pass to ensure
+* we're loading 8-byte aligned words.
+*/
+   memcpy(self-buffer, ptr, self-buffer_size * sizeof(uint64_t));
+   ptr += self-buffer_size * sizeof(uint64_t);
+
+#if __BYTE_ORDER != __BIG_ENDIAN
+   {
+   size_t i;
+   for (i = 0; i  self-buffer_size; ++i)
+   self-buffer[i] = ntohll(self-buffer[i]);
+   }
+#endif
 
-   read32 = (void *)read64;
-   self-rlw = self-buffer + ntohl(*read32++);
+   self-rlw = self-buffer + get_be32(ptr);
 
return (3 * 4) + (self-buffer_size * 8);
 }
-- 
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ewah: support platforms that require aligned reads

[Jonathan Nieder]

Jeff King wrote:

 --- a/ewah/ewah_io.c
 +++ b/ewah/ewah_io.c
 @@ -112,23 +112,38 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
[...]
 +#if __BYTE_ORDER != __BIG_ENDIAN

Is this portable?

On a platform without __BYTE_ORDER or __BIG_ENDIAN defined,
it is interpreted as

#if 0 != 0

which means that such platforms are assumed to be big endian.
Does Mingw define __BYTE_ORDER, for example?


 + {
 + size_t i;
 + for (i = 0; i  self-buffer_size; ++i)
 + self-buffer[i] = ntohll(self-buffer[i]);
 + }
 +#endif

It's tempting to guard with something like

if (ntohl(1) != 1) {
...
}

The optimizer can tell if this is true or false at compile time, so
it shouldn't slow anything down.

With that change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Thanks for the quick fix.

diff --git i/ewah/ewah_io.c w/ewah/ewah_io.c
index 4a7fae6..5a527a4 100644
--- i/ewah/ewah_io.c
+++ w/ewah/ewah_io.c
@@ -135,13 +135,11 @@ int ewah_read_mmap(struct ewah_bitmap *self, void *map, 
size_t len)
memcpy(self-buffer, ptr, self-buffer_size * sizeof(uint64_t));
ptr += self-buffer_size * sizeof(uint64_t);
 
-#if __BYTE_ORDER != __BIG_ENDIAN
-   {
+   if (ntohl(1) != 1) {
size_t i;
for (i = 0; i  self-buffer_size; ++i)
self-buffer[i] = ntohll(self-buffer[i]);
}
-#endif
 
self-rlw = self-buffer + get_be32(ptr);
 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ewah: support platforms that require aligned reads

[Vicent Martí]

On Fri, Jan 24, 2014 at 12:44 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 --- a/ewah/ewah_io.c
 +++ b/ewah/ewah_io.c
 @@ -112,23 +112,38 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
 [...]
 +#if __BYTE_ORDER != __BIG_ENDIAN

 Is this portable?

We explicitly set the __BYTE_ORDER macros in `compat/bswap.h`. In
fact, this preprocessor conditional is the same one that we use when
choosing what version of the `ntohl` macro to define, so that's why I
decided to use it here.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ewah: support platforms that require aligned reads

[Jonathan Nieder]

Vicent Martí wrote:
 On Fri, Jan 24, 2014 at 12:44 AM, Jonathan Nieder jrnie...@gmail.com wrote:

 +#if __BYTE_ORDER != __BIG_ENDIAN

 Is this portable?

 We explicitly set the __BYTE_ORDER macros in `compat/bswap.h`. In
 fact, this preprocessor conditional is the same one that we use when
 choosing what version of the `ntohl` macro to define, so that's why I
 decided to use it here.

Ah, thanks.  Sorry I missed that.  So feel free to add my reviewed-by
to the patch without my tweak, too.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html