diff mbox

[PULL] slirp: Fix issues with -mms-bitfields

Message ID 4E48BF2F.7080901@web.de
State New
Headers show

Pull-request

git://git.kiszka.org/qemu.git queues/slirp

Commit Message

Jan Kiszka Aug. 15, 2011, 6:39 a.m. UTC
The following changes since commit 3b6ffe50300f13240e1b46420ad05da1116df410:

  hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25 +0000)

are available in the git repository at:
  git://git.kiszka.org/qemu.git queues/slirp

Jan Kiszka (1):
      slirp: Fix bit field types in IP header structs

 slirp/ip.h  |    8 ++++----
 slirp/tcp.h |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

---

slirp: Fix bit field types in IP header structs

-mms-bitfields prevents that the bitfields in current IP header structs
are packed into a single byte as it is required. Fix this by using
uint8_t as backing type.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/ip.h  |    8 ++++----
 slirp/tcp.h |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Stefan Weil Aug. 20, 2011, 8 p.m. UTC | #1
Am 15.08.2011 08:39, schrieb Jan Kiszka:
> The following changes since commit 
> 3b6ffe50300f13240e1b46420ad05da1116df410:
>
> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25 
> +0000)
>
> are available in the git repository at:
> git://git.kiszka.org/qemu.git queues/slirp
>
> Jan Kiszka (1):
> slirp: Fix bit field types in IP header structs
>
> slirp/ip.h | 8 ++++----
> slirp/tcp.h | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> ---
>
> slirp: Fix bit field types in IP header structs
>
> -mms-bitfields prevents that the bitfields in current IP header structs
> are packed into a single byte as it is required. Fix this by using
> uint8_t as backing type.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> slirp/ip.h | 8 ++++----
> slirp/tcp.h | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/slirp/ip.h b/slirp/ip.h
> index 48ea38e..72dbe9a 100644
> --- a/slirp/ip.h
> +++ b/slirp/ip.h
> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from 
> the net */
> */
> struct ip {
> #ifdef HOST_WORDS_BIGENDIAN
> - u_int ip_v:4, /* version */
> + uint8_t ip_v:4, /* version */
> ip_hl:4; /* header length */
> #else
> - u_int ip_hl:4, /* header length */
> + uint8_t ip_hl:4, /* header length */
> ip_v:4; /* version */
> #endif
> uint8_t ip_tos; /* type of service */
> @@ -140,10 +140,10 @@ struct ip_timestamp {
> uint8_t ipt_len; /* size of structure (variable) */
> uint8_t ipt_ptr; /* index of current entry */
> #ifdef HOST_WORDS_BIGENDIAN
> - u_int ipt_oflw:4, /* overflow counter */
> + uint8_t ipt_oflw:4, /* overflow counter */
> ipt_flg:4; /* flags, see below */
> #else
> - u_int ipt_flg:4, /* flags, see below */
> + uint8_t ipt_flg:4, /* flags, see below */
> ipt_oflw:4; /* overflow counter */
> #endif
> union ipt_timestamp {
> diff --git a/slirp/tcp.h b/slirp/tcp.h
> index 9d06836..b3817cb 100644
> --- a/slirp/tcp.h
> +++ b/slirp/tcp.h
> @@ -51,10 +51,10 @@ struct tcphdr {
> tcp_seq th_seq; /* sequence number */
> tcp_seq th_ack; /* acknowledgement number */
> #ifdef HOST_WORDS_BIGENDIAN
> - u_int th_off:4, /* data offset */
> + uint8_t th_off:4, /* data offset */
> th_x2:4; /* (unused) */
> #else
> - u_int th_x2:4, /* (unused) */
> + uint8_t th_x2:4, /* (unused) */
> th_off:4; /* data offset */
> #endif
> uint8_t th_flags;

Tested-by: Stefan Weil <weil@mail.berlios.de>

There remain some issues with other packed structs
which are obviously no longer packed with -mms-bitfields,
for example those from bt.h, but slirp looks good with
this patch.

I used pahole (thanks Blue Swirl for this hint) and codiff
to investigate and compare the structs with and without
-mms-bitfields:

* create qemu.exe (or any binary which you want)
* convert it to elf format using objcopy -Oelf32-i386
   (or the cross variant)
* apply pahole -a to the resulting elf file
   (without -a some structs are missing)

Cheers,
Stefan W.
Blue Swirl Aug. 21, 2011, 7:49 p.m. UTC | #2
Thanks, pulled.

On Mon, Aug 15, 2011 at 6:39 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> The following changes since commit 3b6ffe50300f13240e1b46420ad05da1116df410:
>
>  hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25 +0000)
>
> are available in the git repository at:
>  git://git.kiszka.org/qemu.git queues/slirp
>
> Jan Kiszka (1):
>      slirp: Fix bit field types in IP header structs
>
>  slirp/ip.h  |    8 ++++----
>  slirp/tcp.h |    4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> ---
>
> slirp: Fix bit field types in IP header structs
>
> -mms-bitfields prevents that the bitfields in current IP header structs
> are packed into a single byte as it is required. Fix this by using
> uint8_t as backing type.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  slirp/ip.h  |    8 ++++----
>  slirp/tcp.h |    4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/slirp/ip.h b/slirp/ip.h
> index 48ea38e..72dbe9a 100644
> --- a/slirp/ip.h
> +++ b/slirp/ip.h
> @@ -74,10 +74,10 @@ typedef uint32_t n_long;                 /* long as received from the net */
>  */
>  struct ip {
>  #ifdef HOST_WORDS_BIGENDIAN
> -       u_int ip_v:4,                   /* version */
> +       uint8_t ip_v:4,                 /* version */
>                ip_hl:4;                /* header length */
>  #else
> -       u_int ip_hl:4,          /* header length */
> +       uint8_t ip_hl:4,                /* header length */
>                ip_v:4;                 /* version */
>  #endif
>        uint8_t         ip_tos;                 /* type of service */
> @@ -140,10 +140,10 @@ struct    ip_timestamp {
>        uint8_t ipt_len;                /* size of structure (variable) */
>        uint8_t ipt_ptr;                /* index of current entry */
>  #ifdef HOST_WORDS_BIGENDIAN
> -       u_int   ipt_oflw:4,             /* overflow counter */
> +       uint8_t ipt_oflw:4,             /* overflow counter */
>                ipt_flg:4;              /* flags, see below */
>  #else
> -       u_int   ipt_flg:4,              /* flags, see below */
> +       uint8_t ipt_flg:4,              /* flags, see below */
>                ipt_oflw:4;             /* overflow counter */
>  #endif
>        union ipt_timestamp {
> diff --git a/slirp/tcp.h b/slirp/tcp.h
> index 9d06836..b3817cb 100644
> --- a/slirp/tcp.h
> +++ b/slirp/tcp.h
> @@ -51,10 +51,10 @@ struct tcphdr {
>        tcp_seq th_seq;                 /* sequence number */
>        tcp_seq th_ack;                 /* acknowledgement number */
>  #ifdef HOST_WORDS_BIGENDIAN
> -       u_int   th_off:4,               /* data offset */
> +       uint8_t th_off:4,               /* data offset */
>                th_x2:4;                /* (unused) */
>  #else
> -       u_int   th_x2:4,                /* (unused) */
> +       uint8_t th_x2:4,                /* (unused) */
>                th_off:4;               /* data offset */
>  #endif
>        uint8_t th_flags;
>
>
>
TeLeMan Aug. 23, 2011, 10:49 a.m. UTC | #3
On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>
>> The following changes since commit
>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>
>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>> +0000)
>>
>> are available in the git repository at:
>> git://git.kiszka.org/qemu.git queues/slirp
>>
>> Jan Kiszka (1):
>> slirp: Fix bit field types in IP header structs
>>
>> slirp/ip.h | 8 ++++----
>> slirp/tcp.h | 4 ++--
>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> ---
>>
>> slirp: Fix bit field types in IP header structs
>>
>> -mms-bitfields prevents that the bitfields in current IP header structs
>> are packed into a single byte as it is required. Fix this by using
>> uint8_t as backing type.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> slirp/ip.h | 8 ++++----
>> slirp/tcp.h | 4 ++--
>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/slirp/ip.h b/slirp/ip.h
>> index 48ea38e..72dbe9a 100644
>> --- a/slirp/ip.h
>> +++ b/slirp/ip.h
>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>> net */
>> */
>> struct ip {
>> #ifdef HOST_WORDS_BIGENDIAN
>> - u_int ip_v:4, /* version */
>> + uint8_t ip_v:4, /* version */
>> ip_hl:4; /* header length */
>> #else
>> - u_int ip_hl:4, /* header length */
>> + uint8_t ip_hl:4, /* header length */
>> ip_v:4; /* version */
>> #endif
>> uint8_t ip_tos; /* type of service */
>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>> uint8_t ipt_len; /* size of structure (variable) */
>> uint8_t ipt_ptr; /* index of current entry */
>> #ifdef HOST_WORDS_BIGENDIAN
>> - u_int ipt_oflw:4, /* overflow counter */
>> + uint8_t ipt_oflw:4, /* overflow counter */
>> ipt_flg:4; /* flags, see below */
>> #else
>> - u_int ipt_flg:4, /* flags, see below */
>> + uint8_t ipt_flg:4, /* flags, see below */
>> ipt_oflw:4; /* overflow counter */
>> #endif
>> union ipt_timestamp {
>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>> index 9d06836..b3817cb 100644
>> --- a/slirp/tcp.h
>> +++ b/slirp/tcp.h
>> @@ -51,10 +51,10 @@ struct tcphdr {
>> tcp_seq th_seq; /* sequence number */
>> tcp_seq th_ack; /* acknowledgement number */
>> #ifdef HOST_WORDS_BIGENDIAN
>> - u_int th_off:4, /* data offset */
>> + uint8_t th_off:4, /* data offset */
>> th_x2:4; /* (unused) */
>> #else
>> - u_int th_x2:4, /* (unused) */
>> + uint8_t th_x2:4, /* (unused) */
>> th_off:4; /* data offset */
>> #endif
>> uint8_t th_flags;
>
> Tested-by: Stefan Weil <weil@mail.berlios.de>
>
slirp is still broken on my mingw32. I used "#progma
pack(push,1)/#progma pack(pop)" to resolve this issue.

> There remain some issues with other packed structs
> which are obviously no longer packed with -mms-bitfields,
> for example those from bt.h, but slirp looks good with
> this patch.
>
> I used pahole (thanks Blue Swirl for this hint) and codiff
> to investigate and compare the structs with and without
> -mms-bitfields:
>
> * create qemu.exe (or any binary which you want)
> * convert it to elf format using objcopy -Oelf32-i386
>  (or the cross variant)
> * apply pahole -a to the resulting elf file
>  (without -a some structs are missing)
>
> Cheers,
> Stefan W.
>
>
>
Jan Kiszka Aug. 24, 2011, 9:11 a.m. UTC | #4
On 2011-08-23 12:49, TeLeMan wrote:
> On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
>> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>>
>>> The following changes since commit
>>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>>
>>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>>> +0000)
>>>
>>> are available in the git repository at:
>>> git://git.kiszka.org/qemu.git queues/slirp
>>>
>>> Jan Kiszka (1):
>>> slirp: Fix bit field types in IP header structs
>>>
>>> slirp/ip.h | 8 ++++----
>>> slirp/tcp.h | 4 ++--
>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> ---
>>>
>>> slirp: Fix bit field types in IP header structs
>>>
>>> -mms-bitfields prevents that the bitfields in current IP header structs
>>> are packed into a single byte as it is required. Fix this by using
>>> uint8_t as backing type.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>> slirp/ip.h | 8 ++++----
>>> slirp/tcp.h | 4 ++--
>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/slirp/ip.h b/slirp/ip.h
>>> index 48ea38e..72dbe9a 100644
>>> --- a/slirp/ip.h
>>> +++ b/slirp/ip.h
>>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>>> net */
>>> */
>>> struct ip {
>>> #ifdef HOST_WORDS_BIGENDIAN
>>> - u_int ip_v:4, /* version */
>>> + uint8_t ip_v:4, /* version */
>>> ip_hl:4; /* header length */
>>> #else
>>> - u_int ip_hl:4, /* header length */
>>> + uint8_t ip_hl:4, /* header length */
>>> ip_v:4; /* version */
>>> #endif
>>> uint8_t ip_tos; /* type of service */
>>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>>> uint8_t ipt_len; /* size of structure (variable) */
>>> uint8_t ipt_ptr; /* index of current entry */
>>> #ifdef HOST_WORDS_BIGENDIAN
>>> - u_int ipt_oflw:4, /* overflow counter */
>>> + uint8_t ipt_oflw:4, /* overflow counter */
>>> ipt_flg:4; /* flags, see below */
>>> #else
>>> - u_int ipt_flg:4, /* flags, see below */
>>> + uint8_t ipt_flg:4, /* flags, see below */
>>> ipt_oflw:4; /* overflow counter */
>>> #endif
>>> union ipt_timestamp {
>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>> index 9d06836..b3817cb 100644
>>> --- a/slirp/tcp.h
>>> +++ b/slirp/tcp.h
>>> @@ -51,10 +51,10 @@ struct tcphdr {
>>> tcp_seq th_seq; /* sequence number */
>>> tcp_seq th_ack; /* acknowledgement number */
>>> #ifdef HOST_WORDS_BIGENDIAN
>>> - u_int th_off:4, /* data offset */
>>> + uint8_t th_off:4, /* data offset */
>>> th_x2:4; /* (unused) */
>>> #else
>>> - u_int th_x2:4, /* (unused) */
>>> + uint8_t th_x2:4, /* (unused) */
>>> th_off:4; /* data offset */
>>> #endif
>>> uint8_t th_flags;
>>
>> Tested-by: Stefan Weil <weil@mail.berlios.de>
>>
> slirp is still broken on my mingw32. I used "#progma
> pack(push,1)/#progma pack(pop)" to resolve this issue.

Can you drill down to the bottom of this problem? What fields in what
struct are not properly packed? Maybe this is now a compiler bug, so
comparing versions may make sense as well.

Jan
TeLeMan Aug. 25, 2011, 1:14 a.m. UTC | #5
On Wed, Aug 24, 2011 at 17:11, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-08-23 12:49, TeLeMan wrote:
>> On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
>>> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>>>
>>>> The following changes since commit
>>>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>>>
>>>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>>>> +0000)
>>>>
>>>> are available in the git repository at:
>>>> git://git.kiszka.org/qemu.git queues/slirp
>>>>
>>>> Jan Kiszka (1):
>>>> slirp: Fix bit field types in IP header structs
>>>>
>>>> slirp/ip.h | 8 ++++----
>>>> slirp/tcp.h | 4 ++--
>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> ---
>>>>
>>>> slirp: Fix bit field types in IP header structs
>>>>
>>>> -mms-bitfields prevents that the bitfields in current IP header structs
>>>> are packed into a single byte as it is required. Fix this by using
>>>> uint8_t as backing type.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>> slirp/ip.h | 8 ++++----
>>>> slirp/tcp.h | 4 ++--
>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/slirp/ip.h b/slirp/ip.h
>>>> index 48ea38e..72dbe9a 100644
>>>> --- a/slirp/ip.h
>>>> +++ b/slirp/ip.h
>>>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>>>> net */
>>>> */
>>>> struct ip {
>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>> - u_int ip_v:4, /* version */
>>>> + uint8_t ip_v:4, /* version */
>>>> ip_hl:4; /* header length */
>>>> #else
>>>> - u_int ip_hl:4, /* header length */
>>>> + uint8_t ip_hl:4, /* header length */
>>>> ip_v:4; /* version */
>>>> #endif
>>>> uint8_t ip_tos; /* type of service */
>>>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>>>> uint8_t ipt_len; /* size of structure (variable) */
>>>> uint8_t ipt_ptr; /* index of current entry */
>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>> - u_int ipt_oflw:4, /* overflow counter */
>>>> + uint8_t ipt_oflw:4, /* overflow counter */
>>>> ipt_flg:4; /* flags, see below */
>>>> #else
>>>> - u_int ipt_flg:4, /* flags, see below */
>>>> + uint8_t ipt_flg:4, /* flags, see below */
>>>> ipt_oflw:4; /* overflow counter */
>>>> #endif
>>>> union ipt_timestamp {
>>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>>> index 9d06836..b3817cb 100644
>>>> --- a/slirp/tcp.h
>>>> +++ b/slirp/tcp.h
>>>> @@ -51,10 +51,10 @@ struct tcphdr {
>>>> tcp_seq th_seq; /* sequence number */
>>>> tcp_seq th_ack; /* acknowledgement number */
>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>> - u_int th_off:4, /* data offset */
>>>> + uint8_t th_off:4, /* data offset */
>>>> th_x2:4; /* (unused) */
>>>> #else
>>>> - u_int th_x2:4, /* (unused) */
>>>> + uint8_t th_x2:4, /* (unused) */
>>>> th_off:4; /* data offset */
>>>> #endif
>>>> uint8_t th_flags;
>>>
>>> Tested-by: Stefan Weil <weil@mail.berlios.de>
>>>
>> slirp is still broken on my mingw32. I used "#progma
>> pack(push,1)/#progma pack(pop)" to resolve this issue.
>
> Can you drill down to the bottom of this problem? What fields in what
> struct are not properly packed? Maybe this is now a compiler bug, so
> comparing versions may make sense as well.
arphdr.ar_sip is not aligned on  a 4-byte boundary. See my previous post:
http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html

>
> Jan
>
>
Jan Kiszka Aug. 25, 2011, 11:04 a.m. UTC | #6
On 2011-08-25 03:14, TeLeMan wrote:
> On Wed, Aug 24, 2011 at 17:11, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-08-23 12:49, TeLeMan wrote:
>>> On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
>>>> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>>>>
>>>>> The following changes since commit
>>>>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>>>>
>>>>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>>>>> +0000)
>>>>>
>>>>> are available in the git repository at:
>>>>> git://git.kiszka.org/qemu.git queues/slirp
>>>>>
>>>>> Jan Kiszka (1):
>>>>> slirp: Fix bit field types in IP header structs
>>>>>
>>>>> slirp/ip.h | 8 ++++----
>>>>> slirp/tcp.h | 4 ++--
>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> ---
>>>>>
>>>>> slirp: Fix bit field types in IP header structs
>>>>>
>>>>> -mms-bitfields prevents that the bitfields in current IP header structs
>>>>> are packed into a single byte as it is required. Fix this by using
>>>>> uint8_t as backing type.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>> slirp/ip.h | 8 ++++----
>>>>> slirp/tcp.h | 4 ++--
>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/slirp/ip.h b/slirp/ip.h
>>>>> index 48ea38e..72dbe9a 100644
>>>>> --- a/slirp/ip.h
>>>>> +++ b/slirp/ip.h
>>>>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>>>>> net */
>>>>> */
>>>>> struct ip {
>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>> - u_int ip_v:4, /* version */
>>>>> + uint8_t ip_v:4, /* version */
>>>>> ip_hl:4; /* header length */
>>>>> #else
>>>>> - u_int ip_hl:4, /* header length */
>>>>> + uint8_t ip_hl:4, /* header length */
>>>>> ip_v:4; /* version */
>>>>> #endif
>>>>> uint8_t ip_tos; /* type of service */
>>>>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>>>>> uint8_t ipt_len; /* size of structure (variable) */
>>>>> uint8_t ipt_ptr; /* index of current entry */
>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>> - u_int ipt_oflw:4, /* overflow counter */
>>>>> + uint8_t ipt_oflw:4, /* overflow counter */
>>>>> ipt_flg:4; /* flags, see below */
>>>>> #else
>>>>> - u_int ipt_flg:4, /* flags, see below */
>>>>> + uint8_t ipt_flg:4, /* flags, see below */
>>>>> ipt_oflw:4; /* overflow counter */
>>>>> #endif
>>>>> union ipt_timestamp {
>>>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>>>> index 9d06836..b3817cb 100644
>>>>> --- a/slirp/tcp.h
>>>>> +++ b/slirp/tcp.h
>>>>> @@ -51,10 +51,10 @@ struct tcphdr {
>>>>> tcp_seq th_seq; /* sequence number */
>>>>> tcp_seq th_ack; /* acknowledgement number */
>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>> - u_int th_off:4, /* data offset */
>>>>> + uint8_t th_off:4, /* data offset */
>>>>> th_x2:4; /* (unused) */
>>>>> #else
>>>>> - u_int th_x2:4, /* (unused) */
>>>>> + uint8_t th_x2:4, /* (unused) */
>>>>> th_off:4; /* data offset */
>>>>> #endif
>>>>> uint8_t th_flags;
>>>>
>>>> Tested-by: Stefan Weil <weil@mail.berlios.de>
>>>>
>>> slirp is still broken on my mingw32. I used "#progma
>>> pack(push,1)/#progma pack(pop)" to resolve this issue.
>>
>> Can you drill down to the bottom of this problem? What fields in what
>> struct are not properly packed? Maybe this is now a compiler bug, so
>> comparing versions may make sense as well.
> arphdr.ar_sip is not aligned on  a 4-byte boundary. See my previous post:
> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html

What a mess. Do we really have to go through all 257 packed data structs
in QEMU and add these MS compat bits to all potentially affected ones?

Jan
TeLeMan Aug. 25, 2011, 12:02 p.m. UTC | #7
On Thu, Aug 25, 2011 at 19:04, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-08-25 03:14, TeLeMan wrote:
>> On Wed, Aug 24, 2011 at 17:11, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-08-23 12:49, TeLeMan wrote:
>>>> On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
>>>>> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>>>>>
>>>>>> The following changes since commit
>>>>>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>>>>>
>>>>>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>>>>>> +0000)
>>>>>>
>>>>>> are available in the git repository at:
>>>>>> git://git.kiszka.org/qemu.git queues/slirp
>>>>>>
>>>>>> Jan Kiszka (1):
>>>>>> slirp: Fix bit field types in IP header structs
>>>>>>
>>>>>> slirp/ip.h | 8 ++++----
>>>>>> slirp/tcp.h | 4 ++--
>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> slirp: Fix bit field types in IP header structs
>>>>>>
>>>>>> -mms-bitfields prevents that the bitfields in current IP header structs
>>>>>> are packed into a single byte as it is required. Fix this by using
>>>>>> uint8_t as backing type.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>> slirp/ip.h | 8 ++++----
>>>>>> slirp/tcp.h | 4 ++--
>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/slirp/ip.h b/slirp/ip.h
>>>>>> index 48ea38e..72dbe9a 100644
>>>>>> --- a/slirp/ip.h
>>>>>> +++ b/slirp/ip.h
>>>>>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>>>>>> net */
>>>>>> */
>>>>>> struct ip {
>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>> - u_int ip_v:4, /* version */
>>>>>> + uint8_t ip_v:4, /* version */
>>>>>> ip_hl:4; /* header length */
>>>>>> #else
>>>>>> - u_int ip_hl:4, /* header length */
>>>>>> + uint8_t ip_hl:4, /* header length */
>>>>>> ip_v:4; /* version */
>>>>>> #endif
>>>>>> uint8_t ip_tos; /* type of service */
>>>>>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>>>>>> uint8_t ipt_len; /* size of structure (variable) */
>>>>>> uint8_t ipt_ptr; /* index of current entry */
>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>> - u_int ipt_oflw:4, /* overflow counter */
>>>>>> + uint8_t ipt_oflw:4, /* overflow counter */
>>>>>> ipt_flg:4; /* flags, see below */
>>>>>> #else
>>>>>> - u_int ipt_flg:4, /* flags, see below */
>>>>>> + uint8_t ipt_flg:4, /* flags, see below */
>>>>>> ipt_oflw:4; /* overflow counter */
>>>>>> #endif
>>>>>> union ipt_timestamp {
>>>>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>>>>> index 9d06836..b3817cb 100644
>>>>>> --- a/slirp/tcp.h
>>>>>> +++ b/slirp/tcp.h
>>>>>> @@ -51,10 +51,10 @@ struct tcphdr {
>>>>>> tcp_seq th_seq; /* sequence number */
>>>>>> tcp_seq th_ack; /* acknowledgement number */
>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>> - u_int th_off:4, /* data offset */
>>>>>> + uint8_t th_off:4, /* data offset */
>>>>>> th_x2:4; /* (unused) */
>>>>>> #else
>>>>>> - u_int th_x2:4, /* (unused) */
>>>>>> + uint8_t th_x2:4, /* (unused) */
>>>>>> th_off:4; /* data offset */
>>>>>> #endif
>>>>>> uint8_t th_flags;
>>>>>
>>>>> Tested-by: Stefan Weil <weil@mail.berlios.de>
>>>>>
>>>> slirp is still broken on my mingw32. I used "#progma
>>>> pack(push,1)/#progma pack(pop)" to resolve this issue.
>>>
>>> Can you drill down to the bottom of this problem? What fields in what
>>> struct are not properly packed? Maybe this is now a compiler bug, so
>>> comparing versions may make sense as well.
>> arphdr.ar_sip is not aligned on  a 4-byte boundary. See my previous post:
>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html
>
> What a mess. Do we really have to go through all 257 packed data structs
> in QEMU and add these MS compat bits to all potentially affected ones?
I prefer to detect -mms-bitfields and remove it in configure.
Jan Kiszka Aug. 25, 2011, 12:38 p.m. UTC | #8
On 2011-08-25 14:02, TeLeMan wrote:
> On Thu, Aug 25, 2011 at 19:04, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-08-25 03:14, TeLeMan wrote:
>>> On Wed, Aug 24, 2011 at 17:11, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2011-08-23 12:49, TeLeMan wrote:
>>>>> On Sun, Aug 21, 2011 at 04:00, Stefan Weil <weil@mail.berlios.de> wrote:
>>>>>> Am 15.08.2011 08:39, schrieb Jan Kiszka:
>>>>>>>
>>>>>>> The following changes since commit
>>>>>>> 3b6ffe50300f13240e1b46420ad05da1116df410:
>>>>>>>
>>>>>>> hw/scsi-bus.c: Fix use of uninitialised variable (2011-08-14 19:34:25
>>>>>>> +0000)
>>>>>>>
>>>>>>> are available in the git repository at:
>>>>>>> git://git.kiszka.org/qemu.git queues/slirp
>>>>>>>
>>>>>>> Jan Kiszka (1):
>>>>>>> slirp: Fix bit field types in IP header structs
>>>>>>>
>>>>>>> slirp/ip.h | 8 ++++----
>>>>>>> slirp/tcp.h | 4 ++--
>>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> slirp: Fix bit field types in IP header structs
>>>>>>>
>>>>>>> -mms-bitfields prevents that the bitfields in current IP header structs
>>>>>>> are packed into a single byte as it is required. Fix this by using
>>>>>>> uint8_t as backing type.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>> slirp/ip.h | 8 ++++----
>>>>>>> slirp/tcp.h | 4 ++--
>>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/slirp/ip.h b/slirp/ip.h
>>>>>>> index 48ea38e..72dbe9a 100644
>>>>>>> --- a/slirp/ip.h
>>>>>>> +++ b/slirp/ip.h
>>>>>>> @@ -74,10 +74,10 @@ typedef uint32_t n_long; /* long as received from the
>>>>>>> net */
>>>>>>> */
>>>>>>> struct ip {
>>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>>> - u_int ip_v:4, /* version */
>>>>>>> + uint8_t ip_v:4, /* version */
>>>>>>> ip_hl:4; /* header length */
>>>>>>> #else
>>>>>>> - u_int ip_hl:4, /* header length */
>>>>>>> + uint8_t ip_hl:4, /* header length */
>>>>>>> ip_v:4; /* version */
>>>>>>> #endif
>>>>>>> uint8_t ip_tos; /* type of service */
>>>>>>> @@ -140,10 +140,10 @@ struct ip_timestamp {
>>>>>>> uint8_t ipt_len; /* size of structure (variable) */
>>>>>>> uint8_t ipt_ptr; /* index of current entry */
>>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>>> - u_int ipt_oflw:4, /* overflow counter */
>>>>>>> + uint8_t ipt_oflw:4, /* overflow counter */
>>>>>>> ipt_flg:4; /* flags, see below */
>>>>>>> #else
>>>>>>> - u_int ipt_flg:4, /* flags, see below */
>>>>>>> + uint8_t ipt_flg:4, /* flags, see below */
>>>>>>> ipt_oflw:4; /* overflow counter */
>>>>>>> #endif
>>>>>>> union ipt_timestamp {
>>>>>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>>>>>> index 9d06836..b3817cb 100644
>>>>>>> --- a/slirp/tcp.h
>>>>>>> +++ b/slirp/tcp.h
>>>>>>> @@ -51,10 +51,10 @@ struct tcphdr {
>>>>>>> tcp_seq th_seq; /* sequence number */
>>>>>>> tcp_seq th_ack; /* acknowledgement number */
>>>>>>> #ifdef HOST_WORDS_BIGENDIAN
>>>>>>> - u_int th_off:4, /* data offset */
>>>>>>> + uint8_t th_off:4, /* data offset */
>>>>>>> th_x2:4; /* (unused) */
>>>>>>> #else
>>>>>>> - u_int th_x2:4, /* (unused) */
>>>>>>> + uint8_t th_x2:4, /* (unused) */
>>>>>>> th_off:4; /* data offset */
>>>>>>> #endif
>>>>>>> uint8_t th_flags;
>>>>>>
>>>>>> Tested-by: Stefan Weil <weil@mail.berlios.de>
>>>>>>
>>>>> slirp is still broken on my mingw32. I used "#progma
>>>>> pack(push,1)/#progma pack(pop)" to resolve this issue.
>>>>
>>>> Can you drill down to the bottom of this problem? What fields in what
>>>> struct are not properly packed? Maybe this is now a compiler bug, so
>>>> comparing versions may make sense as well.
>>> arphdr.ar_sip is not aligned on  a 4-byte boundary. See my previous post:
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html
>>
>> What a mess. Do we really have to go through all 257 packed data structs
>> in QEMU and add these MS compat bits to all potentially affected ones?
> I prefer to detect -mms-bitfields and remove it in configure.

/me too - if that is possible, ie. if the glib bits we are using doesn't
require us to apply that mode. Can anyone comment on this?

Jan
Avi Kivity Aug. 25, 2011, 1:02 p.m. UTC | #9
On 08/25/2011 03:38 PM, Jan Kiszka wrote:
> >>
> >>  What a mess. Do we really have to go through all 257 packed data structs
> >>  in QEMU and add these MS compat bits to all potentially affected ones?
> >  I prefer to detect -mms-bitfields and remove it in configure.

Can use -mno-ms-bitfields later to override it.

> /me too - if that is possible, ie. if the glib bits we are using doesn't
> require us to apply that mode. Can anyone comment on this?
>

I'd shy away from that.  Even if it works now it may break later.

We should simply avoid bitfields on externally-defined formats; 
meanwhile we can use __attribute__((gcc_struct)) and keep using those we 
have already.
Anthony Liguori Aug. 25, 2011, 1:06 p.m. UTC | #10
On 08/25/2011 07:38 AM, Jan Kiszka wrote:
> On 2011-08-25 14:02, TeLeMan wrote:
>> On Thu, Aug 25, 2011 at 19:04, Jan Kiszka<jan.kiszka@web.de>  wrote:
>>> What a mess. Do we really have to go through all 257 packed data structs
>>> in QEMU and add these MS compat bits to all potentially affected ones?
>> I prefer to detect -mms-bitfields and remove it in configure.
>
> /me too - if that is possible, ie. if the glib bits we are using doesn't
> require us to apply that mode. Can anyone comment on this?

So why can't we just #pragma guard all of the slirp bits?  Why are we 
doing it on a per data structure basis?

Regards,

Anthony Liguori

>
> Jan
>
Anthony Liguori Aug. 25, 2011, 1:07 p.m. UTC | #11
On 08/25/2011 08:02 AM, Avi Kivity wrote:
> On 08/25/2011 03:38 PM, Jan Kiszka wrote:
>> >>
>> >> What a mess. Do we really have to go through all 257 packed data
>> structs
>> >> in QEMU and add these MS compat bits to all potentially affected ones?
>> > I prefer to detect -mms-bitfields and remove it in configure.
>
> Can use -mno-ms-bitfields later to override it.

No, we can't do that.

The reason glib uses -mms-bitfields is that you need to use it in order 
to call Windows APIs which is does.  We will eventually need to do it 
anyway.

>
>> /me too - if that is possible, ie. if the glib bits we are using doesn't
>> require us to apply that mode. Can anyone comment on this?
>>
>
> I'd shy away from that. Even if it works now it may break later.
>
> We should simply avoid bitfields on externally-defined formats;
> meanwhile we can use __attribute__((gcc_struct)) and keep using those we
> have already.

+1

Regards,

Anthony Liguori


>
Jan Kiszka Aug. 25, 2011, 1:09 p.m. UTC | #12
On 2011-08-25 15:07, Anthony Liguori wrote:
> On 08/25/2011 08:02 AM, Avi Kivity wrote:
>> On 08/25/2011 03:38 PM, Jan Kiszka wrote:
>>> >>
>>> >> What a mess. Do we really have to go through all 257 packed data
>>> structs
>>> >> in QEMU and add these MS compat bits to all potentially affected
>>> ones?
>>> > I prefer to detect -mms-bitfields and remove it in configure.
>>
>> Can use -mno-ms-bitfields later to override it.
> 
> No, we can't do that.
> 
> The reason glib uses -mms-bitfields is that you need to use it in order
> to call Windows APIs which is does.  We will eventually need to do it
> anyway.
> 
>>
>>> /me too - if that is possible, ie. if the glib bits we are using doesn't
>>> require us to apply that mode. Can anyone comment on this?
>>>
>>
>> I'd shy away from that. Even if it works now it may break later.
>>
>> We should simply avoid bitfields on externally-defined formats;
>> meanwhile we can use __attribute__((gcc_struct)) and keep using those we
>> have already.
> 
> +1

We aren't discussing bitfields anymore but essential unaligned (and
therefore packed) protocol structs.

And I'm not that much worried about going through the few slirp structs
but all the other 250 packed ones in entire QEMU.

Jan
Jan Kiszka Aug. 25, 2011, 1:13 p.m. UTC | #13
On 2011-08-25 15:06, Anthony Liguori wrote:
> On 08/25/2011 07:38 AM, Jan Kiszka wrote:
>> On 2011-08-25 14:02, TeLeMan wrote:
>>> On Thu, Aug 25, 2011 at 19:04, Jan Kiszka<jan.kiszka@web.de>  wrote:
>>>> What a mess. Do we really have to go through all 257 packed data
>>>> structs
>>>> in QEMU and add these MS compat bits to all potentially affected ones?
>>> I prefer to detect -mms-bitfields and remove it in configure.
>>
>> /me too - if that is possible, ie. if the glib bits we are using doesn't
>> require us to apply that mode. Can anyone comment on this?
> 
> So why can't we just #pragma guard all of the slirp bits?  Why are we
> doing it on a per data structure basis?

Packing all structs is not really a good idea, more a last resort.

Jan
Avi Kivity Aug. 25, 2011, 1:15 p.m. UTC | #14
On 08/25/2011 04:07 PM, Anthony Liguori wrote:
> On 08/25/2011 08:02 AM, Avi Kivity wrote:
>> On 08/25/2011 03:38 PM, Jan Kiszka wrote:
>>> >>
>>> >> What a mess. Do we really have to go through all 257 packed data
>>> structs
>>> >> in QEMU and add these MS compat bits to all potentially affected 
>>> ones?
>>> > I prefer to detect -mms-bitfields and remove it in configure.
>>
>> Can use -mno-ms-bitfields later to override it.
>
> No, we can't do that.
>
> The reason glib uses -mms-bitfields is that you need to use it in 
> order to call Windows APIs which is does.  We will eventually need to 
> do it anyway.

I meant, just for our own objects.  As long as there are no glib APIs 
which use bitfields, it should work.

However, I don't like it either, and prefer the 
__attribute__(((((((gcc_fields)))))) as well.
Avi Kivity Aug. 25, 2011, 1:17 p.m. UTC | #15
On 08/25/2011 04:09 PM, Jan Kiszka wrote:
> >>
> >>  We should simply avoid bitfields on externally-defined formats;
> >>  meanwhile we can use __attribute__((gcc_struct)) and keep using those we
> >>  have already.
> >
> >  +1
>
> We aren't discussing bitfields anymore but essential unaligned (and
> therefore packed) protocol structs.
>
> And I'm not that much worried about going through the few slirp structs
> but all the other 250 packed ones in entire QEMU.
>

AFICT gcc_struct is also about alignment, so it should just_work.
Jan Kiszka Aug. 25, 2011, 1:19 p.m. UTC | #16
On 2011-08-25 15:15, Avi Kivity wrote:
> On 08/25/2011 04:07 PM, Anthony Liguori wrote:
>> On 08/25/2011 08:02 AM, Avi Kivity wrote:
>>> On 08/25/2011 03:38 PM, Jan Kiszka wrote:
>>>> >>
>>>> >> What a mess. Do we really have to go through all 257 packed data
>>>> structs
>>>> >> in QEMU and add these MS compat bits to all potentially affected
>>>> ones?
>>>> > I prefer to detect -mms-bitfields and remove it in configure.
>>>
>>> Can use -mno-ms-bitfields later to override it.
>>
>> No, we can't do that.
>>
>> The reason glib uses -mms-bitfields is that you need to use it in
>> order to call Windows APIs which is does.  We will eventually need to
>> do it anyway.
> 
> I meant, just for our own objects.  As long as there are no glib APIs
> which use bitfields, it should work.
> 
> However, I don't like it either, and prefer the
> __attribute__(((((((gcc_fields)))))) as well.

Could someone with a Windows environment test if that (or (packed,
gcc_fields)?) makes

struct {
	unsigned char a;
	unsigned int b;
};

truly packed again?

Jan
Avi Kivity Aug. 25, 2011, 1:21 p.m. UTC | #17
On 08/25/2011 04:19 PM, Jan Kiszka wrote:
> >
> >  However, I don't like it either, and prefer the
> >  __attribute__(((((((gcc_fields)))))) as well.
>
> Could someone with a Windows environment test if that (or (packed,
> gcc_fields)?) makes
>
> struct {
> 	unsigned char a;
> 	unsigned int b;
> };
>
> truly packed again?
>

You'll need both attributes IIUC.
Avi Kivity Aug. 25, 2011, 1:22 p.m. UTC | #18
On 08/25/2011 04:13 PM, Jan Kiszka wrote:
> >
> >  So why can't we just #pragma guard all of the slirp bits?  Why are we
> >  doing it on a per data structure basis?
>
> Packing all structs is not really a good idea, more a last resort.
>

btw, how are the non-x86s handling this? by trapping and fixuping in the 
kernel?
Anthony Liguori Aug. 25, 2011, 1:28 p.m. UTC | #19
On 08/25/2011 08:13 AM, Jan Kiszka wrote:
> On 2011-08-25 15:06, Anthony Liguori wrote:
>> On 08/25/2011 07:38 AM, Jan Kiszka wrote:
>>> On 2011-08-25 14:02, TeLeMan wrote:
>>>> On Thu, Aug 25, 2011 at 19:04, Jan Kiszka<jan.kiszka@web.de>   wrote:
>>>>> What a mess. Do we really have to go through all 257 packed data
>>>>> structs
>>>>> in QEMU and add these MS compat bits to all potentially affected ones?
>>>> I prefer to detect -mms-bitfields and remove it in configure.
>>>
>>> /me too - if that is possible, ie. if the glib bits we are using doesn't
>>> require us to apply that mode. Can anyone comment on this?
>>
>> So why can't we just #pragma guard all of the slirp bits?  Why are we
>> doing it on a per data structure basis?
>
> Packing all structs is not really a good idea, more a last resort.

It doesn't force packing, it forces GCC style structure layout.

Regards,

Anthony Liguori

> Jan
>
Jan Kiszka Aug. 25, 2011, 1:32 p.m. UTC | #20
On 2011-08-25 15:28, Anthony Liguori wrote:
> On 08/25/2011 08:13 AM, Jan Kiszka wrote:
>> On 2011-08-25 15:06, Anthony Liguori wrote:
>>> On 08/25/2011 07:38 AM, Jan Kiszka wrote:
>>>> On 2011-08-25 14:02, TeLeMan wrote:
>>>>> On Thu, Aug 25, 2011 at 19:04, Jan Kiszka<jan.kiszka@web.de>   wrote:
>>>>>> What a mess. Do we really have to go through all 257 packed data
>>>>>> structs
>>>>>> in QEMU and add these MS compat bits to all potentially affected
>>>>>> ones?
>>>>> I prefer to detect -mms-bitfields and remove it in configure.
>>>>
>>>> /me too - if that is possible, ie. if the glib bits we are using
>>>> doesn't
>>>> require us to apply that mode. Can anyone comment on this?
>>>
>>> So why can't we just #pragma guard all of the slirp bits?  Why are we
>>> doing it on a per data structure basis?
>>
>> Packing all structs is not really a good idea, more a last resort.
> 
> It doesn't force packing, it forces GCC style structure layout.

If we are talking about #pragma pack(...), then that's not what I read
in the docs.

Jan
Peter Maydell Aug. 25, 2011, 1:36 p.m. UTC | #21
On 25 August 2011 14:22, Avi Kivity <avi@redhat.com> wrote:
> On 08/25/2011 04:13 PM, Jan Kiszka wrote:
>> Packing all structs is not really a good idea, more a last resort.
>
> btw, how are the non-x86s handling this? by trapping and fixuping in the
> kernel?

If a structure's packing means it doesn't adhere to the relevant
alignment constraints for the architecture gcc will access the
misaligned integers with a sequence of byte loads and stores.

-- PMM
Avi Kivity Aug. 25, 2011, 1:39 p.m. UTC | #22
On 08/25/2011 04:36 PM, Peter Maydell wrote:
> On 25 August 2011 14:22, Avi Kivity<avi@redhat.com>  wrote:
> >  On 08/25/2011 04:13 PM, Jan Kiszka wrote:
> >>  Packing all structs is not really a good idea, more a last resort.
> >
> >  btw, how are the non-x86s handling this? by trapping and fixuping in the
> >  kernel?
>
> If a structure's packing means it doesn't adhere to the relevant
> alignment constraints for the architecture gcc will access the
> misaligned integers with a sequence of byte loads and stores.

Thanks, good to know.
Stefan Weil Aug. 25, 2011, 2:03 p.m. UTC | #23
As I wrote in my last mail, I compared all structs without and with 
-mms-bitfields
using pahole and codiff. The result is in the appended codiff.log.

About 17 structs changed because of -mms-bitfield. This attribute 
modifies not only
structs with bitfield but also packed structs or structs with other 
attributes
like TCGPool.

Jan's patch fixed slirp bitfields. For the remaining cases, I also 
thought about
removing -mms-bitfield or setting a pragma in qemu-common.h, but now I
prefer a different solution: replace all __attribute__(packed), 
__attribute(__packed__)
by QEMU_PACKED.

QEMU_PACKED will be a macro defined in compiler.h which sets the attributes
needed (also for w32 with -mms-bitfields).

Maybe I can send patches with the changes needed next weekend if 
everybody agrees
to this solution.

Cheers,
Stefan
Anthony Liguori Aug. 25, 2011, 2:23 p.m. UTC | #24
On 08/25/2011 09:03 AM, Stefan Weil wrote:
> As I wrote in my last mail, I compared all structs without and with
> -mms-bitfields
> using pahole and codiff. The result is in the appended codiff.log.
>
> About 17 structs changed because of -mms-bitfield. This attribute
> modifies not only
> structs with bitfield but also packed structs or structs with other
> attributes
> like TCGPool.
>
> Jan's patch fixed slirp bitfields. For the remaining cases, I also
> thought about
> removing -mms-bitfield or setting a pragma in qemu-common.h, but now I
> prefer a different solution: replace all __attribute__(packed),
> __attribute(__packed__)
> by QEMU_PACKED.
>
> QEMU_PACKED will be a macro defined in compiler.h which sets the attributes
> needed (also for w32 with -mms-bitfields).

That sounds like a good idea.

Regards,

Anthony Liguori

>
> Maybe I can send patches with the changes needed next weekend if
> everybody agrees
> to this solution.
>
> Cheers,
> Stefan
>
Jan Kiszka Aug. 25, 2011, 5:15 p.m. UTC | #25
On 2011-08-25 16:03, Stefan Weil wrote:
> As I wrote in my last mail, I compared all structs without and with
> -mms-bitfields
> using pahole and codiff. The result is in the appended codiff.log.
> 
> About 17 structs changed because of -mms-bitfield. This attribute
> modifies not only
> structs with bitfield but also packed structs or structs with other
> attributes
> like TCGPool.
> 
> Jan's patch fixed slirp bitfields. For the remaining cases, I also
> thought about
> removing -mms-bitfield or setting a pragma in qemu-common.h, but now I
> prefer a different solution: replace all __attribute__(packed),
> __attribute(__packed__)
> by QEMU_PACKED.
> 
> QEMU_PACKED will be a macro defined in compiler.h which sets the attributes
> needed (also for w32 with -mms-bitfields).
> 
> Maybe I can send patches with the changes needed next weekend if
> everybody agrees
> to this solution.

Yes, sounds good, specifically as setting gcc_struct in place won't work
either due to the fact it's i386-only.

Jan
diff mbox

Patch

diff --git a/slirp/ip.h b/slirp/ip.h
index 48ea38e..72dbe9a 100644
--- a/slirp/ip.h
+++ b/slirp/ip.h
@@ -74,10 +74,10 @@  typedef uint32_t n_long;                 /* long as received from the net */
  */
 struct ip {
 #ifdef HOST_WORDS_BIGENDIAN
-	u_int ip_v:4,			/* version */
+	uint8_t ip_v:4,			/* version */
 		ip_hl:4;		/* header length */
 #else
-	u_int ip_hl:4,		/* header length */
+	uint8_t ip_hl:4,		/* header length */
 		ip_v:4;			/* version */
 #endif
 	uint8_t		ip_tos;			/* type of service */
@@ -140,10 +140,10 @@  struct	ip_timestamp {
 	uint8_t	ipt_len;		/* size of structure (variable) */
 	uint8_t	ipt_ptr;		/* index of current entry */
 #ifdef HOST_WORDS_BIGENDIAN
-	u_int	ipt_oflw:4,		/* overflow counter */
+	uint8_t	ipt_oflw:4,		/* overflow counter */
 		ipt_flg:4;		/* flags, see below */
 #else
-	u_int	ipt_flg:4,		/* flags, see below */
+	uint8_t	ipt_flg:4,		/* flags, see below */
 		ipt_oflw:4;		/* overflow counter */
 #endif
 	union ipt_timestamp {
diff --git a/slirp/tcp.h b/slirp/tcp.h
index 9d06836..b3817cb 100644
--- a/slirp/tcp.h
+++ b/slirp/tcp.h
@@ -51,10 +51,10 @@  struct tcphdr {
 	tcp_seq	th_seq;			/* sequence number */
 	tcp_seq	th_ack;			/* acknowledgement number */
 #ifdef HOST_WORDS_BIGENDIAN
-	u_int	th_off:4,		/* data offset */
+	uint8_t	th_off:4,		/* data offset */
 		th_x2:4;		/* (unused) */
 #else
-	u_int	th_x2:4,		/* (unused) */
+	uint8_t	th_x2:4,		/* (unused) */
 		th_off:4;		/* data offset */
 #endif
 	uint8_t th_flags;