[U-Boot] IP_t should be a "packed" struct

[U-Boot] IP_t should be a "packed" struct

[Luigi 'Comio' Mantellini]

Hi ML,

I'm working on a mips target and I used qemu_mips target to simulate my target 
(that I hope to have in the next week...)

Following my activities I noticed that IP_t structure is no defined with 
attribute "packed". I noticed this issue because using a self-made toolchain 
(gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 
32bit boundary. This is not ok, because the packets IP_t can be non aligned 
(see the /net/net.c PingSend function, for an example).

The dirty solution is to define the structure with the 
__attribute__((__packed__))... but, from my point of view, a better packet 
forging mechanism should be implemented into the net.c stack.

I attached a trivial patch that solved the issue on my target.

Any comments is welcome.

best regards,

luigi

-- 
Luigi Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.:  +39 02 5167 2813
Fax:   +39 02 5167 2459
Email: luigi.mantellini at idf-hit.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_net_h.patch
Type: text/x-patch
Size: 419 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090127/68310d16/attachment.bin 

[U-Boot] IP_t should be a "packed" struct

[Luigi 'Comio' Mantellini]

Hi ML,

I noticed that also the others headers structs have the same potential 
problem.
I attached the patch to fix the net.h header file.

best regards,

luigi

On Tuesday 27 January 2009 19:32:10 Luigi 'Comio' Mantellini wrote:
> Hi ML,
>
> I'm working on a mips target and I used qemu_mips target to simulate my
> target (that I hope to have in the next week...)
>
> Following my activities I noticed that IP_t structure is no defined with
> attribute "packed". I noticed this issue because using a self-made
> toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all
> bytes to 32bit boundary. This is not ok, because the packets IP_t can be
> non aligned (see the /net/net.c PingSend function, for an example).
>
> The dirty solution is to define the structure with the
> __attribute__((__packed__))... but, from my point of view, a better packet
> forging mechanism should be implemented into the net.c stack.
>
> I attached a trivial patch that solved the issue on my target.
>
> Any comments is welcome.
>
> best regards,
>
> luigi

-- 
Luigi Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.:  +39 02 5167 2813
Fax:   +39 02 5167 2459
Email: luigi.mantellini at idf-hit.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_net_h_patch
Type: text/x-patch
Size: 1402 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090128/008175cd/attachment.bin 

[U-Boot] IP_t should be a "packed" struct

[Ben Warren]

Luigi 'Comio' Mantellini wrote:
> Hi ML,
>
> I'm working on a mips target and I used qemu_mips target to simulate my target 
> (that I hope to have in the next week...)
>
> Following my activities I noticed that IP_t structure is no defined with 
> attribute "packed". I noticed this issue because using a self-made toolchain 
> (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 
> 32bit boundary. This is not ok, because the packets IP_t can be non aligned 
> (see the /net/net.c PingSend function, for an example).
>
>   
Why is your compiler aligning all bytes to 32-bit boundary?  Seems like 
an awful waste of space.  This struct should pack itself nicely, and 
does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and 
gcc 4.0.0 ppc_4xx).
> The dirty solution is to define the structure with the 
> __attribute__((__packed__))... but, from my point of view, a better packet 
> forging mechanism should be implemented into the net.c stack.
>
> I attached a trivial patch that solved the issue on my target.
>
> Any comments is welcome.
>
> best regards,
>
> luigi
>
>  
I'd focus on fixing your toolchain.  Your problem will not be confined 
to protocol headers.
>  
> ------------------------------------------------------------------------
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
regards,
Ben

[U-Boot] IP_t should be a "packed" struct

[Jerry Van Baren]

Ben Warren wrote:
> Luigi 'Comio' Mantellini wrote:
>> Hi ML,
>>
>> I'm working on a mips target and I used qemu_mips target to simulate my target 
>> (that I hope to have in the next week...)
>>
>> Following my activities I noticed that IP_t structure is no defined with 
>> attribute "packed". I noticed this issue because using a self-made toolchain 
>> (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 
>> 32bit boundary. This is not ok, because the packets IP_t can be non aligned 
>> (see the /net/net.c PingSend function, for an example).
>>
>>   
> Why is your compiler aligning all bytes to 32-bit boundary?  Seems like 
> an awful waste of space.  This struct should pack itself nicely, and 
> does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and 
> gcc 4.0.0 ppc_4xx).

The compiler is optimizing for speed and/or execution size at the 
expense of larger data structures either by command (e.g. a -O option) 
or as part of the compiler writer's choice.  CPUs almost always execute 
code significantly faster when the data is properly aligned.  Many CPUs 
require software to deal with the misalignment which costs code space 
and execution time.

Since the compiler wasn't instructed that the IP headers needed to be 
packed, it is within the compiler's right to not pack them.

[snip]

Best regards,
gvb

[U-Boot] IP_t should be a "packed" struct

[Ben Warren]

Jerry Van Baren wrote:
> Ben Warren wrote:
>> Luigi 'Comio' Mantellini wrote:
>>> Hi ML,
>>>
>>> I'm working on a mips target and I used qemu_mips target to simulate 
>>> my target (that I hope to have in the next week...)
>>>
>>> Following my activities I noticed that IP_t structure is no defined 
>>> with attribute "packed". I noticed this issue because using a 
>>> self-made toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler 
>>> has aligned all bytes to 32bit boundary. This is not ok, because the 
>>> packets IP_t can be non aligned (see the /net/net.c PingSend 
>>> function, for an example).
>>>
>>>   
>> Why is your compiler aligning all bytes to 32-bit boundary?  Seems 
>> like an awful waste of space.  This struct should pack itself nicely, 
>> and does on the small sample of toolchains I've tried (gcc 4.3.2 
>> x86_64 and gcc 4.0.0 ppc_4xx).
>
> The compiler is optimizing for speed and/or execution size at the 
> expense of larger data structures either by command (e.g. a -O option) 
> or as part of the compiler writer's choice.  CPUs almost always 
> execute code significantly faster when the data is properly aligned.  
> Many CPUs require software to deal with the misalignment which costs 
> code space and execution time.
>
> Since the compiler wasn't instructed that the IP headers needed to be 
> packed, it is within the compiler's right to not pack them.
>
Sure.  In this case, though, the optimization's not necessarily going to 
gain anything since we never use a raw struct IP_t, only pointers that 
overlay other char arrays through casting.   Of course there's no point 
discussing this much further here, but I suspect that this packing 
problem will exist in many more places than the protocol headers.
> [snip]
>
> Best regards,
> gvb
>
regards,
Ben

[U-Boot] IP_t should be a "packed" struct

[Luigi Mantellini]

Dear All

[U-Boot] IP_t should be a "packed" struct

[Luigi Mantellini]

2009/1/28 Ben Warren <biggerbadderben@gmail.com>:
..

> I'd focus on fixing your toolchain.  Your problem will not be confined to
> protocol headers.


my toolchain works fine ;)


-- 
Luigi 'Comio' Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy

Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantellini at idf-hit.com

[U-Boot] IP_t should be a "packed" struct

[Ben Warren]

Luigi Mantellini wrote:
> Dear All
>
> From my point of view, when packing is formally required (ie packets
> headers), the structs should be declared explicitly as __packed__. The
> correctness of the object code should be independent from the compiler
> optimizations and we should always remember that the offset of a
> struct field is not necessarily the sum of the sizes of the fields
> that precede it.
>
> struct {
>  type1 a;
>  type2 b;
>  type3 c;
> } mystruct
>
>
> offset(mystruct.c)  !=  sizeof(type1) + sizeof(type2).
>
> Regarding the CFLAGS used by me... I haven't set any CFLAGS and I just
> do a make qemu_mips_config CROSS_COMPILE=mips-linux- && make
> CROSS_COMPILE=mips-linux... and a boundary alignment is not an alien
> choice for a good compiler (a standard gcc4.2.4 in my case).
> Furthermore I expect a correct object always... with any -Ox flag (a
> apart bugs... of course).
>
> My idea should be to declare a define like this
>
> #define PKT_HEADER __attribute__((__packed__))
>
> my 2EuroCents.
>
> best regards,
>
> luigi
>
>   
OK, sounds good.  Send a patch please.

regards,
Ben
<snip>

[U-Boot] IP_t should be a "packed" struct

[Wolfgang Denk]

Dear Luigi Mantellini,

In message <b73e93990901281258s37f7e4d6hc6c2ff4c5d5fa47b@mail.gmail.com> you wrote:
> 
> > I'd focus on fixing your toolchain.  Your problem will not be confined to
> > protocol headers.
> 
> my toolchain works fine ;)

Except that it adds padding where it doesn't make sense. I think you
want to fix it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't tell me how hard you work.  Tell me how much you get done.
                                                     -- James J. Ling

[U-Boot] IP_t should be a "packed" struct

[Wolfgang Denk]

Dear Ben Warren,

In message <4980CC59.1070302@gmail.com> you wrote:
>
> > My idea should be to declare a define like this
> >
> > #define PKT_HEADER __attribute__((__packed__))
> >
> > my 2EuroCents.
> >
> > best regards,
> >
> > luigi
> >
> >   
> OK, sounds good.  Send a patch please.

Hm... and what does this give us?

How long  until  sombebody  detects  that  all  the  data  structures
describing  device  register  layout  or so many processors and chips
don't use any __packed__ either? Except for some (IMHO broken, but  I
know  that  I'm  more  attached  to PowerPC anyway) ARM systems it is
sufficient to use a "sane" selection of data types.


I vote against changing this code. Just for a quick  cross-check:  do
the   corresponding   data  structs  in  the  Linux  kernel  use  any
__packed__?

Here is for example a copy of /usr/include/netinet/ip.h :

...
struct iphdr
  {
#if __BYTE_ORDER == __LITTLE_ENDIAN
    unsigned int ihl:4;
    unsigned int version:4;
#elif __BYTE_ORDER == __BIG_ENDIAN
    unsigned int version:4;
    unsigned int ihl:4;
#else
# error	"Please fix <bits/endian.h>"
#endif
    u_int8_t tos;
    u_int16_t tot_len;
    u_int16_t id;
    u_int16_t frag_off;
    u_int8_t ttl;
    u_int8_t protocol;
    u_int16_t check;
    u_int32_t saddr;
    u_int32_t daddr;
    /*The options start here. */
  };
...

Do you see any __packed__?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The C-shell doesn't parse. It adhoculates.
    - Casper.Dik at Holland.Sun.COM in <3ol96k$b2j@engnews2.Eng.Sun.COM>

[U-Boot] IP_t should be a "packed" struct

[Ben Warren]

Wolfgang Denk wrote:
> Dear Ben Warren,
>
> In message <4980CC59.1070302@gmail.com> you wrote:
>   
>>> My idea should be to declare a define like this
>>>
>>> #define PKT_HEADER __attribute__((__packed__))
>>>
>>> my 2EuroCents.
>>>
>>> best regards,
>>>
>>> luigi
>>>
>>>   
>>>       
>> OK, sounds good.  Send a patch please.
>>     
>
> Hm... and what does this give us?
>
> How long  until  sombebody  detects  that  all  the  data  structures
> describing  device  register  layout  or so many processors and chips
> don't use any __packed__ either? Except for some (IMHO broken, but  I
> know  that  I'm  more  attached  to PowerPC anyway) ARM systems it is
> sufficient to use a "sane" selection of data types.
>
>
>   
Well, yeah, I alluded to that in my other e-mail.  I'm not crazy about 
this either, but don't see any significant downside.  Obviously if this 
was a common problem, it would have surfaced a long time ago...
> I vote against changing this code. Just for a quick  cross-check:  do
> the   corresponding   data  structs  in  the  Linux  kernel  use  any
> __packed__?
>
> Here is for example a copy of /usr/include/netinet/ip.h :
>
> ...
> struct iphdr
>   {
> #if __BYTE_ORDER == __LITTLE_ENDIAN
>     unsigned int ihl:4;
>     unsigned int version:4;
> #elif __BYTE_ORDER == __BIG_ENDIAN
>     unsigned int version:4;
>     unsigned int ihl:4;
> #else
> # error	"Please fix <bits/endian.h>"
> #endif
>     u_int8_t tos;
>     u_int16_t tot_len;
>     u_int16_t id;
>     u_int16_t frag_off;
>     u_int8_t ttl;
>     u_int8_t protocol;
>     u_int16_t check;
>     u_int32_t saddr;
>     u_int32_t daddr;
>     /*The options start here. */
>   };
> ...
>
> Do you see any __packed__?
>
>   
Yeah, I made the same observation, but am not fluent enough in the black 
magic of Linux header files to know if packing was being enforced 
somewhere else or in some other way that my little brain can't comprehend.
> Best regards,
>
> Wolfgang Denk
>
>   
regards,
Ben

[U-Boot] IP_t should be a "packed" struct

[Luigi Mantellini]

Dear All,

2009/1/28 Wolfgang Denk <wd@denx.de>:
> Dear Ben Warren,
>
> In message <4980CC59.1070302@gmail.com> you wrote:
>>
>> > My idea should be to declare a define like this
>> >
>> > #define PKT_HEADER __attribute__((__packed__))
>> >
>> > my 2EuroCents.
>> >
>> > best regards,
>> >
>> > luigi
>> >
>> >
>> OK, sounds good.  Send a patch please.

I think that an audit of the code is important to understand if we
have a problem (or not) and how large is the problem.

>
> Hm... and what does this give us?
>
> How long  until  sombebody  detects  that  all  the  data  structures
> describing  device  register  layout  or so many processors and chips
> don't use any __packed__ either? Except for some (IMHO broken, but  I
> know  that  I'm  more  attached  to PowerPC anyway) ARM systems it is
> sufficient to use a "sane" selection of data types.
>

My compiler is not broken...

>
> I vote against changing this code. Just for a quick  cross-check:  do
> the   corresponding   data  structs  in  the  Linux  kernel  use  any
> __packed__?
>

cassini linux # head Makefile  -n5
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 28
EXTRAVERSION = -tuxonice-r1
NAME = Erotic Pickled Herring
cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute |
grep packed | wc -l
3153

I see a lot of packed structs...


> Here is for example a copy of /usr/include/netinet/ip.h :
>
> ...
> struct iphdr
>  {
> #if __BYTE_ORDER == __LITTLE_ENDIAN
>    unsigned int ihl:4;
>    unsigned int version:4;
> #elif __BYTE_ORDER == __BIG_ENDIAN
>    unsigned int version:4;
>    unsigned int ihl:4;
> #else
> # error "Please fix <bits/endian.h>"
> #endif
>    u_int8_t tos;
>    u_int16_t tot_len;
>    u_int16_t id;
>    u_int16_t frag_off;
>    u_int8_t ttl;
>    u_int8_t protocol;
>    u_int16_t check;
>    u_int32_t saddr;
>    u_int32_t daddr;
>    /*The options start here. */
>  };
> ...
>
> Do you see any __packed__?

This doesn't say anything regarding how kernel guys have resolved the
problem (if they are solved...). Checking the kernel headers a lot
(all?) of structs that refers to drivers and internal structures are
defined using the attribute packed. For me this shows that somebody
asked himself if packed is needed or not.
I think that "struct packing" needs to be understood and a global
mechanism should be used (like -fpack-struct option always defined or
a style guideline that requires a tag for each structs). From my point
of view, we cannot define as wrong a compiler that follows different
choices that are anyway conform to the language standard.

My usual 2Eurocents.

best regards,

luigi

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> The C-shell doesn't parse. It adhoculates.
>    - Casper.Dik at Holland.Sun.COM in <3ol96k$b2j@engnews2.Eng.Sun.COM>
>



-- 
Luigi 'Comio' Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy

Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantellini at idf-hit.com

[U-Boot] IP_t should be a "packed" struct

[Wolfgang Denk]

Dear Ben,

In message <4980D38F.4020100@gmail.com> you wrote:
>
> > Here is for example a copy of /usr/include/netinet/ip.h :
...
> > struct iphdr
...
> Yeah, I made the same observation, but am not fluent enough in the black 
> magic of Linux header files to know if packing was being enforced 
> somewhere else or in some other way that my little brain can't comprehend.

Note that this was standard <netinet/ip.h>, i.  e.  no  Linux  kernel
header  file,  but  a standard user land header. Any application code
using network functions would break if this was a real problem.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The human race has one really effective weapon, and that is laughter.
                                                         - Mark Twain

[U-Boot] IP_t should be a "packed" struct

[Wolfgang Denk]

Dear Luigi Mantellini,

In message <b73e93990901281416k766b2bf3qc6429f77545f9191@mail.gmail.com> you wrote:
> 
> I think that an audit of the code is important to understand if we
> have a problem (or not) and how large is the problem.

We (i. e. all of us except you) do not have a problem.

> My compiler is not broken...

Well, YMMV...

> cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute |
> grep packed | wc -l
> 3153
> 
> I see a lot of packed structs...



> > Here is for example a copy of /usr/include/netinet/ip.h :
...
> This doesn't say anything regarding how kernel guys have resolved the
> problem (if they are solved...). Checking the kernel headers a lot

This is not a kernel header. This is one of the standard user space
network headers, and a pretty central one. Feel free to check any
others.

> I think that "struct packing" needs to be understood and a global

I agree on this. This definitely needs to be understood.

Hm... I think I understand it, and it works for me :-)

> mechanism should be used (like -fpack-struct option always defined or
> a style guideline that requires a tag for each structs). From my point

I think this is not needed. Please read the docuemntation about
alignment and padding. It is pretty clear.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"And it should be the law: If you use  the  word  `paradigm'  without
knowing  what  the  dictionary  says  it  means,  you  go to jail. No
exceptions."                     - David Jones @ Megatest Corporation

[U-Boot] IP_t should be a "packed" struct

[Luigi Mantellini]

Dear Wolfgang,

2009/1/28 Wolfgang Denk <wd@denx.de>:
> Dear Luigi Mantellini,
>
> In message <b73e93990901281416k766b2bf3qc6429f77545f9191@mail.gmail.com> you wrote:
>>
>> I think that an audit of the code is important to understand if we
>> have a problem (or not) and how large is the problem.
>
> We (i. e. all of us except you) do not have a problem.

my question is: how can you be sure on this? I haven't used strange
compilers or strange CFLAGS.. I just do "make" on a supported target
(qemu_mips).
I haven't any problem to correct my local source tree. I ask myself
why gcc offers a packed atribute, ms-vc offers pragma packed, ...

>
>> My compiler is not broken...
>
> Well, YMMV...
>

the behavior is clear: in my environment, the default choice is to
align fields on 32bit for speed reasons... and I like this by default
for my applications.
I can ignore the problem using options like -Os (I will try tomorrow)
or -fpack-struct or other global mechanisms or, pay attention on
structures definitions to be sure that the structure size is compiler
and cflags independent.

>> cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute |
>> grep packed | wc -l
>> 3153
>>
>> I see a lot of packed structs...
>
>
>
>> > Here is for example a copy of /usr/include/netinet/ip.h :
> ...
>> This doesn't say anything regarding how kernel guys have resolved the
>> problem (if they are solved...). Checking the kernel headers a lot
>
> This is not a kernel header. This is one of the standard user space
> network headers, and a pretty central one. Feel free to check any
> others.
>

I see. I would like to understand why this structure is
optimization-prof. I will study tomorrow...

>> I think that "struct packing" needs to be understood and a global
>
> I agree on this. This definitely needs to be understood.
>
> Hm... I think I understand it, and it works for me :-)
>

iso C doensn't require packing by default of the structures. To assume
that a structure is packed by default is not a good assumption. All
compilers offer directives to control the behavior of packing... I
believe that there is a reason...

>> mechanism should be used (like -fpack-struct option always defined or
>> a style guideline that requires a tag for each structs). From my point
>
> I think this is not needed. Please read the docuemntation about
> alignment and padding. It is pretty clear.

which documentation should I read? iso c documents? gcc manuals? I
haven't found anything that opposes my affirmations. I want underline
that the structures (IP_t, ... ) don't have field across the
word-machine boundary... but this doesn't exclude an "memory-access"
optimization.
Kindly, can you give me any good links?

Anyway,  I don't want to talk about philosophy. I just noticed an
anomaly and I wanted to share with the ML.

my 2LireItaliane

best regards,

luigi

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "And it should be the law: If you use  the  word  `paradigm'  without
> knowing  what  the  dictionary  says  it  means,  you  go to jail. No
> exceptions."                     - David Jones @ Megatest Corporation
>



-- 
Luigi 'Comio' Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy

Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantellini at idf-hit.com

[U-Boot] IP_t should be a "packed" struct

[Wolfgang Denk]

Dear Luigi Mantellini,

In message <b73e93990901281504k3940b356nbff298e137c0e707@mail.gmail.com> you wrote:
> 
> > We (i. e. all of us except you) do not have a problem.
> 
> my question is: how can you be sure on this? I haven't used strange

Because I read the documentation?

> compilers or strange CFLAGS.. I just do "make" on a supported target
> (qemu_mips).
> I haven't any problem to correct my local source tree. I ask myself
> why gcc offers a packed atribute, ms-vc offers pragma packed, ...

There are cases where it makes a difference. Assume someting like
this:

struct foo {	/* offset     normal	packed	*/
	long	l;	/*	0	    0	*/
	char	c;	/*	4	    4	*/
	long	x;	/*	8	    5	*/
};

Here the alignment requirements are 4 byte for "l", 1 byte  for  "c",
and  4  byte for "x". The compiler will align all variables on 4 byte
addresses normaly, i. e. the offsets are 0, 4 and 8. With  ""packed",
the  natural alignment of "x" on a 4 byte border will be ignored, and
instead it will be "packed" immediately following the storage for the
"c" variable, i. e. resulting in offset  5.  This  may  easily  cause
problems  on some architectures, but that's what you get when you ask
for it.

Now study the data structures used  for  netwrking  -  they  are  all
carefully  crafted that the natural aligment "just fits", i. e. there
are no gaps between variables if they are aligned naturally.

Your compiler must be doing really stupid things here.

> the behavior is clear: in my environment, the default choice is to
> align fields on 32bit for speed reasons... and I like this by default
> for my applications.

What makes you think it would be faster if a 16 bit ("short") data
type was no aligned on a 32 bit border, but only on a 16 bit one?
I am not aware of a system where that would make any speed difference. 

Same for 8 bit (char) data types - for these,  alignment  at  32  bit
offsets makes zero sense to me.

> I can ignore the problem using options like -Os (I will try tomorrow)
> or -fpack-struct or other global mechanisms or, pay attention on
> structures definitions to be sure that the structure size is compiler
> and cflags independent.

The currentcode has no such problems, standard conforming compilers
assumed.

> I see. I would like to understand why this structure is
> optimization-prof. I will study tomorrow...

Please read the docs.

> iso C doensn't require packing by default of the structures. To assume

We are not talking about "packing" here. We are  just  talking  about
alignment  and  insertion of gaps to ensure natural alignment of data
types. This is a well documented area.

> that a structure is packed by default is not a good assumption. All

This is NOT packing! Please do not mix unrelated terms.

> which documentation should I read? iso c documents? gcc manuals? I

ISO C or GCC, whatever is more handy.

> Anyway,  I don't want to talk about philosophy. I just noticed an
> anomaly and I wanted to share with the ML.

Sorry, but there is no anomaly. This is normal, (mostly) standard
conforming C code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"When anyone says `theoretically,' they really mean `not really.'"
- David Parnas

[U-Boot] IP_t should be a "packed" struct

[Alessandro Rubini]

> Now study the data structures used  for  netwrking  -  they  are  all
> carefully  crafted that the natural aligment "just fits", i. e. there
> are no gaps between variables if they are aligned naturally.

Actually, a packed structure not only doesn't have gaps, but can live
at any address. So the compiler must access it byte-by-byte
regardless.  This means that a naturally-aligned structure performs
much worse if declared as packed. And single-word loads and stores are
not atomic any more.

I tried with eldk-4.2 for arm and an older gcc for mips, I have
no ppc or other archs handy right now:

   #include <stdint.h>
   #include <stdio.h>

   #ifdef PACKEDTEST
   #  define P __attribute__ ((packed))
   #else
   #  define P
   #endif

   struct {uint32_t i; uint16_t s; uint8_t c, d;} P datum;

   uint32_t i;
   uint16_t s;
   uint8_t c;

   int main(int argc, char **argv)
   {
   	printf("ih\n");
   	i = datum.i;
   	printf("oh\n");
   	s = datum.s;
   	printf("ah\n");
   	c = datum.c;
   	return 0;
   }  

Compilation: ARM non-packed (only interesting lines):

    8378:       e5942000        ldr     r2, [r4]
    8384:       e5832000        str     r2, [r3]

    838c:       e1d420b4        ldrh    r2, [r4, #4]
    8398:       e1c320b0        strh    r2, [r3]

    83a0:       e5d42006        ldrb    r2, [r4, #6]
    83ac:       e5c32000        strb    r2, [r3]

Compilation: ARM packed (only interesting lines):

    8378:       e5d42001        ldrb    r2, [r4, #1]
    837c:       e5d43000        ldrb    r3, [r4]
    8380:       e5d40002        ldrb    r0, [r4, #2]
    8384:       e5d41003        ldrb    r1, [r4, #3]
    8388:       e1833402        orr     r3, r3, r2, lsl #8
    838c:       e1833800        orr     r3, r3, r0, lsl #16
    8394:       e1833c01        orr     r3, r3, r1, lsl #24
    8398:       e5823000        str     r3, [r2]

    83a4:       e5d43005        ldrb    r3, [r4, #5]
    83a8:       e5d42004        ldrb    r2, [r4, #4]
    83b0:       e1822403        orr     r2, r2, r3, lsl #8
    83b8:       e1c320b0        strh    r2, [r3]

    83c0:       e5d42006        ldrb    r2, [r4, #6]
    83cc:       e5c32000        strb    r2, [r3]

Compilation: MIPS non-packed (only interesting lines):

  44:   8e030000        lw      v1,0(s0)
  5c:   ac230000        sw      v1,0(at)

  74:   96030004        lhu     v1,4(s0)
  8c:   a4230000        sh      v1,0(at)

  a4:   92030006        lbu     v1,6(s0)
  bc:   a0230000        sb      v1,0(at)


Compilation: MIPS packed (only interesting lines):

  44:   8a030003        lwl     v1,3(s0)
  48:   9a030000        lwr     v1,0(s0)
  60:   ac230000        sw      v1,0(at)

  78:   92030005        lbu     v1,5(s0)
  7c:   92020004        lbu     v0,4(s0)
  80:   00031a00        sll     v1,v1,0x8
  84:   00621825        or      v1,v1,v0
  9c:   a4230000        sh      v1,0(at)

  b4:   92030006        lbu     v1,6(s0)
  cc:   a0230000        sb      v1,0(at)


/alessandro