[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