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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

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;



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Stefan Weil]

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.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[TeLeMan]

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.
>
>
>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[TeLeMan]

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
>
>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[TeLeMan]

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.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4168 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Anthony Liguori]

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


>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Anthony Liguori]

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
>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Peter Maydell]

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

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Anthony Liguori]

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
>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

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


[-- Attachment #2: codiff.log --]
[-- Type: text/x-log, Size: 5047 bytes --]

/home/stefan/src/qemu/savannah/qemu/hw/virtio-balloon.c:
  struct VirtIOBalloonStat     |   +6
 1 struct changed
  virtio_balloon_receive_stats |   -1
 1 function changed, 1 bytes removed

/home/stefan/src/qemu/savannah/qemu/block/vmdk.c:
  vmdk_open_vmdk4    |   +9
  vmdk_create_extent |   +6
 2 functions changed, 15 bytes added

/home/stefan/src/qemu/savannah/qemu/block/vvfat.c:
  struct bootsector_t |   +5
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/hw/bt-hci.c:
  bt_hci_connection_accept              |   -2
  bt_hci_lmp_connection_complete        |   -2
  bt_hci_event_read_remote_ext_features |   +2
  bt_hci_event_complete_lmp_handle      |   +7
 4 functions changed, 9 bytes added, 4 bytes removed, diff: +5

/home/stefan/src/qemu/savannah/qemu/slirp/cksum.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/if.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/ip_icmp.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/ip_input.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/ip_output.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/slirp.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/mbuf.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/misc.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/sbuf.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/socket.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/tcp_input.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/tcp_output.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/tcp_subr.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/tcp_timer.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/udp.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/bootp.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/tftp.c:
  struct Slirp    |  +64
  struct arphdr   |   +4
  struct ArpTable |  +64
 3 structs changed

/home/stefan/src/qemu/savannah/qemu/slirp/arp_table.c:
  struct Slirp     |  +64
  struct arphdr    |   +4
  struct ArpTable  |  +64
 3 structs changed
  arp_table_add    |  -49
  arp_table_search |  -24
 2 functions changed, 73 bytes removed

/home/stefan/src/qemu/savannah/qemu/exec.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/translate-all.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/cpu-exec.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/target-i386/translate.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/tcg/tcg.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/tcg/optimize.c:
  struct TCGPool |   +0
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/target-i386/op_helper.c:
  helper_vmexit |   +2
 1 function changed, 2 bytes added

/home/stefan/src/qemu/savannah/qemu/hw/acpi.c:
  struct acpi_table_header |   +2
 1 struct changed

/home/stefan/src/qemu/savannah/qemu/hw/hpet.c:
  struct hpet_fw_entry  |   +4
  struct hpet_fw_config |  +32
 2 structs changed
  hpet_post_load        |   +4
  hpet_reset            |   +8
 2 functions changed, 12 bytes added

/home/stefan/src/qemu/savannah/qemu/hw/usb-ccid.c:
  struct CCID_Header        |   +3
  struct CCID_BULK_IN       |   +3
  struct CCID_SlotStatus    |   +3
  struct CCID_Parameter     |   +3
  struct CCID_DataBlock     |   +3
  struct CCID_XferBlock     |   +4
  struct CCID_IccPowerOn    |   +4
  struct CCID_SetParameters |   +4
 8 structs changed

/home/stefan/src/qemu/savannah/qemu/hw/pc.c:
  struct hpet_fw_entry  |   +4
  struct hpet_fw_config |  +32
 2 structs changed

/tmp/qemu.exe:
 12 functions changed, 38 bytes added, 78 bytes removed, diff: -40

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Anthony Liguori]

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
>

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PULL] slirp: Fix issues with -mms-bitfields

[Blue Swirl]

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;
>
>
>