Align linebuf to avoid misaligned aliases of it

PATCH New Albert Aribaud 2 participants 11 messages Aug 14, 2010 – Aug 20, 2010 Library code

Share & tools

Albert Aribaud

author

2010-08-14 08:11 UTC #0 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Commit 64419e47518bbba059c80b77558f93ad4804145c aliases the uint16_t usp and uint32_t uip variables in print_buffer() to uint8_t variable linebuf without aligning it to an uint32_t address, thus causing data aborts on ARM when doing md.l on 32-bit wide area (and probably 16-bit wide as well).

Aligning linebuf fixes the issue.

Signed-off-by Albert Aribaud <[email protected]>

---
 lib/display_options.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

lib/display_options.c +2 -1

index 20319e6..c544777 100644
@@ -101,7 +101,8 @@ void print_size(unsigned long long size, const char *s)
 #define DEFAULT_LINE_LENGTH_BYTES (16)
 int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
 {
-	uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+	uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+		__attribute__((__aligned__(sizeof(uint32_t))));
 	uint32_t *uip = (void*)linebuf;
 	uint16_t *usp = (void*)linebuf;
 	uint8_t *ucp = (void*)linebuf;

Signature · 1 line

1.7.1

Mike Frysinger

2010-08-14 08:25 UTC #1 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Quoted 5 lines · Albert Aribaud

Commit ?64419e47518bbba059c80b77558f93ad4804145c aliases the uint16_t usp and uint32_t uip variables in print_buffer() to uint8_t variable linebuf without aligning it to an uint32_t address, thus causing data aborts on ARM when doing md.l on 32-bit wide area (and probably 16-bit wide as well).

hmm, i guess i overlooked that issue. usually i'm pretty good at this stuff. sorry about that.

Quoted 1 line

Aligning linebuf fixes the issue.

i dont believe there are issues with gcc stack aligning for native sizes ... just above that

Quoted 5 lines

?int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen) ?{
- ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+ ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+ ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));

__aligned(sizeof(uint32_t))
-mike

Albert ARIBAUD

author

2010-08-14 08:33 UTC #2 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Le 14/08/2010 10:25, Mike Frysinger a ?crit :

Quoted 8 lines

lib/display_options.c void print_size(unsigned long long size, const char *s) · line 105
Quoted 5 lines
int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen) {
lib/display_options.c +2 -1
-       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+               __attribute__((__aligned__(sizeof(uint32_t))));
__aligned(sizeof(uint32_t))
-mike

I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the difference between __aligned and __aligned__?

Amicalement,

Signature · 1 line

Albert.

Mike Frysinger

2010-08-14 17:42 UTC #3 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Quoted 8 lines · Albert ARIBAUD

Le 14/08/2010 10:25, Mike Frysinger a ?crit :
Quoted 7 lines
Quoted 6 lines
?int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
?{
- ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+ ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+ ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));

Quoted 4 lines

Quoted 1 line
__aligned(sizeof(uint32_t))
I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the difference between __aligned and __aligned__?

i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean "__aligned(sizeof(uint32_t))". the linux compiler headers take care of expanding it to an attribute.
-mike

Albert ARIBAUD

author

2010-08-18 12:49 UTC #4 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Le 14/08/2010 19:42, Mike Frysinger a ?crit :

Quoted 11 lines · Albert ARIBAUD

Quoted 10 lines
Le 14/08/2010 10:25, Mike Frysinger a ?crit :
Quoted 8 lines
lib/display_options.c void print_size(unsigned long long size, const char *s) · line 105
Quoted 6 lines
int print_buffer (ulong addr, void* data, uint width, uint count, uint
linelen)
{
lib/display_options.c +2 -1
-       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+               __attribute__((__aligned__(sizeof(uint32_t))));
__aligned(sizeof(uint32_t))

Quoted 7 lines

Quoted 2 lines
I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the difference between __aligned and __aligned__?
i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean "__aligned(sizeof(uint32_t))". the linux compiler headers take care of expanding it to an attribute.
-mike

The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.

Amicalement,

Signature · 1 line

Albert.

Mike Frysinger

2010-08-18 16:46 UTC #5 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Quoted 12 lines · Albert ARIBAUD

Le 14/08/2010 19:42, Mike Frysinger a ?crit :
Quoted 11 lines · Albert ARIBAUD
Quoted 10 lines
Le 14/08/2010 10:25, Mike Frysinger a ?crit :
Quoted 8 lines
Quoted 6 lines
?int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
?{
- ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+ ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+ ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));
__aligned(sizeof(uint32_t))

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the difference between __aligned and __aligned__?
i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean "__aligned(sizeof(uint32_t))". ?the linux compiler headers take care of expanding it to an attribute.
The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.

you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. maybe that's a new topic to start.
-mike

Albert ARIBAUD

author

2010-08-18 17:46 UTC #6 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Le 18/08/2010 18:46, Mike Frysinger a ?crit :

Quoted 20 lines · Albert ARIBAUD

Quoted 19 lines
Le 14/08/2010 19:42, Mike Frysinger a ?crit :
Quoted 17 lines · Albert ARIBAUD
Quoted 8 lines
Le 14/08/2010 10:25, Mike Frysinger a ?crit :
Quoted 7 lines
Quoted 6 lines
int print_buffer (ulong addr, void* data, uint width, uint count, uint
linelen)
{
lib/display_options.c +2 -1
-       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+               __attribute__((__aligned__(sizeof(uint32_t))));
Quoted 4 lines
Quoted 1 line
__aligned(sizeof(uint32_t))
I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the difference between __aligned and __aligned__?
i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean "__aligned(sizeof(uint32_t))". the linux compiler headers take care of expanding it to an attribute.

Quoted 6 lines

Quoted 1 line
The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.
you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. maybe that's a new topic to start.
-mike

I don't understand why I should introduce a dependency on linux. What is the benefit?

Amicalement,

Signature · 1 line

Albert.

Mike Frysinger

2010-08-18 17:54 UTC #7 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Quoted 7 lines · Albert ARIBAUD

Le 18/08/2010 18:46, Mike Frysinger a ?crit :
Quoted 3 lines
you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. ?maybe that's a new topic to start.
I don't understand why I should introduce a dependency on linux. What is the benefit?

u-boot already includes the header in the source tree, not to mention the fact that this file is already including linux/ctype.h -mike

Albert ARIBAUD

author

2010-08-18 20:36 UTC #8 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Le 18/08/2010 19:54, Mike Frysinger a ?crit :

Quoted 6 lines · Albert ARIBAUD

Quoted 5 lines
Le 18/08/2010 18:46, Mike Frysinger a ?crit :
Quoted 3 lines
you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. maybe that's a new topic to start.

Quoted 6 lines

Quoted 2 lines
I don't understand why I should introduce a dependency on linux. What is the benefit?
u-boot already includes the header in the source tree, not to mention the fact that this file is already including linux/ctype.h -mike

But absolutely no .h or .c file uses __aligned whereas __attribute__((__aligned__())) is used in several places, right?

Amicalement,

Signature · 1 line

Albert.

Mike Frysinger

2010-08-19 05:58 UTC #9 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Quoted 7 lines · On Wednesday, August 18, 2010 16:36:39 Albert ARIBAUD wrote:

Le 18/08/2010 19:54, Mike Frysinger a ?crit :
Quoted 6 lines · Albert ARIBAUD
Quoted 5 lines
Le 18/08/2010 18:46, Mike Frysinger a ?crit :
Quoted 3 lines
you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. maybe that's a new topic to start.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I don't understand why I should introduce a dependency on linux. What is the benefit?
u-boot already includes the header in the source tree, not to mention the fact that this file is already including linux/ctype.h
But absolutely no .h or .c file uses __aligned whereas __attribute__((__aligned__())) is used in several places, right?

that would seem to be the case currently
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100819/dd69ca2f/attachment.pgp

Albert ARIBAUD

author

2010-08-20 06:48 UTC #10 lore raw

PATCH 1/1 Align linebuf to avoid misaligned aliases of it

Le 19/08/2010 07:58, Mike Frysinger a ?crit :

Quoted 11 lines · On Wednesday, August 18, 2010 16:36:39 Albert ARIBAUD wrote:

Quoted 10 lines
Le 18/08/2010 19:54, Mike Frysinger a ?crit :
Quoted 9 lines · Albert ARIBAUD
Quoted 7 lines
Le 18/08/2010 18:46, Mike Frysinger a ?crit :
Quoted 3 lines
you need to include linux/compiler.h first ... but i would have thought this be a header already included globally. maybe that's a new topic to start.
I don't understand why I should introduce a dependency on linux. What is the benefit?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
u-boot already includes the header in the source tree, not to mention the fact that this file is already including linux/ctype.h
But absolutely no .h or .c file uses __aligned whereas __attribute__((__aligned__())) is used in several places, right?
that would seem to be the case currently
-mike

In that case, I'll stick with the currently used syntax for this patch.

If someone feels the need to switch over to __aligned, I think issuing a separate global patch which changes all occurrences makes more sense than using both forms in the source code.

Amicalement,

Signature · 1 line

Albert.