转载自:OpenSolaris: code
joerg
Posts: 230
From: Berlin
Registered: 4/27/05
Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 2:53 PM Reply
Hi,
it seems that there are plenty of bugs in the named
include files because the code uses something like:
unsigned int xxx :1;
and seems to asume that this will be packed.
This will never work execpt in a excotic compiler like
the IBM AIX compiler.
Traditional UNIX C compilers pack bitfield in case that
the type is unsigned char and in case there is no bitfield
that crosses a byte boundary.
As the named files are full of UINT32_BIT and UINT16_BIT,
I asume that there are currently plenty of bugs.
Any comment from someone who is envolved with the ACPI code?
Jörg
--
EMail:joerg at schily dot isdn dot cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
[email protected] (uni)
schilling at fokus dot fraunhofer dot de (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
imp
Posts: 17
From: Israel
Registered: 4/27/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 4:48 PM in response to: joerg Reply
Joerg Schilling wrote:
> Hi,
>
> it seems that there are plenty of bugs in the named
> include files because the code uses something like:
>
> unsigned int xxx :1;
>
> and seems to asume that this will be packed.
>
> This will never work execpt in a excotic compiler like
> the IBM AIX compiler.
>
> Traditional UNIX C compilers pack bitfield in case that
> the type is unsigned char and in case there is no bitfield
> that crosses a byte boundary.
>
> As the named files are full of UINT32_BIT and UINT16_BIT,
> I asume that there are currently plenty of bugs.
>
> Any comment from someone who is envolved with the ACPI code?
>
> Jörg
>
If I recall correctly that code is an INTEL reference ACPI
implementation which replaced previous Sun' implementation
recently.
Cyril
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
joerg
Posts: 230
From: Berlin
Registered: 4/27/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 5:43 PM in response to: imp Reply
Cyril Plisko <Cyril dot Plisko at CLLF dot COM> wrote:
> > This will never work execpt in a excotic compiler like
> > the IBM AIX compiler.
> >
> > Traditional UNIX C compilers pack bitfield in case that
> > the type is unsigned char and in case there is no bitfield
> > that crosses a byte boundary.
> >
> > As the named files are full of UINT32_BIT and UINT16_BIT,
> > I asume that there are currently plenty of bugs.
> >
> > Any comment from someone who is envolved with the ACPI code?
> >
> > Jörg
> >
>
> If I recall correctly that code is an INTEL reference ACPI
> implementation which replaced previous Sun' implementation
> recently.
But it is written in a horrobly nonportable way
Jörg
--
EMail:joerg at schily dot isdn dot cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
[email protected] (uni)
schilling at fokus dot fraunhofer dot de (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
seongbae
Posts: 3
Registered: 3/21/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 4:58 PM in response to: joerg Reply
Joerg Schilling wrote:
> Hi,
>
> it seems that there are plenty of bugs in the named
> include files because the code uses something like:
>
> unsigned int xxx :1;
>
> and seems to asume that this will be packed.
1 bit bitfield is always "packed",
since it can not straddle the storage boundary
(whatever its size is). See below.
> This will never work execpt in a excotic compiler like
> the IBM AIX compiler.
>
> Traditional UNIX C compilers pack bitfield in case that
> the type is unsigned char and in case there is no bitfield
> that crosses a byte boundary.
In general (for all possible bitfield sizes),
this is an implementation-defined behavior.
From c99 6.7.2.1:
> 10 An implementation may allocate any addressable storage unit large
> enough to hold a bitfield. If enough space remains, a bit-field that
> immediately follows another bit-field in a structure shall be packed
> into adjacent bits of the same unit. If insufficient space remains,
> whether a bit-field that does not fit is put into the next unit or
> overlaps adjacent units is implementation-defined. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined. The alignment of
> the addressable storage unit is unspecified.
> As the named files are full of UINT32_BIT and UINT16_BIT,
Those can not be assumed to be packed.
Seongbae
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
joerg
Posts: 230
From: Berlin
Registered: 4/27/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 5:55 PM in response to: seongbae Reply
Seongbae Park <seongbae dot park at sun dot com> wrote:
> Joerg Schilling wrote:
> > Hi,
> >
> > it seems that there are plenty of bugs in the named
> > include files because the code uses something like:
> >
> > unsigned int xxx :1;
> >
> > and seems to asume that this will be packed.
>
> 1 bit bitfield is always "packed",
> since it can not straddle the storage boundary
> (whatever its size is). See below.
I am not sure if you understand the same as I do by packed.
If you use "type xxx :yy;
and 'type' is biffer than char or yy is bigger than 8, most
compilers will introduce padding and it seems that the Intel Code
does not like padding.
> > This will never work execpt in a excotic compiler like
> > the IBM AIX compiler.
> >
> > Traditional UNIX C compilers pack bitfield in case that
> > the type is unsigned char and in case there is no bitfield
> > that crosses a byte boundary.
>
> In general (for all possible bitfield sizes),
> this is an implementation-defined behavior.
This is theory from ansi C...Reallity is that you may safely use bitfields
in case you do not use more than type char and in case you do not create
bitfields that cross byte boundaries.
If you check the ACPI structures, you will find that they include
obviously unwanted padding.
typedef struct madt_local_apic_nmi is 8 bytes regardless whether you compile
with gcc or Sun cc.
If you chage the structure to:
typedef struct madt_local_apic_nmi
{
UINT8 Type;
UINT8 Length;
UINT8 ProcessorId;
UINT8 Polarity : 2;
UINT8 TriggerMode : 2;
UINT8 Resx : 4:
UINT8 Reserved1;
UINT8 Lint;
} MADT_LOCAL_APIC_NMI;
it will work as expected.
NOTE that you still need to check the _bitorder_ that is used by the
compiler. The Plan 9 compiler does e.g. use a different bitorder than
traditionally UNIX compilers.
If you like to look at a bitfield using implementation that works
on > 30 platforms, have a look at the SCSI implementation I am using
in cdrtools.
> From c99 6.7.2.1:
> > 10 An implementation may allocate any addressable storage unit large
> > enough to hold a bitfield. If enough space remains, a bit-field that
> > immediately follows another bit-field in a structure shall be packed
> > into adjacent bits of the same unit. If insufficient space remains,
> > whether a bit-field that does not fit is put into the next unit or
> > overlaps adjacent units is implementation-defined. The order of
> > allocation of bit-fields within a unit (high-order to low-order or
> > low-order to high-order) is implementation-defined. The alignment of
> > the addressable storage unit is unspecified.
Conclusion: do never trust ANSI C at this point
> > As the named files are full of UINT32_BIT and UINT16_BIT,
>
> Those can not be assumed to be packed.
Correct and for this reason, we would need to put some effort into the
broken/nonportable code from Intel....
Jörg
--
EMail:joerg at schily dot isdn dot cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
[email protected] (uni)
schilling at fokus dot fraunhofer dot de (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
ian
Posts: 25
From: New Zealand
Registered: 4/27/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 24, 2005 6:44 PM in response to: joerg Reply
Joerg Schilling wrote:
>This is theory from ansi C...Reallity is that you may safely use bitfields
>in case you do not use more than type char and in case you do not create
>bitfields that cross byte boundaries.
>
>If you check the ACPI structures, you will find that they include
>obviously unwanted padding.
>
>typedef struct madt_local_apic_nmi is 8 bytes regardless whether you compile
>with gcc or Sun cc.
>
>If you chage the structure to:
>
>typedef struct madt_local_apic_nmi
>{
> UINT8 Type;
> UINT8 Length;
> UINT8 ProcessorId;
> UINT8 Polarity : 2;
> UINT8 TriggerMode : 2;
> UINT8 Resx : 4:
> UINT8 Reserved1;
> UINT8 Lint;
>} MADT_LOCAL_APIC_NMI;
>
>it will work as expected.
>
>
>
The original is pretty bad, any bit field bigger than 8 bits that isn't
aligned to its natural boundary is very non portable.
Every compiler I have used packs bit fields as long as they fit the type
and the containing type is correctly aligned. This one (the original)
most defiantly isn't and is therefore non-portable.
Ian
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code
jkeil
Posts: 22
From: Germany / Bonn
Registered: 6/16/05
Re: Alignement problems in usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 27, 2005 3:35 AM in response to: joerg Reply
> typedef struct madt_local_apic_nmi is 8 bytes
> regardless whether you compile
> with gcc or Sun cc.
Apparently there are already crashes reported with acpica on certain machines due
to this problem (Bug ID: 6288486)
http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6288486
http://groups-beta.google.com/group/comp.unix.solaris/msg/7a6df011551255fc
Btw: if you add a "#pragma packed(1)" around the struct, gcc / cc don't agree
on the size of the structure any more, and I guess that's the root cause for
Bug ID: 6288486 / the crash reported on usenet in comp.unix.solaris
% cat x.c
typedef unsigned char UINT8;
typedef unsigned short UINT16;
#pragma pack(1)
typedef struct madt_local_apic_nmi
{
UINT8 Type;
UINT8 Length;
UINT8 ProcessorId;
UINT16 Polarity : 2;
UINT16 TriggerMode : 2;
UINT16 Reserved1 : 12;
UINT8 Lint;
} MADT_LOCAL_APIC_NMI;
#pragma pack()
main()
{
printf("%d\n", sizeof(MADT_LOCAL_APIC_NMI));
}
% gcc -o x x.c
% x
6
% cc -o x x.c
% x
7
> If you chage the structure to:
>
> typedef struct madt_local_apic_nmi
> {
> UINT8 Type;
> UINT8 Length;
> UINT8 ProcessorId;
> UINT8 Polarity : 2;
> UINT8 TriggerMode : 2;
> UINT8 Resx : 4:
> UINT8 Reserved1;
> UINT8 Lint;
> } MADT_LOCAL_APIC_NMI;
>
> it will work as expected.
Exactly.
joerg
Posts: 230
From: Berlin
Registered: 4/27/05
Re: Re: Alignement problems in
usr/src/uts/intel/sys/acpi/*.h
Posted: Jun 28, 2005 5:45 AM in response to: jkeil Reply
Jürgen Keil <jk at tools dot de> wrote:
> > typedef struct madt_local_apic_nmi is 8 bytes
> > regardless whether you compile
> > with gcc or Sun cc.
>
> Apparently there are already crashes reported with acpica on certain machines due
> to this problem (Bug ID: 6288486)
>
> http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6288486
> http://groups-beta.google.com/group/comp.unix.solaris/msg/7a6df011551255fc
>
>
> Btw: if you add a "#pragma packed(1)" around the struct, gcc / cc don't agree
> on the size of the structure any more, and I guess that's the root cause for
> Bug ID: 6288486 / the crash reported on usenet in comp.unix.solaris
OK, it was late when I checked the code and I don't know why I did not see
the #pragma packed(1) altough I searched for it. Maybe this is a result of
the fact that there is only one per file and not one per struct.
> > If you chage the structure to:
> >
> > typedef struct madt_local_apic_nmi
> > {
> > UINT8 Type;
> > UINT8 Length;
> > UINT8 ProcessorId;
> > UINT8 Polarity : 2;
> > UINT8 TriggerMode : 2;
> > UINT8 Resx : 4:
> > UINT8 Reserved1;
> > UINT8 Lint;
> > } MADT_LOCAL_APIC_NMI;
> >
> > it will work as expected.
>
> Exactly.
> This message posted from opensolaris.org
So if ther structs are all expected to be packed to the maximum, then we
need to change all Rerserved fields that use a size > 8 bit.
Jörg
--
EMail:joerg at schily dot isdn dot cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
[email protected] (uni)
schilling at fokus dot fraunhofer dot de (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
_______________________________________________
opensolaris-code mailing list
opensolaris-code at opensolaris dot org
https://opensolaris.org:444/mailman/listinfo/opensolaris-code