Re: [U-Boot] packed attribute problem
Dear all, I wanted to point out clues about why GCC manages the packed attribute in this way. The behavior is suspiciously similar to what the ARM RVDS compiler (armcc) does with the __packed qualifier: > [...] > Thus if you wish to define a pointer to a word that can be at any address > (i.e. that can be at a non-natural > alignment) then you must specify this using the __packed qualifier when > defining the pointer: > >__packed int *pi; // pointer to unaligned int > > The ARM compilers will not then use an LDR, but instead 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 and code size penalty. > [...] ( From http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka3544.html ) I am starting to suspect that the "feature" was copied from the armcc compiler, even though there shouldn't be any reason to do that. I can picture someone translating C code from armcc to GCC and changing the __packed qualifier with the packed attribute, and then obtaining undesired 32-bit accesses when he wanted 8-bit accesses. Maybe the fact that GCC packed attribute did not map globals to unaligned addresses was listed as a "bug" by someone who was more familiar with armcc, and the developers who "fixed" it were uncertain about GCC specifications. Maybe the modification was part of a big changeset and went overlooked, and changing it back now would break so much code it's better to leave it so. But these are only speculations. The file "gcc/config/arm/arm.h" inside GCC source contains possibly useful information: > /* Setting STRUCTURE_SIZE_BOUNDARY to 32 produces more efficient code, but the > value set in previous versions of this toolchain was 8, which produces more > compact structures. The command line option -mstructure_size_boundary= > can be used to change this value. For compatibility with the ARM SDK > however the value should be left at 32. ARM SDT Reference Manual (ARM DUI > 0020D) page 2-20 says "Structures are aligned on word boundaries". > The AAPCS specifies a value of 8. */ It seems, though, that using -mstructure-size-boundary=8 or 32 with a 4.4.1-based toolchain has no difference on the managing of packed structs. Another interesting scenario is the access of arrays of packed structs. If a struct is packed but not aligned(4), its sizeof() could be a number that is not a multiple of the alignment (in the case of ARM targets). Even if the first element of the array is aligned, the second element has an unaligned address. Hope it rings some bells. Regards, Francesco ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Hi Scott, > On Thu, 7 Oct 2010 21:31:43 +0200 > Wolfgang Denk wrote: > >> Dear Scott Wood, >> >> In message <20101007135257.05a93...@udp111988uds.am.freescale.net> you wrote: >> > >> > > It is a pretty common method to use a pointer to some struct (for >> > > example, some form of PDU) and make it point to some I/O buffer. >> > >> > Yes, but at that point we are not talking about well-defined C, but >> > rather implementation-specific behavior. There's nothing wrong with >> > it, but the C standard is no longer authoritative on what happens in >> > such cases. >> >> Huch? Which part of that is not well-defined (or even not >> standard-conforming) C? > > Well, how do you obtain the unaligned pointer? [...] > Extensions like __attribute__((packed)) are obviously not > well-defined C. In ISO-IEC9899:TC3[1] I find this about dereferencing a pointer: ,[ 6.5.3.2 footnote 87 ] | [...] | Among the invalid values for dereferencing a pointer by the unary * | operator are a null pointer, an address inappropriately aligned for the | type of object pointed to, and the address of an object after the end of | its lifetime. ` So dereferencing such a pointer - however we arrive at it - is simply invalid. The same sentence is in the ANSI spec[2] also. The C FAQ[3] notes in section 16.8 that using unaligned pointers may very well lead to segmentation faults, bus errors or general protection faults. Not much documentation actually... Cheers Detlev [1] http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf [2] http://flash-gordon.me.uk/ansi.c.txt [3] http://www.c-faq.com/versions.html -- ... the tools we are trying to use and the language or notation we are using to express or record our thoughts, are the major factors determining what we can think or express at all! -- Edsger W. Dijkstra -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On Thu, 7 Oct 2010 21:31:43 +0200 Wolfgang Denk wrote: > Dear Scott Wood, > > In message <20101007135257.05a93...@udp111988uds.am.freescale.net> you wrote: > > > > > It is a pretty common method to use a pointer to some struct (for > > > example, some form of PDU) and make it point to some I/O buffer. > > > > Yes, but at that point we are not talking about well-defined C, but > > rather implementation-specific behavior. There's nothing wrong with > > it, but the C standard is no longer authoritative on what happens in > > such cases. > > Huch? Which part of that is not well-defined (or even not > standard-conforming) C? Well, how do you obtain the unaligned pointer? Casting an integer to a pointer? 6.3.2.3: "An integer may be converted to any pointer type. Except as previously specified, the result is implementation-defined, might not be properly aligned, and might not point to an entity of the referenced type." Casting a pointer to a pointer of a different type? 6.3.2.3: "A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned for the pointed-to type, the behavior is undefined." I was expecting to find something stating that it was not well-defined behavior if you dereference a pointer that points to data of a different type, but couldn't (maybe I wasn't searching for the right terms) -- though a similar limitation is present in 6.5.2.3 for unions: "With one exception, if the value of a member of a union object is used when the most recent store to the object was to a different member, the behavior is implementation-defined." Extensions like __attribute__((packed)) are obviously not well-defined C. > > Yes. And there would also be performance complaints if each of those > > accesses were to trap and be emulated (even ignoring weird stuff like > > old ARM). Thus it's nice to have some sort of pointer or data type > > annotation to tell the compiler to be careful. > > I also complain about poor performance when instead of a single > instruction (a 32 bit load) at least 4 (8 bit) instructions need to be > executed. So only use the unaligned annotation when there's a significant chance of it really being unaligned -- and make sure the compiler knows whether the cost of an unaligned access is significantly worse than the cost of its workaround. > > BTW, I see GCC splitting accesses to bitfields in a packed > > struct into bytes on powerpc, even with -mno-strict-align. > > Indeed. Bitfields have always been evil. Evil or not, I don't see why GCC feels the need to do this. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Scott Wood, In message <20101007135257.05a93...@udp111988uds.am.freescale.net> you wrote: > > > It is a pretty common method to use a pointer to some struct (for > > example, some form of PDU) and make it point to some I/O buffer. > > Yes, but at that point we are not talking about well-defined C, but > rather implementation-specific behavior. There's nothing wrong with > it, but the C standard is no longer authoritative on what happens in > such cases. Huch? Which part of that is not well-defined (or even not standard-conforming) C? > Yes. And there would also be performance complaints if each of those > accesses were to trap and be emulated (even ignoring weird stuff like > old ARM). Thus it's nice to have some sort of pointer or data type > annotation to tell the compiler to be careful. I also complain about poor performance when instead of a single instruction (a 32 bit load) at least 4 (8 bit) instructions need to be executed. > BTW, I see GCC splitting accesses to bitfields in a packed > struct into bytes on powerpc, even with -mno-strict-align. Indeed. Bitfields have always been evil. 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: w...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On Thu, 7 Oct 2010 19:57:01 +0200 Wolfgang Denk wrote: > Dear Scott Wood, > > In message <20101007114634.70e0f...@udp111988uds.am.freescale.net> you wrote: > > > > I think this gets down to how such pointers are generated -- if you > > stick with well-defined C, the compiler/ABI should be able to avoid > > generating an unaligned pointer. > > It is a pretty common method to use a pointer to some struct (for > example, some form of PDU) and make it point to some I/O buffer. Yes, but at that point we are not talking about well-defined C, but rather implementation-specific behavior. There's nothing wrong with it, but the C standard is no longer authoritative on what happens in such cases. > Depending on I/O subsystem, pre-pended protocol headerss and such the > resulting pointer may or may not be aligned. This is something that > the compiler has zero influence on. [Yes, of course the application > could always copy the data to a properly aligned memory region. But I > bet you would quickly complain about the poor throughput if the > network stack on your systems were implemented that way.] Yes. And there would also be performance complaints if each of those accesses were to trap and be emulated (even ignoring weird stuff like old ARM). Thus it's nice to have some sort of pointer or data type annotation to tell the compiler to be careful. > > when unaligned, e.g. old ARM chips that do weird rotaty things rather > > than trap when you do an unaligned access. > > I think it was acceptable for such systems to enforce the compiler (by > setting special compiler switches) to implement such extra access modes. > But doing it by default and unconditionally for all systems of that > architecture seems broken to me. It's unfair collective punishment. Yes, it ought to be configurable -- but this is only happening when you use __attribute__((packed)) without explicit alignment, right? BTW, I see GCC splitting accesses to bitfields in a packed struct into bytes on powerpc, even with -mno-strict-align. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Scott Wood, In message <20101007114634.70e0f...@udp111988uds.am.freescale.net> you wrote: > > I think this gets down to how such pointers are generated -- if you > stick with well-defined C, the compiler/ABI should be able to avoid > generating an unaligned pointer. It is a pretty common method to use a pointer to some struct (for example, some form of PDU) and make it point to some I/O buffer. Depending on I/O subsystem, pre-pended protocol headerss and such the resulting pointer may or may not be aligned. This is something that the compiler has zero influence on. [Yes, of course the application could always copy the data to a properly aligned memory region. But I bet you would quickly complain about the poor throughput if the network stack on your systems were implemented that way.] > > I may be wrong here, but I cannot > > think of any statement in the standards in this regard - it's only data > > types that imply alignments. > > Pointers point to typed data... Yes, and these typed date can be anywhere, even on addresses that don't match the natural alignment of these data. > C doesn't guarantee this, and it would be broken on certain chips Actually I consider this a deficiency in the C specification. I've been using C since Unix Version 6, and to me it is still mostly a high-level assembler language. I definitly do not want the compiler to do different things than what I tell him. > when unaligned, e.g. old ARM chips that do weird rotaty things rather > than trap when you do an unaligned access. I think it was acceptable for such systems to enforce the compiler (by setting special compiler switches) to implement such extra access modes. But doing it by default and unconditionally for all systems of that architecture seems broken to me. It's unfair collective punishment. But alas. That's how it is in ARM land... 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: w...@denx.de Perl already has an IDE. It's called Unix. -- Tom Christiansen in 375bd...@cs.colorado.edu ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On Thu, 7 Oct 2010 11:59:07 +0200
Detlev Zundel wrote:
> Hi Scott,
>
> > So instead of the code breaking in the obvious situation where the
> > unaligned address is assigned to a pointer type that implies alignment,
> > it breaks when sufficient barriers to optimization are added that the
> > compiler can no longer keep track of where the value came from?
>
> This is getting somewhat theoretical here, but I do not believe that "a
> pointer type implies alignment" in C.
I think this gets down to how such pointers are generated -- if you
stick with well-defined C, the compiler/ABI should be able to avoid
generating an unaligned pointer.
> I may be wrong here, but I cannot
> think of any statement in the standards in this regard - it's only data
> types that imply alignments.
Pointers point to typed data...
> So if this is true, then for pointers gcc
> will always have to reason from actual values assigned to them.
That's not always possible. How will GCC reason about this pointer,
where foo is called from a different compilation unit?
int foo(int *ptr)
{
return *ptr;
}
>
> > Seems like the right thing is for GCC to warn about the assignment to a
> > plain "u32 *", and allow a warningless assignment to something like
> > "u32 __attribute__((packed)) *".
>
> Somehow I still feel that gcc should _always_ generate 32-bit accesses
> when I dereference a u32 pointer in code.
C doesn't guarantee this, and it would be broken on certain chips
when unaligned, e.g. old ARM chips that do weird rotaty things rather
than trap when you do an unaligned access.
This is why MMIO accessors should be done with inline assembly rather
than a volatile pointer, although it's usually just a theoretical
concern.
> If such an access traps the machine, then so be it. Then the code has
> to be changed to reflect this in the source code level which will _make
> programmers ware of it_.
There is something at the source code level triggering this:
__attribute__((packed)).
That said, I don't think a lack of alignment of certain fields within
the struct ought to imply a lack of alignment of the start of the
struct. There should have been two separate attributes for that, but
changing it now would probably not be a good thing (unless two entirely
new names are introduced).
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Hi Scott,
> On Tue, 5 Oct 2010 13:43:01 +0200
> Detlev Zundel wrote:
>
>> Hi Reinhard,
>>
>> [...]
>>
>> >> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> >> this is *not* an issue with very recent tool chains.
>> >
>> > OK, for directly adressing elements inside a packed struct;
>> > but the original post said:
>> >
>> > "struct xyz {
>> >int x;
>> >int y;
>> >int z[CONST];
>> > } __attribute__ ((packed));
>> >
>> > struct xyz *abc;
>> > u32 * status_reg = (u32 *)&abc->z[0];
>> >
>> > writel(status, status_reg);"
>> >
>> > So the "status_reg" pointer is in a completely unrelated (to the packed
>> > struct)
>> > "u32 *" and still the access is done like it was packed. If the
>> > compiler silently drags that attribute along into the "u32 *"
>> > THAT is really sick!
>>
>> I cannot follow why you think this is sick - actually I think this is
>> clever. GCC "knows" that abc points to a packed struct and it has no
>> clue on actual values, so it _has to assume_ it is not aligned.
>
> So instead of the code breaking in the obvious situation where the
> unaligned address is assigned to a pointer type that implies alignment,
> it breaks when sufficient barriers to optimization are added that the
> compiler can no longer keep track of where the value came from?
This is getting somewhat theoretical here, but I do not believe that "a
pointer type implies alignment" in C. I may be wrong here, but I cannot
think of any statement in the standards in this regard - it's only data
types that imply alignments. So if this is true, then for pointers gcc
will always have to reason from actual values assigned to them.
> Seems like the right thing is for GCC to warn about the assignment to a
> plain "u32 *", and allow a warningless assignment to something like
> "u32 __attribute__((packed)) *".
Somehow I still feel that gcc should _always_ generate 32-bit accesses
when I dereference a u32 pointer in code. Silently substituting 4 byte
accesses is simply not the right thing to do (maybe with some
-i-know-what-i-am-doing option, but certainly not by default).
If such an access traps the machine, then so be it. Then the code has
to be changed to reflect this in the source code level which will _make
programmers ware of it_. Doing such a "silent fixup" which extends to
other situations thereby changing general semantics of the C language is
not a good thing IMHO.
Cheers
Detlev
--
Modern technique has made it possible for leisure, within limits, to be not
the prerogative of small privileged classes, but a right evenly distributed
throughout the community. The morality of work is the morality of slaves,
and the modern world has no need of slavery.-- Bertrand Russell
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On Tue, 5 Oct 2010 13:43:01 +0200
Detlev Zundel wrote:
> Hi Reinhard,
>
> [...]
>
> >> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
> >> this is *not* an issue with very recent tool chains.
> >
> > OK, for directly adressing elements inside a packed struct;
> > but the original post said:
> >
> > "struct xyz {
> > int x;
> > int y;
> > int z[CONST];
> > } __attribute__ ((packed));
> >
> > struct xyz *abc;
> > u32 * status_reg = (u32 *)&abc->z[0];
> >
> > writel(status, status_reg);"
> >
> > So the "status_reg" pointer is in a completely unrelated (to the packed
> > struct)
> > "u32 *" and still the access is done like it was packed. If the
> > compiler silently drags that attribute along into the "u32 *"
> > THAT is really sick!
>
> I cannot follow why you think this is sick - actually I think this is
> clever. GCC "knows" that abc points to a packed struct and it has no
> clue on actual values, so it _has to assume_ it is not aligned.
So instead of the code breaking in the obvious situation where the
unaligned address is assigned to a pointer type that implies alignment,
it breaks when sufficient barriers to optimization are added that the
compiler can no longer keep track of where the value came from?
Seems like the right thing is for GCC to warn about the assignment to a
plain "u32 *", and allow a warningless assignment to something like
"u32 __attribute__((packed)) *".
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Hi Reinhard,
[...]
>> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> this is *not* an issue with very recent tool chains.
>
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
>
> "struct xyz {
> int x;
> int y;
> int z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);"
>
> So the "status_reg" pointer is in a completely unrelated (to the packed
> struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!
I cannot follow why you think this is sick - actually I think this is
clever. GCC "knows" that abc points to a packed struct and it has no
clue on actual values, so it _has to assume_ it is not aligned.
status_reg is initialized as a pointer to a member of this unaligned
pointer and is thus also considered unaligned. Seems perfectly fine to
me.
Cheers
Detlev
--
BUGS:
It is not yet possible to change operating system by writing to
/proc/sys/kernel/ostype.
-- Linux sysctl(2) man page
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On 10/4/2010 6:13 PM, Reinhard Meyer wrote:
>> > bar:
>> > @ Function supports interworking.
>> > @ args = 0, pretend = 0, frame = 0
>> > @ frame_needed = 0, uses_anonymous_args = 0
>> > @ link register save eliminated.
>> > @ lr needed for prologue
>> > mov r2, #5
>> > mov r3, #4096
>> > str r2, [r3, #0]
>> > bx lr
>> > .size bar, .-bar
>> > .ident "GCC: (GNU) 4.2.2"
>> >
>> >
>> > Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> > this is *not* an issue with very recent tool chains.
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
>
> "struct xyz {
> int x;
> int y;
> int z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);"
>
> So the "status_reg" pointer is in a completely unrelated (to the packed
> struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!
>
Yes, This is exactly what I wanted to point out. I can see this behavior with
gcc-4.5.0 but the same code works well with gcc-4.2.4
Vipin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Wolfgang Denk, Vipin Kumar, > > In message <4ca9acaa.2020...@st.com> you wrote: This writel results in writing byte by byte on the address pointed to by status_reg. This problem is visible with both gcc version 4.4.1 as well as 4.5.0 >>> I bet this is on some ARM system? >> Yes, it is on an ARM system (CortexA9). But I still feel that since I am >> creating >> a new u32 * status_reg, the code should not use any intelligence and use the >> pointer >> only to produce an str instruction in the form >> str r0, [r1] >> >> But it retains the packed property of the structure even with a new u32 >> ponter >> typecasted to u32 * >> u32 * status_reg = (u32 *)xyz->x; >> >> A writel to status_reg results in byte by byte writing How do you know that? Disassembly? Bus snooping? > > I agree with you. I always considered such behaviour of the ARM C > compiler a bug, and still do. However, people with better knowledge > of the ARm architecture than me might be able to explain why the > responsible PTB consider this to be a good and necessary "feature" of > th compiler. Maybe because writel(v,a) is ultimately defined as (*(volatile unsigned int *)(a) = (v)) and the compiler has to assume the worst, i.e. that (a) is not word aligned? Maybe there is a new compiler switch to turn that "pessimisation" off? On the other hand, if what you say were true, all code for AT91 that uses writel() to access 32 bit only peripheral registers would not work with those newer toolchains (still 4.2.4 here). Best Regards, Reinhard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Vipin Kumar, In message <4ca9acaa.2020...@st.com> you wrote: > > >> This writel results in writing byte by byte on the address pointed to by > >> status_reg. > >> This problem is visible with both gcc version 4.4.1 as well as 4.5.0 > > > > I bet this is on some ARM system? > > Yes, it is on an ARM system (CortexA9). But I still feel that since I am > creating > a new u32 * status_reg, the code should not use any intelligence and use the > pointer > only to produce an str instruction in the form > str r0, [r1] > > But it retains the packed property of the structure even with a new u32 > ponter > typecasted to u32 * > u32 * status_reg = (u32 *)xyz->x; > > A writel to status_reg results in byte by byte writing I agree with you. I always considered such behaviour of the ARM C compiler a bug, and still do. However, people with better knowledge of the ARm architecture than me might be able to explain why the responsible PTB consider this to be a good and necessary "feature" of th compiler. > > Hm... Why do these structs have any "__attribute__ ((packed))" at all? > > Even I could not understand that very well Eventually alignment of these structs cannot be guaranteed? 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: w...@denx.de PLEASE NOTE: Some Quantum Physics Theories Suggest That When the Con- sumer Is Not Directly Observing This Product, It May Cease to Exist or Will Exist Only in a Vague and Undetermined State. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On 10/4/2010 4:26 PM, Reinhard Meyer wrote: > Dear Wolfgang Denk, Vipin Kumar, >> >> In message <4ca9acaa.2020...@st.com> you wrote: > This writel results in writing byte by byte on the address pointed to by > status_reg. > This problem is visible with both gcc version 4.4.1 as well as 4.5.0 I bet this is on some ARM system? >>> Yes, it is on an ARM system (CortexA9). But I still feel that since I am >>> creating >>> a new u32 * status_reg, the code should not use any intelligence and use >>> the pointer >>> only to produce an str instruction in the form >>> str r0, [r1] >>> >>> But it retains the packed property of the structure even with a new u32 >>> ponter >>> typecasted to u32 * >>> u32 * status_reg = (u32 *)xyz->x; >>> >>> A writel to status_reg results in byte by byte writing > > How do you know that? Disassembly? Bus snooping? > Disassembly >> >> I agree with you. I always considered such behaviour of the ARM C >> compiler a bug, and still do. However, people with better knowledge >> of the ARm architecture than me might be able to explain why the >> responsible PTB consider this to be a good and necessary "feature" of >> th compiler. > > Maybe because writel(v,a) is ultimately defined as > > (*(volatile unsigned int *)(a) = (v)) > > and the compiler has to assume the worst, i.e. that (a) is not word > aligned? Maybe there is a new compiler switch to turn that > "pessimisation" off? > I agree with you on this. But if I create a new u32 pointer and point to an element with in the packed structure, then what should be the behavior ? > On the other hand, if what you say were true, all code for AT91 that > uses writel() to access 32 bit only peripheral registers would not > work with those newer toolchains (still 4.2.4 here). > Yes, if the at91 code is using packed attribute for device structures Regards Vipin ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On 10/4/2010 3:47 PM, Wolfgang Denk wrote:
> Dear Vipin Kumar,
>
Dear Wolfgang
> In message <4ca9a095.9000...@st.com> you wrote:
>>
>> I encountered a problem something like
>>
>> struct xyz {
>> int x;
>> int y;
>> int z[CONST];
>> } __attribute__ ((packed));
>>
>> struct xyz *abc;
>> u32 * status_reg = (u32 *)&abc->z[0];
>>
>> writel(status, status_reg);
>>
>> This writel results in writing byte by byte on the address pointed to by
>> status_reg.
>> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
>
> I bet this is on some ARM system?
>
Yes, it is on an ARM system (CortexA9). But I still feel that since I am
creating
a new u32 * status_reg, the code should not use any intelligence and use the
pointer
only to produce an str instruction in the form
str r0, [r1]
But it retains the packed property of the structure even with a new u32 ponter
typecasted to u32 *
u32 * status_reg = (u32 *)xyz->x;
A writel to status_reg results in byte by byte writing
>> Incidently, the same code works well with 4.2.4
>
> ...which surprises me. I thought this has always been an ARM
> "feature".
>
>> The problem is visible in the usb host driver which uses the packed
>> structures for
>> accessing device registers.
>
> Hm... Why do these structs have any "__attribute__ ((packed))" at all?
>
Even I could not understand that very well
Regards
Vipin
> Best regards,
>
> Wolfgang Denk
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Balau,
In message you
wrote:
>
> The compiler (4.4.1) generates the expected 32bit store instruction when
> using:
>
> struct p {
> int n;
> } __attribute__ ((packed, aligned(4)));
Confirmed. Thanks for pointing this out.
> In case of hardware registers, I have yet to see a case where this is not
> true.
Indeed, sounds as if this was a solution in case the "packed" should
really be needed - which I seriously doubt.
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: w...@denx.de
Objects in mirror are closer than they appear.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Wolfgang Denk,
> Dear Reinhard Meyer,
>
> In message <4ca9be94.6000...@emk-elektronik.de> you wrote:
>> Do you imply that the code is really different when the pointer gets
>> its value by assigning it NOT to a packed entity? Hard to believe.
>
> This is a special "feature" of GCC on ARM.
>
> -> cat foo.c
> #define writel(v,a) (*(volatile unsigned int *)(a) = (v))
>
> struct p {
> int n;
> } __attribute__ ((packed));
>
> struct q {
> int n;
> };
>
> void foo()
> {
> struct p *pp = (struct p *)0x1000;
>
> pp->n = 5;
> }
>
> void bar()
> {
> struct q *qq = (struct q *)0x1000;
>
> qq->n = 5;
> }
> -> arm-linux-gcc -O -S foo.c
> -> cat foo.s
> .file "foo.c"
> .text
> .align 2
> .global foo
> .type foo, %function
> foo:
> @ Function supports interworking.
> @ args = 0, pretend = 0, frame = 0
> @ frame_needed = 0, uses_anonymous_args = 0
> @ link register save eliminated.
> @ lr needed for prologue
> mov r3, #4096
> mov r2, #0
> orr r1, r2, #5
> strbr1, [r3, #0]
> strbr2, [r3, #1]
> strbr2, [r3, #2]
> strbr2, [r3, #3]
> bx lr
> .size foo, .-foo
> .align 2
> .global bar
> .type bar, %function
In a non-packed struct an int will never be unaligned
(unless you use an unaligned pointer to the whole struct)
In a packed struct an int might be unaligned, so it
_might_ make sense for the compiler to handle that
differently on ARM. Assume you overlay (bad idea anyway)
a packed structure over some communication data stream
thats is byte oriented. On most architectures that would
work (besides obvious endianess issues) but on ARM it would
(without raising an exception) malfunction.
[remember the "display_buffer issue"]
> bar:
> @ Function supports interworking.
> @ args = 0, pretend = 0, frame = 0
> @ frame_needed = 0, uses_anonymous_args = 0
> @ link register save eliminated.
> @ lr needed for prologue
> mov r2, #5
> mov r3, #4096
> str r2, [r3, #0]
> bx lr
> .size bar, .-bar
> .ident "GCC: (GNU) 4.2.2"
>
>
> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
> this is *not* an issue with very recent tool chains.
OK, for directly adressing elements inside a packed struct;
but the original post said:
"struct xyz {
int x;
int y;
int z[CONST];
} __attribute__ ((packed));
struct xyz *abc;
u32 * status_reg = (u32 *)&abc->z[0];
writel(status, status_reg);"
So the "status_reg" pointer is in a completely unrelated (to the packed struct)
"u32 *" and still the access is done like it was packed. If the
compiler silently drags that attribute along into the "u32 *"
THAT is really sick!
Reinhard
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Wolfgang Denk and Reinhard Meyer,
The compiler (4.4.1) generates the expected 32bit store instruction when using:
struct p {
int n;
} __attribute__ ((packed, aligned(4)));
In case of hardware registers, I have yet to see a case where this is not true.
Regards,
Francesco
On Mon, Oct 4, 2010 at 2:43 PM, Reinhard Meyer wrote:
> Dear Wolfgang Denk,
>
>> Dear Reinhard Meyer,
>>
>> In message <4ca9be94.6000...@emk-elektronik.de> you wrote:
>>> Do you imply that the code is really different when the pointer gets
>>> its value by assigning it NOT to a packed entity? Hard to believe.
>>
>> This is a special "feature" of GCC on ARM.
>>
>> -> cat foo.c
>> #define writel(v,a) (*(volatile unsigned int *)(a) = (v))
>>
>> struct p {
>> int n;
>> } __attribute__ ((packed));
>>
>> struct q {
>> int n;
>> };
>>
>> void foo()
>> {
>> struct p *pp = (struct p *)0x1000;
>>
>> pp->n = 5;
>> }
>>
>> void bar()
>> {
>> struct q *qq = (struct q *)0x1000;
>>
>> qq->n = 5;
>> }
>> -> arm-linux-gcc -O -S foo.c
>> -> cat foo.s
>> .file "foo.c"
>> .text
>> .align 2
>> .global foo
>> .type foo, %function
>> foo:
>> @ Function supports interworking.
>> @ args = 0, pretend = 0, frame = 0
>> @ frame_needed = 0, uses_anonymous_args = 0
>> @ link register save eliminated.
>> @ lr needed for prologue
>> mov r3, #4096
>> mov r2, #0
>> orr r1, r2, #5
>> strb r1, [r3, #0]
>> strb r2, [r3, #1]
>> strb r2, [r3, #2]
>> strb r2, [r3, #3]
>> bx lr
>> .size foo, .-foo
>> .align 2
>> .global bar
>> .type bar, %function
>
> In a non-packed struct an int will never be unaligned
> (unless you use an unaligned pointer to the whole struct)
>
> In a packed struct an int might be unaligned, so it
> _might_ make sense for the compiler to handle that
> differently on ARM. Assume you overlay (bad idea anyway)
> a packed structure over some communication data stream
> thats is byte oriented. On most architectures that would
> work (besides obvious endianess issues) but on ARM it would
> (without raising an exception) malfunction.
> [remember the "display_buffer issue"]
>
>> bar:
>> @ Function supports interworking.
>> @ args = 0, pretend = 0, frame = 0
>> @ frame_needed = 0, uses_anonymous_args = 0
>> @ link register save eliminated.
>> @ lr needed for prologue
>> mov r2, #5
>> mov r3, #4096
>> str r2, [r3, #0]
>> bx lr
>> .size bar, .-bar
>> .ident "GCC: (GNU) 4.2.2"
>>
>>
>> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> this is *not* an issue with very recent tool chains.
>
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
>
> "struct xyz {
> int x;
> int y;
> int z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);"
>
> So the "status_reg" pointer is in a completely unrelated (to the packed
> struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!
>
> Reinhard
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Vipin Kumar, In message <4ca9b316.3050...@st.com> you wrote: > > >>> Hm... Why do these structs have any "__attribute__ ((packed))" at all? > >> > >> Even I could not understand that very well > > > > Eventually alignment of these structs cannot be guaranteed? > > In my opinion it can be guaranteed. > btw, I am talking about ehci_hcor structure in include/usb/host/ehci.h > The only reason I am confused is that a lot many platforms would have faced a > similar problem (or is it only me). > > Please confirm if I should remove the packed attribute and send a patch This is mostly a decision Remy has to make (on cc:) > This also raises one doubt. Since u-boot code now contains structures to > access > device registers, using packed attribute with these structures can be lethal On ARM, indeed. Such structures must not use any packed attributes. 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: w...@denx.de About the use of language: it is impossible to sharpen a pencil with a blunt ax. It is equally vain to try to do it with ten blunt axes instead. -- Edsger Dijkstra ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Reinhard Meyer,
In message <4ca9be94.6000...@emk-elektronik.de> you wrote:
>
> Do you imply that the code is really different when the pointer gets
> its value by assigning it NOT to a packed entity? Hard to believe.
This is a special "feature" of GCC on ARM.
-> cat foo.c
#define writel(v,a) (*(volatile unsigned int *)(a) = (v))
struct p {
int n;
} __attribute__ ((packed));
struct q {
int n;
};
void foo()
{
struct p *pp = (struct p *)0x1000;
pp->n = 5;
}
void bar()
{
struct q *qq = (struct q *)0x1000;
qq->n = 5;
}
-> arm-linux-gcc -O -S foo.c
-> cat foo.s
.file "foo.c"
.text
.align 2
.global foo
.type foo, %function
foo:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
@ lr needed for prologue
mov r3, #4096
mov r2, #0
orr r1, r2, #5
strbr1, [r3, #0]
strbr2, [r3, #1]
strbr2, [r3, #2]
strbr2, [r3, #3]
bx lr
.size foo, .-foo
.align 2
.global bar
.type bar, %function
bar:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
@ lr needed for prologue
mov r2, #5
mov r3, #4096
str r2, [r3, #0]
bx lr
.size bar, .-bar
.ident "GCC: (GNU) 4.2.2"
Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
this is *not* an issue with very recent tool chains.
> 2. the culprit on ARM is that unaligned accesses do not raise any signal
> but silently IGNORE the "unused" adress lines which leads to very
> undesirable effects that are hard to find.
Well, the compiler turning a single 32 bit access silently into 4 x 8
bit accesses can also lead to very undesirable effects that are hard
to find, especially when accessing hardware that performs
auto-incrementing or any kind of (IRQ or similar) ACK for each
access.
> *((int *)0x1) = 5 is the same as *((int *)0x10003) = 5 !
> Both write 5 to the word at adress 0x1 !
>
> So, MAYBE, in newer toolchains it was decided to circumvent that problem
> by always assuming unaligned pointers unless clearly instructed otherwise.
No, AFAICT this behaviour has not changed in the last few months / years.
> Nope. It does not have to do with packed. It would have to do with the fact
> that I/O registers cannot be safely written byte-wise.
Yes, it has to do with pcked. See above.
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: w...@denx.de
If a person (a) is poorly, (b) receives treatment intended to make
him better, and (c) gets better, then no power of reasoning known to
medical science can convince him that it may not have been the
treatment that restored his health.
- Sir Peter Medawar, The Art of the Soluble
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Reinhard Meyer, In message <4ca9cc04.5020...@emk-elektronik.de> you wrote: > > In a non-packed struct an int will never be unaligned > (unless you use an unaligned pointer to the whole struct) Which may happen for example when overlaying such a struct on top of some I/O buffer. > In a packed struct an int might be unaligned, so it > _might_ make sense for the compiler to handle that > differently on ARM. Assume you overlay (bad idea anyway) In a packed struct, where the struct itself is located with sufficient alignment, and where all previous elements in the struct are sufficiently aligned (like in the example I gave), it is NOT possible that such unaligment happens. > a packed structure over some communication data stream > thats is byte oriented. On most architectures that would > work (besides obvious endianess issues) but on ARM it would > (without raising an exception) malfunction. Keep in mind that the compiler knows the start address of the struct, and the alignment of the elements. It could easily determine that no unaligned accesses can result. With the same logic you would have to enforce byte accesses for the not-packed structs as well, as theese might be accessed through a not sufficiently aligned pointer. > So the "status_reg" pointer is in a completely unrelated (to the packed > struct) > "u32 *" and still the access is done like it was packed. If the > compiler silently drags that attribute along into the "u32 *" > THAT is really sick! We do agree on this statement. GCC on ARM is a strange beast. 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: w...@denx.de "The whole problem with the world is that fools and fanatics are always so certain of themselves, but wiser people so full of doubts." - Bertrand Russell ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Vipin Kumar, A writel to status_reg results in byte by byte writing >> How do you know that? Disassembly? Bus snooping? >> > > Disassembly > >>> I agree with you. I always considered such behaviour of the ARM C >>> compiler a bug, and still do. However, people with better knowledge >>> of the ARm architecture than me might be able to explain why the >>> responsible PTB consider this to be a good and necessary "feature" of >>> th compiler. >> Maybe because writel(v,a) is ultimately defined as >> >> (*(volatile unsigned int *)(a) = (v)) >> >> and the compiler has to assume the worst, i.e. that (a) is not word >> aligned? Maybe there is a new compiler switch to turn that >> "pessimisation" off? >> > > I agree with you on this. But if I create a new u32 pointer and point to > an element with in the packed structure, then what should be the behavior ? Do you imply that the code is really different when the pointer gets its value by assigning it NOT to a packed entity? Hard to believe. My assumptions: 1. it has to do NOTHING with packed attributes, once the pointer is in a new variable, that "packed" information should be lost. Just try int *ptr = (int *) 0x1; a) *ptr = 5; b) writel (5, ptr); do a) and b) produce different code? 2. the culprit on ARM is that unaligned accesses do not raise any signal but silently IGNORE the "unused" adress lines which leads to very undesirable effects that are hard to find. *((int *)0x1) = 5 is the same as *((int *)0x10003) = 5 ! Both write 5 to the word at adress 0x1 ! So, MAYBE, in newer toolchains it was decided to circumvent that problem by always assuming unaligned pointers unless clearly instructed otherwise. > >> On the other hand, if what you say were true, all code for AT91 that >> uses writel() to access 32 bit only peripheral registers would not >> work with those newer toolchains (still 4.2.4 here). >> > > Yes, if the at91 code is using packed attribute for device structures Nope. It does not have to do with packed. It would have to do with the fact that I/O registers cannot be safely written byte-wise. Reinhard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
Dear Vipin Kumar,
In message <4ca9a095.9000...@st.com> you wrote:
>
> I encountered a problem something like
>
> struct xyz {
> int x;
> int y;
> int z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);
>
> This writel results in writing byte by byte on the address pointed to by
> status_reg.
> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
I bet this is on some ARM system?
> Incidently, the same code works well with 4.2.4
...which surprises me. I thought this has always been an ARM
"feature".
> The problem is visible in the usb host driver which uses the packed
> structures for
> accessing device registers.
Hm... Why do these structs have any "__attribute__ ((packed))" at all?
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: w...@denx.de
If I had to live my life again, I'd make the same mistakes, only
sooner. -- Tallulah Bankhead
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] packed attribute problem
On 10/4/2010 4:14 PM, Wolfgang Denk wrote: > Dear Vipin Kumar, > > In message <4ca9acaa.2020...@st.com> you wrote: >> This writel results in writing byte by byte on the address pointed to by status_reg. This problem is visible with both gcc version 4.4.1 as well as 4.5.0 >>> >>> I bet this is on some ARM system? >> >> Yes, it is on an ARM system (CortexA9). But I still feel that since I am >> creating >> a new u32 * status_reg, the code should not use any intelligence and use the >> pointer >> only to produce an str instruction in the form >> str r0, [r1] >> >> But it retains the packed property of the structure even with a new u32 >> ponter >> typecasted to u32 * >> u32 * status_reg = (u32 *)xyz->x; >> >> A writel to status_reg results in byte by byte writing > > I agree with you. I always considered such behaviour of the ARM C > compiler a bug, and still do. However, people with better knowledge > of the ARm architecture than me might be able to explain why the > responsible PTB consider this to be a good and necessary "feature" of > th compiler. > >>> Hm... Why do these structs have any "__attribute__ ((packed))" at all? >> >> Even I could not understand that very well > > Eventually alignment of these structs cannot be guaranteed? > In my opinion it can be guaranteed. btw, I am talking about ehci_hcor structure in include/usb/host/ehci.h The only reason I am confused is that a lot many platforms would have faced a similar problem (or is it only me). Please confirm if I should remove the packed attribute and send a patch This also raises one doubt. Since u-boot code now contains structures to access device registers, using packed attribute with these structures can be lethal Regards Vipin ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
