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

[Frediano Ziglio]

> 
> On Fri, Sep 22, 2017 at 11:10:02AM +0100, Frediano Ziglio wrote:
> > 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))
> 
> For int32, this generates
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> *)(ptr))->v))
> rather than
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((int32_unaligned_t
> *)(ptr))->v))
> I don't know if this is intentional?

Yes, intentional and consistent with previous code.
The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.

> (the change would be:
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index da87d44..241e620 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -63,7 +63,7 @@ def write_parser_helpers(writer):
>              utype = "uint%d" % (size)
>              type = "%sint%d" % (sign, size)
>              swap = "SPICE_BYTESWAP%d" % size
> -            writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, utype))
> +            writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, type))
>              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 [16, 32, 64]:
> 
> Apart from this, looks good.
> 
> Christophe
> 

Frediano