* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
@ 2010-08-27 20:23 Reinhard Meyer
2010-08-30 8:59 ` Detlev Zundel
0 siblings, 1 reply; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-27 20:23 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-27 20:23 [U-Boot] [PATCH] display_buffer: fix misaligned buffer Reinhard Meyer
@ 2010-08-30 8:59 ` Detlev Zundel
2010-08-30 9:22 ` Reinhard Meyer
0 siblings, 1 reply; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 8:59 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 8:59 ` Detlev Zundel
@ 2010-08-30 9:22 ` Reinhard Meyer
2010-08-30 9:39 ` Reinhard Meyer
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-30 9:22 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 9:22 ` Reinhard Meyer
@ 2010-08-30 9:39 ` Reinhard Meyer
2010-08-30 10:02 ` Detlev Zundel
2010-09-07 23:23 ` Wolfgang Denk
2010-08-30 9:49 ` Detlev Zundel
2010-09-07 23:22 ` Wolfgang Denk
2 siblings, 2 replies; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-30 9:39 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 9:22 ` Reinhard Meyer
2010-08-30 9:39 ` Reinhard Meyer
@ 2010-08-30 9:49 ` Detlev Zundel
2010-09-07 23:22 ` Wolfgang Denk
2 siblings, 0 replies; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 9:49 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 9:39 ` Reinhard Meyer
@ 2010-08-30 10:02 ` Detlev Zundel
2010-08-30 10:31 ` Stefano Babic
2010-09-07 23:23 ` Wolfgang Denk
1 sibling, 1 reply; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 10:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 10:02 ` Detlev Zundel
@ 2010-08-30 10:31 ` Stefano Babic
2010-08-30 10:46 ` Albert ARIBAUD
2010-08-30 11:05 ` Detlev Zundel
0 siblings, 2 replies; 23+ messages in thread
From: Stefano Babic @ 2010-08-30 10:31 UTC (permalink / raw)
To: u-boot
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
=====================================================================
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 10:31 ` Stefano Babic
@ 2010-08-30 10:46 ` Albert ARIBAUD
2010-08-30 11:04 ` Reinhard Meyer
2010-08-30 11:05 ` Detlev Zundel
1 sibling, 1 reply; 23+ messages in thread
From: Albert ARIBAUD @ 2010-08-30 10:46 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 10:46 ` Albert ARIBAUD
@ 2010-08-30 11:04 ` Reinhard Meyer
0 siblings, 0 replies; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-30 11:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 10:31 ` Stefano Babic
2010-08-30 10:46 ` Albert ARIBAUD
@ 2010-08-30 11:05 ` Detlev Zundel
2010-08-30 13:37 ` Reinhard Meyer
1 sibling, 1 reply; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 11:05 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 11:05 ` Detlev Zundel
@ 2010-08-30 13:37 ` Reinhard Meyer
2010-08-30 16:47 ` Detlev Zundel
0 siblings, 1 reply; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-30 13:37 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 13:37 ` Reinhard Meyer
@ 2010-08-30 16:47 ` Detlev Zundel
2010-08-30 18:03 ` Albert ARIBAUD
0 siblings, 1 reply; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 16:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 16:47 ` Detlev Zundel
@ 2010-08-30 18:03 ` Albert ARIBAUD
2010-08-30 18:25 ` Reinhard Meyer
2010-08-30 22:29 ` Detlev Zundel
0 siblings, 2 replies; 23+ messages in thread
From: Albert ARIBAUD @ 2010-08-30 18:03 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 18:03 ` Albert ARIBAUD
@ 2010-08-30 18:25 ` Reinhard Meyer
2010-08-30 22:32 ` Detlev Zundel
2010-08-30 22:29 ` Detlev Zundel
1 sibling, 1 reply; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-30 18:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 18:03 ` Albert ARIBAUD
2010-08-30 18:25 ` Reinhard Meyer
@ 2010-08-30 22:29 ` Detlev Zundel
2010-08-31 5:38 ` Albert ARIBAUD
1 sibling, 1 reply; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 22:29 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 18:25 ` Reinhard Meyer
@ 2010-08-30 22:32 ` Detlev Zundel
0 siblings, 0 replies; 23+ messages in thread
From: Detlev Zundel @ 2010-08-30 22:32 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 22:29 ` Detlev Zundel
@ 2010-08-31 5:38 ` Albert ARIBAUD
2010-08-31 6:04 ` Reinhard Meyer
0 siblings, 1 reply; 23+ messages in thread
From: Albert ARIBAUD @ 2010-08-31 5:38 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-31 5:38 ` Albert ARIBAUD
@ 2010-08-31 6:04 ` Reinhard Meyer
2010-09-01 15:01 ` Detlev Zundel
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Reinhard Meyer @ 2010-08-31 6:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-31 6:04 ` Reinhard Meyer
@ 2010-09-01 15:01 ` Detlev Zundel
2010-09-02 7:39 ` Wolfgang Denk
2010-09-02 17:42 ` Mike Frysinger
2 siblings, 0 replies; 23+ messages in thread
From: Detlev Zundel @ 2010-09-01 15:01 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-31 6:04 ` Reinhard Meyer
2010-09-01 15:01 ` Detlev Zundel
@ 2010-09-02 7:39 ` Wolfgang Denk
2010-09-02 17:42 ` Mike Frysinger
2 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Denk @ 2010-09-02 7:39 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-31 6:04 ` Reinhard Meyer
2010-09-01 15:01 ` Detlev Zundel
2010-09-02 7:39 ` Wolfgang Denk
@ 2010-09-02 17:42 ` Mike Frysinger
2 siblings, 0 replies; 23+ messages in thread
From: Mike Frysinger @ 2010-09-02 17:42 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 9:22 ` Reinhard Meyer
2010-08-30 9:39 ` Reinhard Meyer
2010-08-30 9:49 ` Detlev Zundel
@ 2010-09-07 23:22 ` Wolfgang Denk
2 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Denk @ 2010-09-07 23:22 UTC (permalink / raw)
To: u-boot
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!"
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH] display_buffer: fix misaligned buffer
2010-08-30 9:39 ` Reinhard Meyer
2010-08-30 10:02 ` Detlev Zundel
@ 2010-09-07 23:23 ` Wolfgang Denk
1 sibling, 0 replies; 23+ messages in thread
From: Wolfgang Denk @ 2010-09-07 23:23 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2010-09-07 23:23 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-27 20:23 [U-Boot] [PATCH] display_buffer: fix misaligned buffer Reinhard Meyer
2010-08-30 8:59 ` Detlev Zundel
2010-08-30 9:22 ` Reinhard Meyer
2010-08-30 9:39 ` Reinhard Meyer
2010-08-30 10:02 ` Detlev Zundel
2010-08-30 10:31 ` Stefano Babic
2010-08-30 10:46 ` Albert ARIBAUD
2010-08-30 11:04 ` Reinhard Meyer
2010-08-30 11:05 ` Detlev Zundel
2010-08-30 13:37 ` Reinhard Meyer
2010-08-30 16:47 ` Detlev Zundel
2010-08-30 18:03 ` Albert ARIBAUD
2010-08-30 18:25 ` Reinhard Meyer
2010-08-30 22:32 ` Detlev Zundel
2010-08-30 22:29 ` Detlev Zundel
2010-08-31 5:38 ` Albert ARIBAUD
2010-08-31 6:04 ` Reinhard Meyer
2010-09-01 15:01 ` Detlev Zundel
2010-09-02 7:39 ` Wolfgang Denk
2010-09-02 17:42 ` Mike Frysinger
2010-09-07 23:23 ` Wolfgang Denk
2010-08-30 9:49 ` Detlev Zundel
2010-09-07 23:22 ` Wolfgang Denk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.