[Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Frediano Ziglio]

Instead of assuming that the system can safely do unaligned access
to memory use packed structures to allow the compiler generate
best code possible.
A packed structure tells the compiler to not leave padding inside it
and that the structure can be unaligned so any field can be unaligned
having to generate proper access code based on architecture.
For instance ARM7 can use unaligned access but not for 64 bit
numbers (currently these accesses are emulated by Linux kernel
with obvious performance consequences).

This changes the current methods from:

#ifdef WORDS_BIGENDIAN
#define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr))))
#define write_uint32(ptr, val) *(uint32_t *)(ptr) = SPICE_BYTESWAP32((uint32_t)val)
#else
#define read_uint32(ptr) (*((uint32_t *)(ptr)))
#define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
#endif

to:

#include <spice/start-packed.h>
typedef struct SPICE_ATTR_PACKED {
    uint32_t v;
} uint32_unaligned_t;
#include <spice/end-packed.h>

#ifdef WORDS_BIGENDIAN
#define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t *)(ptr))->v))
#define write_uint32(ptr, val) ((uint32_unaligned_t *)(ptr))->v = SPICE_BYTESWAP32((uint32_t)val)
#else
#define read_uint32(ptr) (((uint32_unaligned_t *)(ptr))->v)
#define write_uint32(ptr, val) (((uint32_unaligned_t *)(ptr))->v) = val
#endif

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 common/marshaller.c         | 57 ++++++++++++++++++++++++++++++++-------------
 python_modules/demarshal.py | 33 ++++++++++++++++++--------
 2 files changed, 64 insertions(+), 26 deletions(-)

Changes since v1:
- add same code to marshaller.

diff --git a/common/marshaller.c b/common/marshaller.c
index 2fccd52..adfb8cd 100644
--- a/common/marshaller.c
+++ b/common/marshaller.c
@@ -28,24 +28,49 @@
 #include <unistd.h>
 #include <stdio.h>
 
+#include <spice/start-packed.h>
+typedef struct SPICE_ATTR_PACKED {
+    int16_t val;
+} int16_unaligned_t;
+
+typedef struct SPICE_ATTR_PACKED {
+    uint16_t val;
+} uint16_unaligned_t;
+
+typedef struct SPICE_ATTR_PACKED {
+    int32_t val;
+} int32_unaligned_t;
+
+typedef struct SPICE_ATTR_PACKED {
+    uint32_t val;
+} uint32_unaligned_t;
+
+typedef struct SPICE_ATTR_PACKED {
+    int64_t val;
+} int64_unaligned_t;
+
+typedef struct SPICE_ATTR_PACKED {
+    uint64_t val;
+} uint64_unaligned_t;
+#include <spice/end-packed.h>
+
+#define write_int8(ptr,v) (*(int8_t *)(ptr) = v)
+#define write_uint8(ptr,v) (*(uint8_t *)(ptr) = v)
+
 #ifdef WORDS_BIGENDIAN
-#define write_int8(ptr,v) (*((int8_t *)(ptr)) = v)
-#define write_uint8(ptr,v) (*((uint8_t *)(ptr)) = v)
-#define write_int16(ptr,v) (*((int16_t *)(ptr)) = SPICE_BYTESWAP16((uint16_t)(v)))
-#define write_uint16(ptr,v) (*((uint16_t *)(ptr)) = SPICE_BYTESWAP16((uint16_t)(v)))
-#define write_int32(ptr,v) (*((int32_t *)(ptr)) = SPICE_BYTESWAP32((uint32_t)(v)))
-#define write_uint32(ptr,v) (*((uint32_t *)(ptr)) = SPICE_BYTESWAP32((uint32_t)(v)))
-#define write_int64(ptr,v) (*((int64_t *)(ptr)) = SPICE_BYTESWAP64((uint64_t)(v)))
-#define write_uint64(ptr,v) (*((uint64_t *)(ptr)) = SPICE_BYTESWAP64((uint64_t)(v)))
+#define write_int16(ptr,v) (((uint16_unaligned_t *)(ptr))->val = SPICE_BYTESWAP16((uint16_t)(v)))
+#define write_uint16(ptr,v) (((uint16_unaligned_t *)(ptr))->val = SPICE_BYTESWAP16((uint16_t)(v)))
+#define write_int32(ptr,v) (((uint32_unaligned_t *)(ptr))->val = SPICE_BYTESWAP32((uint32_t)(v)))
+#define write_uint32(ptr,v) (((uint32_unaligned_t *)(ptr))->val = SPICE_BYTESWAP32((uint32_t)(v)))
+#define write_int64(ptr,v) (((uint64_unaligned_t *)(ptr))->val = SPICE_BYTESWAP64((uint64_t)(v)))
+#define write_uint64(ptr,v) (((uint64_unaligned_t *)(ptr))->val = SPICE_BYTESWAP64((uint64_t)(v)))
 #else
-#define write_int8(ptr,v) (*((int8_t *)(ptr)) = v)
-#define write_uint8(ptr,v) (*((uint8_t *)(ptr)) = v)
-#define write_int16(ptr,v) (*((int16_t *)(ptr)) = v)
-#define write_uint16(ptr,v) (*((uint16_t *)(ptr)) = v)
-#define write_int32(ptr,v) (*((int32_t *)(ptr)) = v)
-#define write_uint32(ptr,v) (*((uint32_t *)(ptr)) = v)
-#define write_int64(ptr,v) (*((int64_t *)(ptr)) = v)
-#define write_uint64(ptr,v) (*((uint64_t *)(ptr)) = v)
+#define write_int16(ptr,v) (((int16_unaligned_t *)(ptr))->val = v)
+#define write_uint16(ptr,v) (((uint16_unaligned_t *)(ptr))->val = v)
+#define write_int32(ptr,v) (((int32_unaligned_t *)(ptr))->val = v)
+#define write_uint32(ptr,v) (((uint32_unaligned_t *)(ptr))->val = v)
+#define write_int64(ptr,v) (((int64_unaligned_t *)(ptr))->val = v)
+#define write_uint64(ptr,v) (((uint64_unaligned_t *)(ptr))->val = v)
 #endif
 
 typedef struct {
diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index 1ea131d..da87d44 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -40,24 +40,37 @@ def write_parser_helpers(writer):
 
     writer = writer.function_helper()
 
+    writer.writeln("#include <spice/start-packed.h>")
+    for size in [16, 32, 64]:
+        for sign in ["", "u"]:
+            type = "%sint%d" % (sign, size)
+            writer.begin_block("typedef struct SPICE_ATTR_PACKED")
+            writer.variable_def("%s_t" % type, "v")
+            writer.end_block(newline=False)
+            writer.writeln(" %s_unaligned_t;" % type)
+    writer.writeln("#include <spice/end-packed.h>")
+    writer.newline()
+
+    for sign in ["", "u"]:
+        type = "%sint8" % sign
+        writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" % type)
+        writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr) = val" % (type))
+    writer.newline()
+
     writer.writeln("#ifdef WORDS_BIGENDIAN")
-    for size in [8, 16, 32, 64]:
+    for size in [16, 32, 64]:
         for sign in ["", "u"]:
             utype = "uint%d" % (size)
             type = "%sint%d" % (sign, size)
             swap = "SPICE_BYTESWAP%d" % size
-            if size == 8:
-                writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" % type)
-                writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr) = val" % (type))
-            else:
-                writer.macro("read_%s" % type, "ptr", "((%s_t)%s(*((%s_t *)(ptr))))" % (type, swap, utype))
-                writer.macro("write_%s" % type, "ptr, val", "*(%s_t *)(ptr) = %s((%s_t)val)" % (utype, swap, utype))
+            writer.macro("read_%s" % type, "ptr", "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, utype))
+            writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t *)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
     writer.writeln("#else")
-    for size in [8, 16, 32, 64]:
+    for size in [16, 32, 64]:
         for sign in ["", "u"]:
             type = "%sint%d" % (sign, size)
-            writer.macro("read_%s" % type, "ptr", "(*((%s_t *)(ptr)))" % type)
-            writer.macro("write_%s" % type, "ptr, val", "(*((%s_t *)(ptr))) = val" % type)
+            writer.macro("read_%s" % type, "ptr", "(((%s_unaligned_t *)(ptr))->v)" % type)
+            writer.macro("write_%s" % type, "ptr, val", "(((%s_unaligned_t *)(ptr))->v) = val" % type)
     writer.writeln("#endif")
 
     for size in [8, 16, 32, 64]:
-- 
2.13.5