display_buffer: fix misaligned buffer
PATCH 1/1 display_buffer: fix misaligned buffer
--- lib/display_options.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
lib/display_options.c +1 -1
@@ -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;
Signature · 1 line
1.5.6.5
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Reinhard,
Quoted 10 lines
Signed-off-by Reinhard Meyer <[email protected]>--- lib/display_options.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)lib/display_options.c +0
-0
lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 8 lines
lib/display_options.c +1
-1@@ -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
Signature · 9 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Detlev,
Quoted 18 lines
lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 13 lines
lib/display_options.c +1
-1@@ -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?
Quoted 6 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
Quoted 6 lines · Reinhard Meyer
Quoted 4 lines
lib/display_options.c +1
-0+uint32_t linebuf[MAX_LINE_LENGTH_BYTES /4 +1 ];lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 3 lines
lib/display_options.c +0
-0uint32_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
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Reinhard,
Quoted 8 lines
Quoted 6 lines · Reinhard Meyer
Quoted 4 lines
lib/display_options.c +1
-0+uint32_t linebuf[MAX_LINE_LENGTH_BYTES /4 +1 ];lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 3 lines
lib/display_options.c +0
-0uint32_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>
Quoted 7 lines
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
Signature · 8 lines
``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
PATCH 1/1 display_buffer: fix misaligned buffer
Quoted 2 lines · Detlev Zundel
Hi Reinhard,
Hi Reinhard, hi Detlev,
Quoted 11 lines
Quoted 7 lines
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
Signature · 5 lines
===================================================================== 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 =====================================================================
PATCH 1/1 display_buffer: fix misaligned buffer
Le 30/08/2010 12:31, Stefano Babic a ?crit :
Quoted 13 lines
Quoted 2 lines · Detlev Zundel
Hi Reinhard,
Hi Reinhard, hi Detlev,
Quoted 8 lines
Quoted 7 lines
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;
Quoted 7 lines
Quoted 3 lines
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,
Signature · 1 line
Albert.
PATCH 1/1 display_buffer: fix misaligned buffer
Quoted 16 lines · Albert ARIBAUD
Le 30/08/2010 12:31, Stefano Babic a ?crit :
Quoted 15 lines
Quoted 2 lines · Detlev Zundel
Hi Reinhard,
Hi Reinhard, hi Detlev,
Quoted 10 lines
Quoted 7 lines
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?
Quoted 6 lines
Quoted 3 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
In reply to Stefano BabicHi Stefano,
Quoted 4 lines
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
Signature · 7 lines
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.dePATCH 1/1 display_buffer: fix misaligned buffer
Quoted 11 lines · Detlev Zundel
Quoted 4 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Reinhard,
Quoted 12 lines
Quoted 10 lines · Detlev Zundel
Quoted 4 lines
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].
Quoted 4 lines
Quoted 1 line
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
Signature · 7 lines
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.dePATCH 1/1 display_buffer: fix misaligned buffer
Le 30/08/2010 18:47, Detlev Zundel a ?crit :
Quoted 14 lines
Hi Reinhard,
Quoted 12 lines
Quoted 10 lines · Detlev Zundel
Quoted 4 lines
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].
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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,
Signature · 1 line
Albert.
PATCH 1/1 display_buffer: fix misaligned buffer
Quoted 23 lines · Albert ARIBAUD
Le 30/08/2010 18:47, Detlev Zundel a ?crit :
Quoted 14 lines
Hi Reinhard,
Quoted 12 lines
Quoted 10 lines · Detlev Zundel
Quoted 4 lines
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].
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 8 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Reinhard,
Quoted 1 line
I'll provide a patch tomorrow,
Thanks!
Quoted 1 line
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
Signature · 9 lines
... 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.dePATCH 1/1 display_buffer: fix misaligned buffer
In reply to Albert ARIBAUDHi Albert,
Quoted 23 lines
Le 30/08/2010 18:47, Detlev Zundel a ?crit :
Quoted 14 lines
Hi Reinhard,
Quoted 12 lines
Quoted 10 lines · Detlev Zundel
Quoted 4 lines
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].
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
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
Signature · 7 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
Le 31/08/2010 00:29, Detlev Zundel a ?crit :
Quoted 32 lines
Hi Albert,
Quoted 23 lines
Le 30/08/2010 18:47, Detlev Zundel a ?crit :
Quoted 14 lines
Hi Reinhard,
Quoted 12 lines
Quoted 10 lines · Detlev Zundel
Quoted 4 lines
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].
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
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.
Quoted 6 lines
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_tarray for something which originally was anuint8_tarray 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,
Signature · 1 line
Albert.
PATCH 1/1 display_buffer: fix misaligned buffer
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
PATCH 1/1 display_buffer: fix misaligned buffer
Hi Reinhard,
Quoted 28 lines
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 */
Quoted 9 lines
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 :)
Quoted 11 lines
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).
Quoted 13 lines
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?
Quoted 1 line
Of course, all is purely cosmetic ;)
Well this time my signature is in order :)
Cheers
Detlev
Signature · 8 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
In reply to Reinhard MeyerDear Reinhard Meyer,
Quoted 18 lines · In message <[email protected]> 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:
Quoted 7 lines
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.
Quoted 14 lines
/* 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 */
Quoted 8 lines
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?
Quoted 13 lines
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
Signature · 6 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
In reply to Reinhard MeyerQuoted 15 lines · 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; }
Quoted 1 line
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
PATCH 1/1 display_buffer: fix misaligned buffer
In reply to Reinhard MeyerDear Reinhard Meyer,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 6 lines · Reinhard Meyer
Quoted 4 lines
lib/display_options.c +1
-0+uint32_t linebuf[MAX_LINE_LENGTH_BYTES /4 +1 ];lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 3 lines
lib/display_options.c +0
-0uint32_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.
Quoted 8 lines
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
Signature · 5 lines
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
PATCH 1/1 display_buffer: fix misaligned buffer
In reply to Reinhard MeyerHi Reinhard,
Quoted 34 lines
Hi Detlev,
Quoted 18 lines
lib/display_options.c void print_size(unsigned long long size, const char *s) · line 107
Quoted 13 lines
lib/display_options.c +1
-1@@ -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?
Quoted 6 lines
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];
Quoted 5 lines
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
__alignedas 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.
Quoted 7 lines
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
Signature · 6 lines
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.dePATCH 1/1 display_buffer: fix misaligned buffer
In reply to Reinhard MeyerDear Reinhard Meyer,
Quoted 8 lines · In message <[email protected]> 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.
Quoted 8 lines
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
__alignedas 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
Signature · 5 lines
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!"