[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Reinhard Meyer]

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;
-- 
1.5.6.5

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Wolfgang Denk]

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

-- 
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: wd at denx.de
It is surely a great calamity for  a  human  being  to  have  no  ob-
sessions.                                                - Robert Bly

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Reinhard Meyer]

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

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Mike Frysinger]

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
-------------- 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/20100908/3fe215e0/attachment.pgp 

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Wolfgang Denk]

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

-- 
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: wd at denx.de
All repairs tend to destroy the structure, to  increase  the  entropy
and  disorder  of the system. Less and less effort is spent on fixing
original design flaws; more and more is spent on fixing flaws  intro-
duced by earlier fixes.       - Fred Brooks, "The Mythical Man Month"

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Mike Frysinger]

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
-------------- 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/20100908/adf3f6e5/attachment.pgp 

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Wolfgang Denk]

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

-- 
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: wd at denx.de
Hello! I'm from outer space,  and I've made myself look like a signa-
ture.  While  you  are reading this, I'm having sex with your eyes. I
know it feels good to you, because you're smiling. I'm very horny, so
send me to someone else when you've had enough. Thanks!
                              Sincerely, A Stranger in a Strange Land

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Reinhard Meyer]

On 08.09.2010 09:48, Wolfgang Denk wrote:
> 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.

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

[U-Boot] [PATCH v2] display_buffer: fix misaligned buffer

[Mike Frysinger]

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
-------------- 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/20100908/f4959350/attachment.pgp