* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
@ 2005-04-22 14:57 Woojung.Huh at smsc.com
2005-04-22 18:39 ` Wolfgang Denk
0 siblings, 1 reply; 14+ messages in thread
From: Woojung.Huh at smsc.com @ 2005-04-22 14:57 UTC (permalink / raw)
To: u-boot
Hi All,
I'm newbie to u-boot.
We are using u-boot for proprietary ARM board with NS16550 compatible serial controller.
It is configured as 4bytes access to serial controller, so set CFG_NS16550_REG_SIZE to 4.
But, because of "__attribute__ ((packed))" of struct NS16550, all register accesses to NS16550 are generated to byte
access (LDRB, STRB in ARM).
I think it should NOT have __attribute__ ((packed)) if NS16550 is configured as 4bytes access.
Am I missed something?
Thanks,
Woojung
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-22 14:57 [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h Woojung.Huh at smsc.com
@ 2005-04-22 18:39 ` Wolfgang Denk
2005-04-25 7:44 ` Jean-Paul Saman
0 siblings, 1 reply; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-22 18:39 UTC (permalink / raw)
To: u-boot
In message <OF34DF8405.50E611E8-ON85256FEB.0051A68A-85256FEB.00522DF9@smsc.com> you wrote:
>
> We are using u-boot for proprietary ARM board with NS16550 compatible serial controller.
> It is configured as 4bytes access to serial controller, so set CFG_NS16550_REG_SIZE to 4.
> But, because of "__attribute__ ((packed))" of struct NS16550, all register accesses to NS16550 are generated to byte
> access (LDRB, STRB in ARM).
No, this staement makes no sense to me.
> I think it should NOT have __attribute__ ((packed)) if NS16550 is configured as 4bytes access.
> Am I missed something?
I think so.
"__attribute__ ((packed))" only affects the alignment of the entries
in the structure. It has no influence on how to access these entries.
If your access is a 32 bit read or write access then it will not be
changed whether there is a "__attribute__ ((packed))" in place or
not.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every program has at least one bug and can be shortened by at least
one instruction -- from which, by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
@ 2005-04-22 18:55 Woojung.Huh at smsc.com
0 siblings, 0 replies; 14+ messages in thread
From: Woojung.Huh at smsc.com @ 2005-04-22 18:55 UTC (permalink / raw)
To: u-boot
Wolfgang,
Thanks for your reply.
Maybe my configuration was wrong at that time when I compile sources.
Now it works as you described. (It was non-sense to me too.)
It means "__attribute__((packed))" doesn't affect 4byte access to serial controller.
Thanks,
Woojung
Wolfgang Denk
<wd@denx.de> To: Woojung Huh/SMSC at SMSC
Sent by: cc: U-Boot-Users at lists.sourceforge.net
wd at denx.de Subject: Re: [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
04/22/2005 02:39
PM
In message <OF34DF8405.50E611E8-ON85256FEB.0051A68A-85256FEB.00522DF9@smsc.com> you wrote:
>
> We are using u-boot for proprietary ARM board with NS16550 compatible serial controller.
> It is configured as 4bytes access to serial controller, so set CFG_NS16550_REG_SIZE to 4.
> But, because of "__attribute__ ((packed))" of struct NS16550, all register accesses to NS16550 are generated to byte
> access (LDRB, STRB in ARM).
No, this staement makes no sense to me.
> I think it should NOT have __attribute__ ((packed)) if NS16550 is configured as 4bytes access.
> Am I missed something?
I think so.
"__attribute__ ((packed))" only affects the alignment of the entries
in the structure. It has no influence on how to access these entries.
If your access is a 32 bit read or write access then it will not be
changed whether there is a "__attribute__ ((packed))" in place or
not.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every program has at least one bug and can be shortened by at least
one instruction -- from which, by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-22 18:39 ` Wolfgang Denk
@ 2005-04-25 7:44 ` Jean-Paul Saman
2005-04-25 8:20 ` Wolfgang Denk
0 siblings, 1 reply; 14+ messages in thread
From: Jean-Paul Saman @ 2005-04-25 7:44 UTC (permalink / raw)
To: u-boot
Wofgang,
Did you inspect the generated assembly statemens of __attribute__
((packed)) on the struct NS16650 when CFG_NS16550_REG_SIZE is 4? If you
did then you would have noticed gcc (atleast 3.4.3 and 3.4.0) generates
byte access code for it. As I mailed to this mailinglist at the first of
March 2005. Subject: "[PATCH] revised word alignment fixes for word
aligned NS16550 UART"
Kind greetings,
Jean-Paul Saman
Philips Semiconductors CTO/RTG
Philips HighTech Campus, building WDA 3.29
Professor van den Holstlaan 4
5655 AA Eindhoven
tel: +31 (0)40 27 45131
Wolfgang Denk <wd@denx.de>
Sent by:
u-boot-users-admin at lists.sourceforge.net
22-04-2005 20:39
To: Woojung.Huh at smsc.com
cc: U-Boot-Users at lists.sourceforge.net
(bcc: Jean-Paul Saman/EHV/SC/PHILIPS)
Subject: Re: [U-Boot-Users] struct NS16550 {...}
__attribute__ ((packed)) at ns16550.h
Classification:
In message
<OF34DF8405.50E611E8-ON85256FEB.0051A68A-85256FEB.00522DF9@smsc.com> you
wrote:
>
> We are using u-boot for proprietary ARM board with NS16550 compatible
serial controller.
> It is configured as 4bytes access to serial controller, so set
CFG_NS16550_REG_SIZE to 4.
> But, because of "__attribute__ ((packed))" of struct NS16550, all
register accesses to NS16550 are generated to byte
> access (LDRB, STRB in ARM).
No, this staement makes no sense to me.
> I think it should NOT have __attribute__ ((packed)) if NS16550 is
configured as 4bytes access.
> Am I missed something?
I think so.
"__attribute__ ((packed))" only affects the alignment of the entries
in the structure. It has no influence on how to access these entries.
If your access is a 32 bit read or write access then it will not be
changed whether there is a "__attribute__ ((packed))" in place or
not.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every program has at least one bug and can be shortened by at least
one instruction -- from which, by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 7:44 ` Jean-Paul Saman
@ 2005-04-25 8:20 ` Wolfgang Denk
2005-04-25 14:06 ` Arthur Shipkowski
0 siblings, 1 reply; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-25 8:20 UTC (permalink / raw)
To: u-boot
[Please do not send regular messages to the u-boot-users-admin address!]
In message <OFC533536B.66CF098A-ONC1256FEE.002A2AD5-C1256FEE.002A9FFB@philips.com> you wrote:
>
> Did you inspect the generated assembly statemens of __attribute__
> ((packed)) on the struct NS16650 when CFG_NS16550_REG_SIZE is 4? If you
No, I did not.
> did then you would have noticed gcc (atleast 3.4.3 and 3.4.0) generates
> byte access code for it. As I mailed to this mailinglist at the first of
If this is really true, then there is either bug in GCC or in the code.
If I code a 32 bit access the compiler must not use any other accesses.
If CFG_NS16550_REG_SIZE is 4, then all entries in "struct NS16550"
are of type "unsigned long". All of the compilers I'm working with
generate exactly the same code whether "__attribute__ ((packed))" is
used or not.
[BTW: at the moment only the U824 board uses CFG_NS16550_REG_SIZE=4,
and this is a PowerPC board.]
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"One day," said a dull voice from down below, "I'm going to be back
in form again and you're going to be very sorry you said that. For a
very long time. I might even go so far as to make even more Time just
for you to be sorry in." - Terry Pratchett, _Small Gods_
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 8:20 ` Wolfgang Denk
@ 2005-04-25 14:06 ` Arthur Shipkowski
2005-04-25 14:56 ` Wolfgang Denk
0 siblings, 1 reply; 14+ messages in thread
From: Arthur Shipkowski @ 2005-04-25 14:06 UTC (permalink / raw)
To: u-boot
> If this is really true, then there is either bug in GCC or in the code.
> If I code a 32 bit access the compiler must not use any other accesses.
If you use __attribute__ ((packed)), you are telling the compiler you have
an unaligned data structure (e.g. longs not by-4 aligned). Therefore, GCC
will always reassemble the 32-bit value from byte accesses.
Art Shipkowski
Videon Central Software Engineer
(814)235-1111 x307
art at videon-central.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 14:06 ` Arthur Shipkowski
@ 2005-04-25 14:56 ` Wolfgang Denk
2005-04-25 15:18 ` Pantelis Antoniou
0 siblings, 1 reply; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-25 14:56 UTC (permalink / raw)
To: u-boot
In message <001501c5499f$f5364750$1780a8c0@art> you wrote:
> > If this is really true, then there is either bug in GCC or in the code.
> > If I code a 32 bit access the compiler must not use any other accesses.
>
> If you use __attribute__ ((packed)), you are telling the compiler you have
> an unaligned data structure (e.g. longs not by-4 aligned). Therefore, GCC
> will always reassemble the 32-bit value from byte accesses.
You are wrong, with both of your statements.
-> cat foo.c
struct foo {
unsigned long foo1;
unsigned long foo2;
unsigned long foo3;
} __attribute__ ((packed));
unsigned long dummy (struct foo *p)
{
return (p->foo1 + p->foo2 + p->foo3);
}
Test 1: PowerPC:
-> ppc_8xx-gcc -O -S foo.c
-> cat foo.s
...
dummy:
mr 9,3
lwz 3,0(3)
lwz 0,4(9)
add 3,3,0
lwz 0,8(9)
add 3,3,0
blr
Test 2: MIPS:
-> mips_4KC-gcc -O -S foo.c
-> cat foo.s
...
dummy:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, extra= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.cpload $25
.set reorder
ulw $2,0($4)
ulw $3,4($4)
#nop
addu $2,$2,$3
ulw $3,8($4)
.set noreorder
.set nomacro
j $31
addu $2,$2,$3
.set macro
.set reorder
As you can see, GCC alway uses plain word accesses (32 bit).
__attribute__ ((packed)) is telling the compiler that IN CASE of
unaligned fields in the structure no padding should be performend,
which will then result in unaligned fields that may cause access
problems on certain architectures.
If a compiler is generating byte accesses instead (especially if the
relevant data has been declared as volatile) then this compiler is
broken.
Yes, I know that some versions of ARM compilers behave like that. But
the fact that they do does not mean that they are correct. Accessing
a (volatile) long variable though 4 byte accesses is a bug.
Period.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Murder is contrary to the laws of man and God.
-- M-5 Computer, "The Ultimate Computer", stardate 4731.3
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 14:56 ` Wolfgang Denk
@ 2005-04-25 15:18 ` Pantelis Antoniou
2005-04-25 19:43 ` Wolfgang Denk
0 siblings, 1 reply; 14+ messages in thread
From: Pantelis Antoniou @ 2005-04-25 15:18 UTC (permalink / raw)
To: u-boot
Wolfgang Denk wrote:
> In message <001501c5499f$f5364750$1780a8c0@art> you wrote:
>
>>>If this is really true, then there is either bug in GCC or in the code.
>>>If I code a 32 bit access the compiler must not use any other accesses.
>>
>>If you use __attribute__ ((packed)), you are telling the compiler you have
>>an unaligned data structure (e.g. longs not by-4 aligned). Therefore, GCC
>>will always reassemble the 32-bit value from byte accesses.
>
>
> You are wrong, with both of your statements.
>
> -> cat foo.c
> struct foo {
> unsigned long foo1;
> unsigned long foo2;
> unsigned long foo3;
> } __attribute__ ((packed));
>
> unsigned long dummy (struct foo *p)
> {
> return (p->foo1 + p->foo2 + p->foo3);
> }
>
>
> Test 1: PowerPC:
>
> -> ppc_8xx-gcc -O -S foo.c
> -> cat foo.s
> ...
> dummy:
> mr 9,3
> lwz 3,0(3)
> lwz 0,4(9)
> add 3,3,0
> lwz 0,8(9)
> add 3,3,0
> blr
>
>
> Test 2: MIPS:
>
> -> mips_4KC-gcc -O -S foo.c
> -> cat foo.s
> ...
> dummy:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, extra= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .cpload $25
> .set reorder
> ulw $2,0($4)
> ulw $3,4($4)
> #nop
> addu $2,$2,$3
> ulw $3,8($4)
> .set noreorder
> .set nomacro
> j $31
> addu $2,$2,$3
> .set macro
> .set reorder
>
>
> As you can see, GCC alway uses plain word accesses (32 bit).
>
>
>
Not on ARM it does not.
>
> Best regards,
>
> Wolfgang Denk
>
Both PPC & MIPS can do proper unalligned load/stores (or fault to
an exception).
ARM (silently) does the wrong thing i.e. masks out the lower 2 bits.
So gcc on ARM generates byte load/stores.
Regards
Pantelis
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 15:18 ` Pantelis Antoniou
@ 2005-04-25 19:43 ` Wolfgang Denk
2005-04-25 20:15 ` Arthur Shipkowski
2005-04-26 6:23 ` Pantelis Antoniou
0 siblings, 2 replies; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-25 19:43 UTC (permalink / raw)
To: u-boot
In message <426D0A4C.5000105@intracom.gr> you wrote:
>
> Not on ARM it does not.
Yes, I know? Why did you not quote this part of my message, too?
> ARM (silently) does the wrong thing i.e. masks out the lower 2 bits.
> So gcc on ARM generates byte load/stores.
...which is broken, especially when there is no need to do this since
the fields are properly aligned.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Monday is an awful way to spend one seventh of your life.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 19:43 ` Wolfgang Denk
@ 2005-04-25 20:15 ` Arthur Shipkowski
2005-04-25 20:59 ` Wolfgang Denk
2005-04-26 6:23 ` Pantelis Antoniou
1 sibling, 1 reply; 14+ messages in thread
From: Arthur Shipkowski @ 2005-04-25 20:15 UTC (permalink / raw)
To: u-boot
>> ARM (silently) does the wrong thing i.e. masks out the lower 2 bits.
>> So gcc on ARM generates byte load/stores.
>
> ...which is broken, especially when there is no need to do this since
> the fields are properly aligned.
Based on a quick Google scan of the GCC mailing list, __attribute__
((packed)) also works (partially due to necessity and partially due to
breaking old code) as a mode to allow unaligned access to a given structure.
Some architectures offer no support for doing unaligned long loads and
stores directly (I suspect on ARM it might be due to some sort of
brokenness). On the other hand, PPC handles it in hardware, while MIPS
invokes an exception handler in the Linux kernel
(arch/mips/kernel/unaligned.c).
There are discussions on this on the GCC mailing list going back a ways; a
notable one (that mentions m68k as another platform that generates byte
accesses besides ARM) is at http://gcc.gnu.org/ml/gcc/1997-10/msg00063.html
while a more recent bug report hinting at this fact is at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12200
Art
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 20:15 ` Arthur Shipkowski
@ 2005-04-25 20:59 ` Wolfgang Denk
0 siblings, 0 replies; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-25 20:59 UTC (permalink / raw)
To: u-boot
Dear Arthur,
in message <003501c549d3$7d67aa00$1780a8c0@art> you wrote:
>
> There are discussions on this on the GCC mailing list going back a ways; a
> notable one (that mentions m68k as another platform that generates byte
> accesses besides ARM) is at http://gcc.gnu.org/ml/gcc/1997-10/msg00063.html
> while a more recent bug report hinting at this fact is at
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12200
Thanks for digging this out.
So the real fix for this problem should be to use a proper
"__attribute__((aligned(?)))" where struct NS16550 is instantiated.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I have a theory that it's impossible to prove anything, but I can't
prove it.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-25 19:43 ` Wolfgang Denk
2005-04-25 20:15 ` Arthur Shipkowski
@ 2005-04-26 6:23 ` Pantelis Antoniou
2005-04-26 7:21 ` Wolfgang Denk
1 sibling, 1 reply; 14+ messages in thread
From: Pantelis Antoniou @ 2005-04-26 6:23 UTC (permalink / raw)
To: u-boot
Wolfgang Denk wrote:
> In message <426D0A4C.5000105@intracom.gr> you wrote:
>
>>Not on ARM it does not.
>
>
> Yes, I know? Why did you not quote this part of my message, too?
>
>
>>ARM (silently) does the wrong thing i.e. masks out the lower 2 bits.
>>So gcc on ARM generates byte load/stores.
>
>
> ...which is broken, especially when there is no need to do this since
> the fields are properly aligned.
>
The compiler does not know this. A pointer to the structure could be
pointing to an non-aligned address, and when an aligned field within
the structure is accessed it will be not aligned.
This typically happens when networking.
IMHO __attribute((packed)) is deceivingly named, since it implies
two things.
1) Don't leave any space between fields of the structure.
2) Access the fields of the structure even when the alignment of the
pointer pointing to it is wrong.
Just by the name, you expect only (1).
> Best regards,
>
> Wolfgang Denk
>
Regards
Pantelis
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-26 6:23 ` Pantelis Antoniou
@ 2005-04-26 7:21 ` Wolfgang Denk
2005-04-26 14:32 ` Cory Tusar
0 siblings, 1 reply; 14+ messages in thread
From: Wolfgang Denk @ 2005-04-26 7:21 UTC (permalink / raw)
To: u-boot
In message <426DDE65.4060009@intracom.gr> you wrote:
>
> > ...which is broken, especially when there is no need to do this since
> > the fields are properly aligned.
>
> The compiler does not know this. A pointer to the structure could be
> pointing to an non-aligned address, and when an aligned field within
> the structure is accessed it will be not aligned.
This is no reason for anything. The same could be true for ANY
structures, even without using the packed attribute. It is the user's
problem if he passes incorrectly aligned pointers.
> This typically happens when networking.
Yes, and usually you will have to explicitely deal with this.
> IMHO __attribute((packed)) is deceivingly named, since it implies
> two things.
I think you are wrong. You are trying to explain the current imple-
mentation of GCC for ARM, assuming silently that the implementation
is correct. I say: it is broken.
> 1) Don't leave any space between fields of the structure.
Correct - this is what the documentation says:
The `packed' attribute specifies that a variable or structure field
should have the smallest possible alignment--one byte for a
variable, and one bit for a field, unless you specify a larger
value with the `aligned' attribute.
Here is a structure in which the field `x' is packed, so that it
immediately follows `a':
struct foo
{
char a;
int x[2] __attribute__ ((packed));
};
> 2) Access the fields of the structure even when the alignment of the
> pointer pointing to it is wrong.
Wrong. Remember that a structure may be a representation for things
like hardware registers. Registers may have funny properties, like
auto-incrementing with each access, or auto-resetting on read. If I
perform a 32 bit read on such an object the compiler MUST NOT break
this up into any other combination of accesses.
> Just by the name, you expect only (1).
By the name, and the documentation, and common sense.
I think we have covered all relevant facts now. I understand that we
won't have GCC fixed soon, and I already suggested what I think
should be used to work around this compiler bug (using aligned).
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quote from the Boss... "I didn't say it was your fault. I said I was
going to blame it on you."
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h
2005-04-26 7:21 ` Wolfgang Denk
@ 2005-04-26 14:32 ` Cory Tusar
0 siblings, 0 replies; 14+ messages in thread
From: Cory Tusar @ 2005-04-26 14:32 UTC (permalink / raw)
To: u-boot
Wolfgang Denk wrote:
> In message <426DDE65.4060009@intracom.gr> you wrote:
>
>>>...which is broken, especially when there is no need to do this since
>>>the fields are properly aligned.
>>
>>The compiler does not know this. A pointer to the structure could be
>>pointing to an non-aligned address, and when an aligned field within
>>the structure is accessed it will be not aligned.
>
> This is no reason for anything. The same could be true for ANY
> structures, even without using the packed attribute. It is the user's
> problem if he passes incorrectly aligned pointers.
No, it is the compiler's problem if it incorrectly generates code which
breaks given an access to an unaligned field. The pointer itself could
very well be correctly aligned, and yet be pointing to an unaligned data
structure. The compiler has no way of knowing this at compile-time, and
hence must generate code which is the "least common denominator", i.e. -
the code will work, correctly, in ALL situations, regardless of the
structure's alignment.
>>This typically happens when networking.
>
> Yes, and usually you will have to explicitely deal with this.
>
>>IMHO __attribute((packed)) is deceivingly named, since it implies
>>two things.
>
> I think you are wrong. You are trying to explain the current imple-
> mentation of GCC for ARM, assuming silently that the implementation
> is correct. I say: it is broken.
Given that a similar series of bytewise accesses will also be generated
using armcc, I'd say gcc is _consistent_.
http://www.arm.com/support/faqdev/1469.html
http://www.arm.com/support/faqdev/1228.html
http://www.arm.com/support/faqip/3661.html
Again, the compiler MUST ensure that no incorrect code is generated to
perform the access. Simply because said access is not performed in what
may be the optimal way, does NOT mean the compiler is broken, or that
this is a bug.
From one of the above referenced tech notes:
"Unknown alignment (eg. a pointer to a word that can be at any address)
This is specified by a pointer to a __packed item, eg.
__packed int *pi;
In this case, the ARM compilers generate code which correctly accesses
the value regardless of the alignment of the pointer. This code
generated will be a sequence of byte accesses, or variable
alignment-dependent shifting and masking (depending on the compile
options) and will therefore incur a performance penalty.
Note that *any* __packed object accessed through a pointer has unknown
alignment, even packed structures."
>>1) Don't leave any space between fields of the structure.
>
> Correct - this is what the documentation says:
>
> The `packed' attribute specifies that a variable or structure field
> should have the smallest possible alignment--one byte for a
> variable, and one bit for a field, unless you specify a larger
> value with the `aligned' attribute.
>
> Here is a structure in which the field `x' is packed, so that it
> immediately follows `a':
>
> struct foo
> {
> char a;
> int x[2] __attribute__ ((packed));
> };
...which further implies that ANY access to a member of a packed struct
may be unaligned, even if it is aligned within the structure definition
itself, if the structure itself is unaligned in memory...
struct foo {
unsigned long foo1;
unsigned long foo2;
unsigned long foo3;
} __attribute__ ((packed));
unsigned long dummy(struct foo *p)
{
p = (struct foo *)1;
return(p->foo1 + p->foo2 + p->foo3);
}
Yes, the above is contrived, but it serves to illustrate the point.
>>2) Access the fields of the structure even when the alignment of the
>> pointer pointing to it is wrong.
>
> Wrong. Remember that a structure may be a representation for things
> like hardware registers. Registers may have funny properties, like
> auto-incrementing with each access, or auto-resetting on read. If I
> perform a 32 bit read on such an object the compiler MUST NOT break
> this up into any other combination of accesses.
No, right. If the _user_ chooses to implement such a structure for
memory-mapped accesses, it is then up to the _user_ to validate that the
accesses will be performed as intended. The _compiler_ is responsible
ONLY for generating code to correctly load/store all 32 bits of data.
There is nothing which requires that the load/store be atomic in nature.
The _compiler_ has no knowledge it is reading/writing a register which
requires atomic accesses, only the _user_ does.
>>Just by the name, you expect only (1).
>
> By the name, and the documentation, and common sense.
Then I doubt we share a common sense of "common sense". ;)
Oh, the pain of the pun!
> I think we have covered all relevant facts now. I understand that we
> won't have GCC fixed soon, and I already suggested what I think
> should be used to work around this compiler bug (using aligned).
gcc does not need fixed in this regard; the above is not a bug. gcc is
generating correct code in all cases given the fact that the ARM
architecture has no support for unaligned accesses.
Please cc: me directly if anyone wishes to discuss this further, as I
should think this is now well off-topic for the list.
-Cory
--
Cory T. Tusar
Embedded Systems Engineer
Videon Central, Inc.
2171 Sandy Drive
State College, PA 16801
(814) 235-1111 x316
(814) 235-1118 fax
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." --Brian W. Kernighan
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2005-04-26 14:32 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-04-22 14:57 [U-Boot-Users] struct NS16550 {...} __attribute__ ((packed)) at ns16550.h Woojung.Huh at smsc.com
2005-04-22 18:39 ` Wolfgang Denk
2005-04-25 7:44 ` Jean-Paul Saman
2005-04-25 8:20 ` Wolfgang Denk
2005-04-25 14:06 ` Arthur Shipkowski
2005-04-25 14:56 ` Wolfgang Denk
2005-04-25 15:18 ` Pantelis Antoniou
2005-04-25 19:43 ` Wolfgang Denk
2005-04-25 20:15 ` Arthur Shipkowski
2005-04-25 20:59 ` Wolfgang Denk
2005-04-26 6:23 ` Pantelis Antoniou
2005-04-26 7:21 ` Wolfgang Denk
2005-04-26 14:32 ` Cory Tusar
-- strict thread matches above, loose matches on Subject: below --
2005-04-22 18:55 Woojung.Huh at smsc.com
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox