On 4/23/2012 1:25 PM, Angus Salkeld wrote:
On 23/04/12 12:05 +0200, Fabio M. Di Nitto wrote:
On 4/23/2012 11:42 AM, Angus Salkeld wrote:
Hi all
Fabio noticed that we have some odd attributes on the qb_ipc_header structs.
They currently have (aligned(8)) and should really have (packed) for efficient packing on-wire (http://gcc.gnu.org/onlinedocs/gcc-3.1/gcc/Type-Attributes.html).
He noticed this while doing some valgind testing.
This is not so easy to fix as it will break ABI.
So like this:
diff --git a/include/qb/qbipc_common.h b/include/qb/qbipc_common.h index 0a4118c..7af2c7e 100644 --- a/include/qb/qbipc_common.h +++ b/include/qb/qbipc_common.h @@ -32,15 +32,15 @@ extern "C" { /* *INDENT-ON* */
struct qb_ipc_request_header {
int32_t id __attribute__ ((aligned(8)));
int32_t size __attribute__ ((aligned(8)));
-} __attribute__ ((aligned(8)));
int32_t id;
int32_t size;
+} __attribute__ (packed);
struct qb_ipc_response_header {
int32_t id __attribute__ ((aligned(8)));
int32_t size __attribute__ ((aligned(8)));
int32_t error __attribute__ ((aligned(8)));
-} __attribute__ ((aligned(8)));
int32_t id;
int32_t size;
int32_t error;
+} __attribute__ (packed);
enum qb_ipc_type { QB_IPC_SOCKET,
Because the alignment changes per arch, this is not so easy to work around.
Is this a problem? Can we just leave it as-is? What other options are there?
One step back here..
The issue here is that we get one error/warning for each packet we send on-wire from valgrind for uninitialized memory passing around, this is because the aligned(8) for each structure entry, adds padding.
For operational / runtime / production, this has only the issue to double the size of the structure itself, leaving less space for user data (aka a small per hit).
But for debugging purposes this is a nightmare and makes valgrind very hard to use.
We have different options here, but only correct one, as Angus already mention, is going to break ABI and onwire compat.
The other possible solutions are:
1 memset (&qb_ipc_*header,..)
2 silence valgrind (http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress)
3 rewrite the structure to add explicit padding instead of aligned and init those values from corosync (this isn´t so much better than adding memset, but it makes it explicit what´s in the structure and in theory we gain 2 extra fields for further use, without breaking onwire).
On all arches - are you sure of the padding size and whether the padding needs to be before or after the value?
the size is pretty set.. int32_t in the structure is fixed across all arches and to pad to 8 bytes, there is only one answer ;)
as for before or after, i don´t know. we will need to investigate.
I think as long as we don´t break onwire for x86 and x86_64 we should be good.
The other benefit of using solution #3 is that we don´t let gcc decide where the padding is (and rely on something external) to determine on-wire compatibility).
If gcc behaves differently in respect of "before or after" based on version and/or architecture, that would introduce other incompatibilities.
Fabio