6:39 a.m.
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