On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list