On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Playing games with field offsets in a struct causes all sorts of alignment warnings on ARM platforms
util/virkeycode.c: In function '__virKeycodeValueFromString': util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset))
Wow. The end result is still properly aligned if object was aligned to begin with, but the warning is quite appropriate, as the code is hard to follow.
There is no compelling reason to use a struct for the keycode tables. It can easily just use an array of arrays instead, avoiding all alignment problems
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virkeycode-mapgen.py | 78 ++++++++++++++++++------- src/util/virkeycode.c | 130 ++++++++++++++++++------------------------ 2 files changed, 110 insertions(+), 98 deletions(-)
[Fair warning - my python is weak, so I cheated and looked at the generated C code virkeymaps.h file pre- and post-patch instead - you basically went from a single table of structs: -struct keycode virKeycodes[] = { - { "KEY_RESERVED",NULL,NULL,0,0,0,0,0,0,0,0,0,0}, to multiple tables of one piece of information per table: +const char *virKeymapNames_linux[] = { + "KEY_RESERVED", ... +unsigned short virKeymapValues_linux[] = { + 0, ]
+++ b/src/util/virkeycode.c @@ -22,51 +22,57 @@ #include <string.h> #include <stddef.h>
-#define getfield(object, field_type, field_offset) \ - (*(typeof(field_type) *)((char *)(object) + field_offset)) - -struct keycode { - const char *linux_name; - const char *os_x_name; - const char *win32_name; - unsigned short linux_keycode; - unsigned short os_x; - unsigned short atset1; - unsigned short atset2; - unsigned short atset3; - unsigned short xt; - unsigned short xt_kbd; - unsigned short usb; - unsigned short win32; - unsigned short rfb; -};
This maps up with dropping the struct...
#define VIRT_KEY_INTERNAL #include "virkeymaps.h"
-static unsigned int codeOffset[] = { +static const char **virKeymapNames[] = { [VIR_KEYCODE_SET_LINUX] = - offsetof(struct keycode, linux_keycode), + virKeymapNames_linux,
...and this maps up with pointing to the individual table, rather than a funky offset within the struct at element 0 of the big array. ACK (once you clean up the nit that Michal pointed out about 'make syntax-check') -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org