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

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

[Reinhard Meyer]

Signed-off-by: Reinhard Meyer <u-boot@emk-elektronik.de>
---
 lib/display_options.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/display_options.c b/lib/display_options.c
index 20319e6..9048a8a 100644
--- a/lib/display_options.c
+++ b/lib/display_options.c
@@ -101,7 +101,7 @@ 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 linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
 	uint32_t *uip = (void*)linebuf;
 	uint16_t *usp = (void*)linebuf;
 	uint8_t *ucp = (void*)linebuf;
-- 
1.5.6.5

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

[Detlev Zundel]

Hi Reinhard,

> Signed-off-by: Reinhard Meyer <u-boot@emk-elektronik.de>
> ---
>  lib/display_options.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/display_options.c b/lib/display_options.c
> index 20319e6..9048a8a 100644
> --- a/lib/display_options.c
> +++ b/lib/display_options.c
> @@ -101,7 +101,7 @@ 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 linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>  	uint32_t *uip = (void*)linebuf;
>  	uint16_t *usp = (void*)linebuf;
>  	uint8_t *ucp = (void*)linebuf;

Sorry to jump in here late, but I do not like this change.  How can a
reader of the code who has not followed the discussion here infer that
the datatype is there to ensure alignment?

I am willing to bet at least a few beers that it will not take long
until someone posts a patch changing the datatype back, because
c-strings are bytes.

I would much rather see an alignment attribute, which will _document_
the problem _and_ fix it, instead of only fixing it.

Cheers
  Detlev

-- 
The GNU GPL makes sense in terms of its purpose: freedom and social
solidarity.  Trying to understand it in terms of the goals and values of
open source is like trying understand a CD drive's retractable drawer as
a cupholder.  You can use it for that, but that is not what it was
designed for.                      -- Richard Stallman
--
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: dzu at denx.de

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

[Reinhard Meyer]

Hi Detlev,
>> diff --git a/lib/display_options.c b/lib/display_options.c
>> index 20319e6..9048a8a 100644
>> --- a/lib/display_options.c
>> +++ b/lib/display_options.c
>> @@ -101,7 +101,7 @@ 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 linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>>  	uint32_t *uip = (void*)linebuf;
>>  	uint16_t *usp = (void*)linebuf;
>>  	uint8_t *ucp = (void*)linebuf;
> 
> Sorry to jump in here late, but I do not like this change.  How can a
> reader of the code who has not followed the discussion here infer that
> the datatype is there to ensure alignment?
> 
> I am willing to bet at least a few beers that it will not take long
> until someone posts a patch changing the datatype back, because
> c-strings are bytes.
> 
> I would much rather see an alignment attribute, which will _document_
> the problem _and_ fix it, instead of only fixing it.

One could add a comment above like:
	/*
	 * it is mandatory that linebuf stays uint32_t aligned
	 * since we are going to slide along it with a uint32_t
	 * pointer
	 */
	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];

I personally prefer this above an attribute. Its disputeable but I prefer
to do things with "normal C constructs" where possible. You can already
see from the discussion that __aligned as a toolchain-abstracted
variant (defined in a toolchain header file) or attribute((__aligned__))
as a very toolchain dependant variant shall be used ;)

Anyway, both patches have been offered, any will work for me as long as
I can see ASCII properly on ARM machines...

without patch:
22000000: 41424344 41424344 41424344 41424344    ADCBADCBADCBAV4.
with patch:
22000000: 41424344 41424344 41424344 41424344    DCBADCBADCBADCBA

Reinhard

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

[Reinhard Meyer]

Reinhard Meyer schrieb:
>> +	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>>>  	uint32_t *uip = (void*)linebuf;
>>>  	uint16_t *usp = (void*)linebuf;
>>>  	uint8_t *ucp = (void*)linebuf;
> I personally prefer this above an attribute. Its disputeable but I prefer
> to do things with "normal C constructs" where possible.
Reading this, after it had been sent, a perfect patch
should make the buffer an union:

union {
	uint32_t ui[MAX.../4+1];
	uint16_t us[MAX.../2+1];
	uint8_t  uc[MAX...+1];
} linebuf;

Reinhard

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

[Detlev Zundel]

Hi Reinhard,

> Reinhard Meyer schrieb:
>>> +	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>>>>  	uint32_t *uip = (void*)linebuf;
>>>>  	uint16_t *usp = (void*)linebuf;
>>>>  	uint8_t *ucp = (void*)linebuf;
>> I personally prefer this above an attribute. Its disputeable but I prefer
>> to do things with "normal C constructs" where possible.
> Reading this, after it had been sent, a perfect patch

The quest for the "perfect patch", now that's the spirit ;)  

<completely off-topic>
Very likely it will be easier than the perfect pac-man game[1] and I
hope we don't have such a split screen waiting *lol*
</cot>

> should make the buffer an union:
>
> union {
> 	uint32_t ui[MAX.../4+1];
> 	uint16_t us[MAX.../2+1];
> 	uint8_t  uc[MAX...+1];
> } linebuf;

That also sounds good indeed - it even better documents the intention of
the code so by my own arguments I'd vote for it.  I presume you will
follow up with such a patch once you tested it?

Thanks!
  Detlev

[1] http://en.wikipedia.org/wiki/Pac-Man

-- 
``The number of UNIX installations has grown to 10,
with more expected.''     Unix Programmers Manual -- 1972
The number of UNIX variants has grown to dozens,
with more expected.                               -- 2001
--
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: dzu at denx.de

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

[Stefano Babic]

Detlev Zundel wrote:
> Hi Reinhard,
> 
Hi Reinhard, hi Detlev,

>> should make the buffer an union:
>>
>> union {
>> 	uint32_t ui[MAX.../4+1];
>> 	uint16_t us[MAX.../2+1];
>> 	uint8_t  uc[MAX...+1];
>> } linebuf;
> 
> That also sounds good indeed - it even better documents the intention of
> the code so by my own arguments I'd vote for it.  I presume you will
> follow up with such a patch once you tested it?

I agree this is a better solution as adding a simple comment. Some time
a comment is valid only at the time of the writing, and further patches
could drop its meaning if the comment is not updated, too.

Detlev, regarding the discussion I would only point out that we have to
be sure that such kind of patch will be merged in the current release.
It would be a real pity if a new official realease is published and then
even a simple "md" command does not work on ARM.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

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

[Albert ARIBAUD]

Le 30/08/2010 12:31, Stefano Babic a ?crit :
> Detlev Zundel wrote:
>> Hi Reinhard,
>>
> Hi Reinhard, hi Detlev,
>
>>> should make the buffer an union:
>>>
>>> union {
>>> 	uint32_t ui[MAX.../4+1];
>>> 	uint16_t us[MAX.../2+1];
>>> 	uint8_t  uc[MAX...+1];
>>> } linebuf;
>>
>> That also sounds good indeed - it even better documents the intention of
>> the code so by my own arguments I'd vote for it.  I presume you will
>> follow up with such a patch once you tested it?
>
> I agree this is a better solution as adding a simple comment. Some time
> a comment is valid only at the time of the writing, and further patches
> could drop its meaning if the comment is not updated, too.

Do we have to pick one? I say the code should use union *and* a one-line 
comment should mention how the union enforces the alignment requirement.

Amicalement,
-- 
Albert.

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

[Reinhard Meyer]

Albert ARIBAUD schrieb:
> Le 30/08/2010 12:31, Stefano Babic a ?crit :
>> Detlev Zundel wrote:
>>> Hi Reinhard,
>>>
>> Hi Reinhard, hi Detlev,
>>
>>>> should make the buffer an union:
>>>>
>>>> union {
>>>> 	uint32_t ui[MAX.../4+1];
>>>> 	uint16_t us[MAX.../2+1];
>>>> 	uint8_t  uc[MAX...+1];
>>>> } linebuf;
>>> That also sounds good indeed - it even better documents the intention of
>>> the code so by my own arguments I'd vote for it.  I presume you will
>>> follow up with such a patch once you tested it?
>> I agree this is a better solution as adding a simple comment. Some time
>> a comment is valid only at the time of the writing, and further patches
>> could drop its meaning if the comment is not updated, too.
> 
> Do we have to pick one? I say the code should use union *and* a one-line 
> comment should mention how the union enforces the alignment requirement.

I will do that. Test only on ARM9. Others must try to compile and see no
other arch gives warnings.

Reinhard

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

[Detlev Zundel]

Hi Stefano,

> Detlev, regarding the discussion I would only point out that we have to
> be sure that such kind of patch will be merged in the current release.
> It would be a real pity if a new official realease is published and then
> even a simple "md" command does not work on ARM.

I don't see a problem here.  All proposed patches (with/without
attribute and union) surely fix a bug, so they will go into mainline
when consent is reached on which one to use.  This should well happen
before the pending release on September 12th[1].

Am I misunderstanding anything here?

Cheers
  Detlev

[1] http://www.denx.de/wiki/U-Boot/ReleaseCycle

-- 
Whatever you do will be insignificant,
but it is very important that you do it.
                                        -- Mahatma Gandhi
--
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: dzu at denx.de

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

[Reinhard Meyer]

Detlev Zundel schrieb:

>> Detlev, regarding the discussion I would only point out that we have to
>> be sure that such kind of patch will be merged in the current release.
>> It would be a real pity if a new official realease is published and then
>> even a simple "md" command does not work on ARM.
> 
> I don't see a problem here.  All proposed patches (with/without
> attribute and union) surely fix a bug, so they will go into mainline
> when consent is reached on which one to use.  This should well happen
> before the pending release on September 12th[1].
> 
> Am I misunderstanding anything here?

No... but I would require that the "union" approach would be wanted,
BEFORE I put effort into doing it.

Reinhard

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

[Detlev Zundel]

Hi Reinhard,

> Detlev Zundel schrieb:
>
>>> Detlev, regarding the discussion I would only point out that we have to
>>> be sure that such kind of patch will be merged in the current release.
>>> It would be a real pity if a new official realease is published and then
>>> even a simple "md" command does not work on ARM.
>> 
>> I don't see a problem here.  All proposed patches (with/without
>> attribute and union) surely fix a bug, so they will go into mainline
>> when consent is reached on which one to use.  This should well happen
>> before the pending release on September 12th[1].
>> 
>> Am I misunderstanding anything here?
>
> No... but I would require that the "union" approach would be wanted,
> BEFORE I put effort into doing it.

I'd very much appreciate your effort as I want the solution now that you
did whet my appetite.

Thanks
  Detlev

-- 
The fact is, volatile on data structures is a bug. It's a wart in the C 
language. It shouldn't be used.
                                   -- Linus Torvalds 
--
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: dzu at denx.de

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

[Albert ARIBAUD]

Le 30/08/2010 18:47, Detlev Zundel a ?crit :
> Hi Reinhard,
>
>> Detlev Zundel schrieb:
>>
>>>> Detlev, regarding the discussion I would only point out that we have to
>>>> be sure that such kind of patch will be merged in the current release.
>>>> It would be a real pity if a new official realease is published and then
>>>> even a simple "md" command does not work on ARM.
>>>
>>> I don't see a problem here.  All proposed patches (with/without
>>> attribute and union) surely fix a bug, so they will go into mainline
>>> when consent is reached on which one to use.  This should well happen
>>> before the pending release on September 12th[1].
>>>
>>> Am I misunderstanding anything here?
>>
>> No... but I would require that the "union" approach would be wanted,
>> BEFORE I put effort into doing it.
>
> I'd very much appreciate your effort as I want the solution now that you
> did whet my appetite.

Besides, re: 'fixing with the side-effect of a different thing': I think 
the alignment caused by using an union is not actually a side effect of 
it but an intended effect of it, as the compiler must ensure correct 
alignment of each union member -- on architectures where alignment of 
32-bit ints is unnecessary, the union will not cause undue alignment, 
whereas the __aligned__ attribute would.

Amicalement,
-- 
Albert.

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

[Reinhard Meyer]

On 30.08.2010 20:03, Albert ARIBAUD wrote:
> Le 30/08/2010 18:47, Detlev Zundel a ?crit :
>> Hi Reinhard,
>>
>>> Detlev Zundel schrieb:
>>>
>>>>> Detlev, regarding the discussion I would only point out that we have to
>>>>> be sure that such kind of patch will be merged in the current release.
>>>>> It would be a real pity if a new official realease is published and then
>>>>> even a simple "md" command does not work on ARM.
>>>>
>>>> I don't see a problem here.  All proposed patches (with/without
>>>> attribute and union) surely fix a bug, so they will go into mainline
>>>> when consent is reached on which one to use.  This should well happen
>>>> before the pending release on September 12th[1].
>>>>
>>>> Am I misunderstanding anything here?
>>>
>>> No... but I would require that the "union" approach would be wanted,
>>> BEFORE I put effort into doing it.
>>
>> I'd very much appreciate your effort as I want the solution now that you
>> did whet my appetite.
>
> Besides, re: 'fixing with the side-effect of a different thing': I think
> the alignment caused by using an union is not actually a side effect of
> it but an intended effect of it, as the compiler must ensure correct
> alignment of each union member -- on architectures where alignment of
> 32-bit ints is unnecessary, the union will not cause undue alignment,
> whereas the __aligned__ attribute would.
>
> Amicalement,

I'll provide a patch tomorrow, right now I am not near a LinuX system ;)
Reinhard

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

[Detlev Zundel]

Hi Reinhard,

> I'll provide a patch tomorrow,

Thanks!

> right now I am not near a LinuX system ;)

Well at least you have the comfort a somewhat sensible mail user agent
there ;)

Cheers
  Detlev

-- 
... what [Microsoft] Exchange provides is *like* email, but it is *not* email.
Once you start trying to use it for real email, you find it's broken by design
in a large number of ways.  It makes no  sense for [a company] to require that
you use Exchange for Internet email, because that's not what Exchange does.
                      -- David Woodhouse <1281348164.12908.47.camel@localhost>
--
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: dzu at denx.de

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

[Detlev Zundel]

Hi Albert,

> Le 30/08/2010 18:47, Detlev Zundel a ?crit :
>> Hi Reinhard,
>>
>>> Detlev Zundel schrieb:
>>>
>>>>> Detlev, regarding the discussion I would only point out that we have to
>>>>> be sure that such kind of patch will be merged in the current release.
>>>>> It would be a real pity if a new official realease is published and then
>>>>> even a simple "md" command does not work on ARM.
>>>>
>>>> I don't see a problem here.  All proposed patches (with/without
>>>> attribute and union) surely fix a bug, so they will go into mainline
>>>> when consent is reached on which one to use.  This should well happen
>>>> before the pending release on September 12th[1].
>>>>
>>>> Am I misunderstanding anything here?
>>>
>>> No... but I would require that the "union" approach would be wanted,
>>> BEFORE I put effort into doing it.
>>
>> I'd very much appreciate your effort as I want the solution now that you
>> did whet my appetite.
>
> Besides, re: 'fixing with the side-effect of a different thing': I think 
> the alignment caused by using an union is not actually a side effect of 
> it but an intended effect of it, as the compiler must ensure correct 
> alignment of each union member -- on architectures where alignment of 
> 32-bit ints is unnecessary, the union will not cause undue alignment, 
> whereas the __aligned__ attribute would.

Absolutely and that's why I like the solution.  It clearly states the
intentions of the code.

The 'side effect of another thing' that I was talking about was the
proposed local change of using an uint32_t array for something which
originally was an uint8_t array in order to gain the alignment.

Cheers
  Detlev

-- 
Greenspun's Tenth Rule of Programming: "Any sufficiently complicated C
or Fortran program contains an ad-hoc, informally-specified bug-ridden
slow implementation of half of Common Lisp."
--
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: dzu at denx.de

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

[Albert ARIBAUD]

Le 31/08/2010 00:29, Detlev Zundel a ?crit :
> Hi Albert,
>
>> Le 30/08/2010 18:47, Detlev Zundel a ?crit :
>>> Hi Reinhard,
>>>
>>>> Detlev Zundel schrieb:
>>>>
>>>>>> Detlev, regarding the discussion I would only point out that we have to
>>>>>> be sure that such kind of patch will be merged in the current release.
>>>>>> It would be a real pity if a new official realease is published and then
>>>>>> even a simple "md" command does not work on ARM.
>>>>>
>>>>> I don't see a problem here.  All proposed patches (with/without
>>>>> attribute and union) surely fix a bug, so they will go into mainline
>>>>> when consent is reached on which one to use.  This should well happen
>>>>> before the pending release on September 12th[1].
>>>>>
>>>>> Am I misunderstanding anything here?
>>>>
>>>> No... but I would require that the "union" approach would be wanted,
>>>> BEFORE I put effort into doing it.
>>>
>>> I'd very much appreciate your effort as I want the solution now that you
>>> did whet my appetite.
>>
>> Besides, re: 'fixing with the side-effect of a different thing': I think
>> the alignment caused by using an union is not actually a side effect of
>> it but an intended effect of it, as the compiler must ensure correct
>> alignment of each union member -- on architectures where alignment of
>> 32-bit ints is unnecessary, the union will not cause undue alignment,
>> whereas the __aligned__ attribute would.
>
> Absolutely and that's why I like the solution.  It clearly states the
> intentions of the code.
>
> The 'side effect of another thing' that I was talking about was the
> proposed local change of using an uint32_t array for something which
> originally was an uint8_t array in order to gain the alignment.

I apologize, then. I did not understand your post this way because I was 
considering the uint32_t in the union, which is 'legitimate' as it is 
used for 'md.l'.

Amicalement,
-- 
Albert.

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

[Reinhard Meyer]

Hi,

making the change to the union, I also realized that

/* Copy from memory into linebuf and print hex values */
for (i = 0; i < linelen; i++) {
	uint32_t x;
	if (width == 4)
		x = lb.u32[i] = *(volatile uint32_t *)data;
	else if (width == 2)
		x = lb.u16[i] = *(volatile uint16_t *)data;
	else
		x = lb.u8[i] = *(volatile uint8_t *)data;
	printf(" %0*x", width * 2, x);
	data += width;
}

is still a bit "ugly". What about:

union data {
	u_int32_t *u32;
	u_int16_t *u16;
	u_int8_t *u8;
	void *v;
} dp;
dp.v = data;

then:

/* Copy from memory into linebuf and print hex values */
for (i = 0; i < linelen; i++) {
	if (width == 4)
		x = lb.u32[i] = *(dp.u32)++;
	else if (width == 2)
		x = lb.u16[i] = *(dp.u16)++;
	else
		x = lb.u8[i] = *(dp.u8)++;
	printf(" %0*x", width * 2, x);
}

optionally calling printf inside the if:

/* Copy from memory into linebuf and print hex values */
for (i = 0; i < linelen; i++) {
	if (width == 4)
		printf(" %08x", lb.u32[i] = *(dp.u32)++);
	else if (width == 2)
		printf(" %04x", lb.u16[i] = *(dp.u16)++);
	else
		printf(" %02x", lb.u8[i] = *(dp.u8)++);
}

maybe it would even be more effective to swap for and if:

/* Copy from memory into linebuf and print hex values */
if (width == 4) {
	for (i = 0; i < linelen; i++)
		printf(" %08x", lb.u32[i] = *(dp.u32)++);
} else if (width == 2) {
	for (i = 0; i < linelen; i++)
		printf(" %04x", lb.u16[i] = *(dp.u16)++);
} else {
	for (i = 0; i < linelen; i++)
		printf(" %02x", lb.u8[i] = *(dp.u8)++);
}

Of course, all is purely cosmetic ;)

Reinhard

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

[Detlev Zundel]

Hi Reinhard,

> making the change to the union, I also realized that
>
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	uint32_t x;
> 	if (width == 4)
> 		x = lb.u32[i] = *(volatile uint32_t *)data;
> 	else if (width == 2)
> 		x = lb.u16[i] = *(volatile uint16_t *)data;
> 	else
> 		x = lb.u8[i] = *(volatile uint8_t *)data;
> 	printf(" %0*x", width * 2, x);
> 	data += width;
> }
>
> is still a bit "ugly". What about:
>
> union data {
> 	u_int32_t *u32;
> 	u_int16_t *u16;
> 	u_int8_t *u8;
> 	void *v;
> } dp;
> dp.v = data;
>
> then:
>
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	if (width == 4)
> 		x = lb.u32[i] = *(dp.u32)++;
> 	else if (width == 2)
> 		x = lb.u16[i] = *(dp.u16)++;
> 	else
> 		x = lb.u8[i] = *(dp.u8)++;
> 	printf(" %0*x", width * 2, x);
> }

If this works then I have to admit, I like what I see :)

> optionally calling printf inside the if:
>
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	if (width == 4)
> 		printf(" %08x", lb.u32[i] = *(dp.u32)++);
> 	else if (width == 2)
> 		printf(" %04x", lb.u16[i] = *(dp.u16)++);
> 	else
> 		printf(" %02x", lb.u8[i] = *(dp.u8)++);
> }

Yes, this may speedup the printf also (ok, not much, but hey).

> maybe it would even be more effective to swap for and if:
>
> /* Copy from memory into linebuf and print hex values */
> if (width == 4) {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %08x", lb.u32[i] = *(dp.u32)++);
> } else if (width == 2) {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %04x", lb.u16[i] = *(dp.u16)++);
> } else {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %02x", lb.u8[i] = *(dp.u8)++);
> }

No, please don't, but why not do a switch on width?

> Of course, all is purely cosmetic ;)

Well this time my signature is in order :)

Cheers
  Detlev

-- 
The  mathematician's patterns,  like the  painter's or  the poet's,  must be
beautiful;  the ideas, like the colours or the words, must fit together in a
harmonious way. Beauty is the first test: there is no permanent place in the
world for ugly mathematics.                       -- G. H. Hardy
--
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: dzu at denx.de

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

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C7C9B85.6080202@emk-elektronik.de> you wrote:
> 
> making the change to the union, I also realized that
> 
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	uint32_t x;
> 	if (width == 4)
> 		x = lb.u32[i] = *(volatile uint32_t *)data;
> 	else if (width == 2)
> 		x = lb.u16[i] = *(volatile uint16_t *)data;
> 	else
> 		x = lb.u8[i] = *(volatile uint8_t *)data;
> 	printf(" %0*x", width * 2, x);
> 	data += width;
> }
> 
> is still a bit "ugly". What about:
> 
> union data {
> 	u_int32_t *u32;
> 	u_int16_t *u16;
> 	u_int8_t *u8;
> 	void *v;
> } dp;
> dp.v = data;

This is missing the extremely important "volatile" attribute.

> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	if (width == 4)
> 		x = lb.u32[i] = *(dp.u32)++;
> 	else if (width == 2)
> 		x = lb.u16[i] = *(dp.u16)++;
> 	else
> 		x = lb.u8[i] = *(dp.u8)++;
> 	printf(" %0*x", width * 2, x);
> }
> 
> optionally calling printf inside the if:
> 
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	if (width == 4)
> 		printf(" %08x", lb.u32[i] = *(dp.u32)++);
> 	else if (width == 2)
> 		printf(" %04x", lb.u16[i] = *(dp.u16)++);
> 	else
> 		printf(" %02x", lb.u8[i] = *(dp.u8)++);
> }

This will increase the code size, right?

> maybe it would even be more effective to swap for and if:
> 
> /* Copy from memory into linebuf and print hex values */
> if (width == 4) {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %08x", lb.u32[i] = *(dp.u32)++);
> } else if (width == 2) {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %04x", lb.u16[i] = *(dp.u16)++);
> } else {
> 	for (i = 0; i < linelen; i++)
> 		printf(" %02x", lb.u8[i] = *(dp.u8)++);
> }

This is much harder to read and to understand. NAK.

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
"In the face of entropy and nothingness, you kind of have to  pretend
it's  not  there  if  you  want  to  keep writing good code."
- Karl Lehenbauer

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

[Mike Frysinger]

On Tuesday, August 31, 2010 02:04:53 Reinhard Meyer wrote:
> making the change to the union, I also realized that
> 
> /* Copy from memory into linebuf and print hex values */
> for (i = 0; i < linelen; i++) {
> 	uint32_t x;
> 	if (width == 4)
> 		x = lb.u32[i] = *(volatile uint32_t *)data;
> 	else if (width == 2)
> 		x = lb.u16[i] = *(volatile uint16_t *)data;
> 	else
> 		x = lb.u8[i] = *(volatile uint8_t *)data;
> 	printf(" %0*x", width * 2, x);
> 	data += width;
> }
> 
> is still a bit "ugly". What about:

maybe, but as Wolfgang points out, the whole point of unifying these code 
paths was to shrink code.  re-expanding it just so that the printf is clear is 
not worthwhile imo.  personally (and probably since i'm the one who changed 
the 3xprintf into 1 printf) find the field width version easier to understand.
-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/20100902/50570b16/attachment.pgp 

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

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C7B7C52.1040606@emk-elektronik.de> you wrote:
> Reinhard Meyer schrieb:
> >> +	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
> >>>  	uint32_t *uip = (void*)linebuf;
> >>>  	uint16_t *usp = (void*)linebuf;
> >>>  	uint8_t *ucp = (void*)linebuf;
> > I personally prefer this above an attribute. Its disputeable but I prefer
> > to do things with "normal C constructs" where possible.
> Reading this, after it had been sent, a perfect patch
> should make the buffer an union:
> 
> union {
> 	uint32_t ui[MAX.../4+1];
> 	uint16_t us[MAX.../2+1];
> 	uint8_t  uc[MAX...+1];
> } linebuf;

Sorry, but I do not want to see any of this /4 and /2 stuff.

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
Pain is a thing of the mind.  The mind can be controlled.
	-- Spock, "Operation -- Annihilate!" stardate 3287.2

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

[Detlev Zundel]

Hi Reinhard,

> Hi Detlev,
>>> diff --git a/lib/display_options.c b/lib/display_options.c
>>> index 20319e6..9048a8a 100644
>>> --- a/lib/display_options.c
>>> +++ b/lib/display_options.c
>>> @@ -101,7 +101,7 @@ 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 linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>>>  	uint32_t *uip = (void*)linebuf;
>>>  	uint16_t *usp = (void*)linebuf;
>>>  	uint8_t *ucp = (void*)linebuf;
>> 
>> Sorry to jump in here late, but I do not like this change.  How can a
>> reader of the code who has not followed the discussion here infer that
>> the datatype is there to ensure alignment?
>> 
>> I am willing to bet at least a few beers that it will not take long
>> until someone posts a patch changing the datatype back, because
>> c-strings are bytes.
>> 
>> I would much rather see an alignment attribute, which will _document_
>> the problem _and_ fix it, instead of only fixing it.
>
> One could add a comment above like:
> 	/*
> 	 * it is mandatory that linebuf stays uint32_t aligned
> 	 * since we are going to slide along it with a uint32_t
> 	 * pointer
> 	 */
> 	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];
>
> I personally prefer this above an attribute. Its disputeable but I prefer
> to do things with "normal C constructs" where possible. You can already
> see from the discussion that __aligned as a toolchain-abstracted
> variant (defined in a toolchain header file) or attribute((__aligned__))
> as a very toolchain dependant variant shall be used ;)

Well of course, but we have need for such pragmas anyway:

[dzu at pollux u-boot-testing (master)]$ grep -re '__attribute__[ \t]*((packed' . | wc -l
257

I agree that if we can fix something with "standards", we should do it.
But if the standards do not provide a clean way for something, but
instead requires the "misuse of the side-effect of a different thing",
then I much rather use the a non-standard construct _intended_ for the
problem.  

No comment is neccessary when we use the attribute - this alone is a
positive aspect for me - code should always document itself.  Whenever I
need a comment to describe the intention of a c statement, I rethink
what I try to do.

> Anyway, both patches have been offered, any will work for me as long as
> I can see ASCII properly on ARM machines...
>
> without patch:
> 22000000: 41424344 41424344 41424344 41424344    ADCBADCBADCBAV4.
> with patch:
> 22000000: 41424344 41424344 41424344 41424344    DCBADCBADCBADCBA

Sorry for being so late, but I really prefer the attribute variant.

Cheers
  Detlev

-- 
Those who deny freedom to others deserve it not for themselves.
           -- Abraham Lincoln
--
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: dzu at denx.de

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

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C7B7864.2080607@emk-elektronik.de> you wrote:
> 
> One could add a comment above like:
> 	/*
> 	 * it is mandatory that linebuf stays uint32_t aligned
> 	 * since we are going to slide along it with a uint32_t
> 	 * pointer
> 	 */
> 	uint32_t linebuf[MAX_LINE_LENGTH_BYTES/4 + 1];

...and using the attribute avoids the ugly code and all the 5 lines of
comment because it's self-explanatory.

> I personally prefer this above an attribute. Its disputeable but I prefer
> to do things with "normal C constructs" where possible. You can already
> see from the discussion that __aligned as a toolchain-abstracted
> variant (defined in a toolchain header file) or attribute((__aligned__))
> as a very toolchain dependant variant shall be used ;)
> 
> Anyway, both patches have been offered, any will work for me as long as
> I can see ASCII properly on ARM machines...

Please let's use the attribute version.

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
"355/113 -- Not the famous irrational number PI,  but  an  incredible
simulation!"