[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer
use a union to cause necessary alignment per architecture
Signed-off-by: Reinhard Meyer u-boot@emk-elektronik.de --- lib/display_options.c | 24 +++++++++++++----------- 1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/display_options.c b/lib/display_options.c index 20319e6..4f2ad69 100644 --- a/lib/display_options.c +++ b/lib/display_options.c @@ -101,10 +101,12 @@ 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]; - uint32_t *uip = (void*)linebuf; - uint16_t *usp = (void*)linebuf; - uint8_t *ucp = (void*)linebuf; + /* linebuf as a union causes proper alignment */ + union linebuf { + uint32_t ui[MAX_LINE_LENGTH_BYTES/4 + 1]; + uint16_t us[MAX_LINE_LENGTH_BYTES/2 + 1]; + uint8_t uc[MAX_LINE_LENGTH_BYTES/1 + 1]; + } lb; int i;
if (linelen*width > MAX_LINE_LENGTH_BYTES) @@ -123,21 +125,21 @@ int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen) for (i = 0; i < linelen; i++) { uint32_t x; if (width == 4) - x = uip[i] = *(volatile uint32_t *)data; + x = lb.ui[i] = *(volatile uint32_t *)data; else if (width == 2) - x = usp[i] = *(volatile uint16_t *)data; + x = lb.us[i] = *(volatile uint16_t *)data; else - x = ucp[i] = *(volatile uint8_t *)data; + x = lb.uc[i] = *(volatile uint8_t *)data; printf(" %0*x", width * 2, x); data += width; }
/* Print data in ASCII characters */ for (i = 0; i < linelen * width; i++) - if (!isprint(ucp[i]) || ucp[i] >= 0x80) - ucp[i] = '.'; - ucp[i] = '\0'; - printf(" %s\n", ucp); + if (!isprint(lb.uc[i]) || lb.uc[i] >= 0x80) + lb.uc[i] = '.'; + lb.uc[i] = '\0'; + printf(" %s\n", lb.uc);
/* update references */ addr += linelen * width;
Dear Reinhard Meyer,
In message 1283243000-25427-1-git-send-email-u-boot@emk-elektronik.de you wrote:
use a union to cause necessary alignment per architecture
Signed-off-by: Reinhard Meyer u-boot@emk-elektronik.de
lib/display_options.c | 24 +++++++++++++----------- 1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/display_options.c b/lib/display_options.c index 20319e6..4f2ad69 100644 --- a/lib/display_options.c +++ b/lib/display_options.c @@ -101,10 +101,12 @@ 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];
- uint32_t *uip = (void*)linebuf;
- uint16_t *usp = (void*)linebuf;
- uint8_t *ucp = (void*)linebuf;
- /* linebuf as a union causes proper alignment */
- union linebuf {
uint32_t ui[MAX_LINE_LENGTH_BYTES/4 + 1];uint16_t us[MAX_LINE_LENGTH_BYTES/2 + 1];uint8_t uc[MAX_LINE_LENGTH_BYTES/1 + 1];
Please replace the magic numbers 4, 2 and 1 by respective sizeof().
int i;
if (linelen*width > MAX_LINE_LENGTH_BYTES) @@ -123,21 +125,21 @@ int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen) for (i = 0; i < linelen; i++) { uint32_t x; if (width == 4)
x = uip[i] = *(volatile uint32_t *)data;
x = lb.ui[i] = *(volatile uint32_t *)data; else if (width == 2)
x = usp[i] = *(volatile uint16_t *)data;
x = lb.us[i] = *(volatile uint16_t *)data; else
x = ucp[i] = *(volatile uint8_t *)data;
x = lb.uc[i] = *(volatile uint8_t *)data; printf(" %0*x", width * 2, x); data += width;}
/* Print data in ASCII characters */ for (i = 0; i < linelen * width; i++)
if (!isprint(ucp[i]) || ucp[i] >= 0x80)ucp[i] = '.';ucp[i] = '\0';printf(" %s\n", ucp);
if (!isprint(lb.uc[i]) || lb.uc[i] >= 0x80)lb.uc[i] = '.';
Please fix this old bug: the multi-line statement needs braces.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
- /* linebuf as a union causes proper alignment */
- union linebuf {
uint32_t ui[MAX_LINE_LENGTH_BYTES/4 + 1];uint16_t us[MAX_LINE_LENGTH_BYTES/2 + 1];uint8_t uc[MAX_LINE_LENGTH_BYTES/1 + 1];Please replace the magic numbers 4, 2 and 1 by respective sizeof().
AND
Please let's use the attribute version.
Which shall it be now???
Best Regards, Reinhard
Dear Reinhard Meyer,
In message 4C87065A.5050904@emk-elektronik.de you wrote:
Dear Wolfgang Denk,
- /* linebuf as a union causes proper alignment */
- union linebuf {
uint32_t ui[MAX_LINE_LENGTH_BYTES/4 + 1];uint16_t us[MAX_LINE_LENGTH_BYTES/2 + 1];uint8_t uc[MAX_LINE_LENGTH_BYTES/1 + 1];Please replace the magic numbers 4, 2 and 1 by respective sizeof().
AND
Please let's use the attribute version.
Which shall it be now???
Well, my personal preference would be to use "attribute", but after the long discussion it seems the general preference is the version above so I will not block this. I just ask to change the "magic numbers" 1, 2 and 4 into sizeof() so it's obvious where these are coming from.
Best regards,
Wolfgang Denk
On 08.09.2010 09:48, Wolfgang Denk wrote:
Dear Reinhard Meyer,
In message4C87065A.5050904@emk-elektronik.de you wrote:
Dear Wolfgang Denk,
- /* linebuf as a union causes proper alignment */
- union linebuf {
uint32_t ui[MAX_LINE_LENGTH_BYTES/4 + 1];uint16_t us[MAX_LINE_LENGTH_BYTES/2 + 1];uint8_t uc[MAX_LINE_LENGTH_BYTES/1 + 1];Please replace the magic numbers 4, 2 and 1 by respective sizeof().
AND
Please let's use the attribute version.
Which shall it be now???
Well, my personal preference would be to use "attribute", but after the long discussion it seems the general preference is the version above so I will not block this. I just ask to change the "magic numbers" 1, 2 and 4 into sizeof() so it's obvious where these are coming from.
OK, I will change that, even add the {} at the for() loop.
I am working with too many (non-gnu) C compilers and have seen too many ways for "attribute", "#pragma", etc. So I prefer to do things within the standard "C" language ;)
Reinhard
On Wednesday, September 08, 2010 04:04:37 Reinhard Meyer wrote:
I am working with too many (non-gnu) C compilers and have seen too many ways for "attribute", "#pragma", etc. So I prefer to do things within the standard "C" language ;)
which is entirely why linux/compiler.h exists in the first place and which already provides an attribute wrapper :P -mike
On Tuesday, September 07, 2010 19:33:38 Wolfgang Denk wrote:
Reinhard Meyer wrote:
for (i = 0; i < linelen * width; i++)
if (!isprint(ucp[i]) || ucp[i] >= 0x80)ucp[i] = '.';ucp[i] = '\0';printf(" %s\n", ucp);
if (!isprint(lb.uc[i]) || lb.uc[i] >= 0x80)lb.uc[i] = '.';Please fix this old bug: the multi-line statement needs braces.
which bug ? the logic is: for (...) if (...) ... = ...;
you want the for loop to have explicit braces ? for (...) { if (...) ... = ...; }
which would make it a style "bug", and not a code bug -mike
Dear Mike Frysinger,
In message 201009080020.15801.vapier@gentoo.org you wrote:
which bug ? the logic is: for (...) if (...) ... = ...;
you want the for loop to have explicit braces ? for (...) { if (...) ... = ...; }
which would make it a style "bug", and not a code bug
s/bug/coding style violation/
Best regards,
Wolfgang Denk
On Wednesday, September 08, 2010 01:43:20 Wolfgang Denk wrote:
Mike Frysinger wrote:
which bug ? the logic is: for (...)
if (...)... = ...;
you want the for loop to have explicit braces ?
for (...) {
if (...)... = ...;
}
which would make it a style "bug", and not a code bug
s/bug/coding style violation/
np ... just wanted to make sure i wasnt missing something wrong with the normal running of the code sine i was poking around in there recently. -mike
participants (3)
-
Mike Frysinger -
Reinhard Meyer -
Wolfgang Denk