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

[Luigi Mantellini]

Dear All,

2009/1/28 Wolfgang Denk <wd at denx.de>:
> Dear Ben Warren,
>
> In message <4980CC59.1070302 at 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 at 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