Patchwork [U-Boot] GCC4.4: Squash multiple warnings due to strict aliasing

login
register
mail settings
Submitter Marek Vasut
Date Sept. 26, 2011, 2:06 a.m.
Message ID <1317002798-27112-1-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/116345/
State Changes Requested
Headers show

Comments

Marek Vasut - Sept. 26, 2011, 2:06 a.m.
Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 arch/arm/include/asm/io.h |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)
Nick Thompson - Sept. 26, 2011, 9:26 a.m.
On 26/09/11 03:06, Marek Vasut wrote:
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  arch/arm/include/asm/io.h |   30 ++++++++++++++++++------------
>  1 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index 1fbc531..61f4987 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr)
>  extern inline void __raw_writesb(unsigned int addr, const void *data, int bytelen)
>  {
>  	uint8_t *buf = (uint8_t *)data;
> -	while(bytelen--)
> -		__arch_putb(*buf++, addr);
> +	int i;
> +	for (i = 0; i < bytelen; i++)
> +		__arch_putb(buf[i], addr);
>  }
>  

This fixes the problem in these use cases, but leaves the door open.

Would it be better to change the __arch_putb macro into an extern inline function
instead which would catch these and future cases?

Nick.
Marek Vasut - Sept. 26, 2011, 9:32 a.m.
On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote:
> On 26/09/11 03:06, Marek Vasut wrote:
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > ---
> > 
> >  arch/arm/include/asm/io.h |   30 ++++++++++++++++++------------
> >  1 files changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> > index 1fbc531..61f4987 100644
> > --- a/arch/arm/include/asm/io.h
> > +++ b/arch/arm/include/asm/io.h
> > @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr)
> > 
> >  extern inline void __raw_writesb(unsigned int addr, const void *data,
> >  int bytelen) {
> >  
> >  	uint8_t *buf = (uint8_t *)data;
> > 
> > -	while(bytelen--)
> > -		__arch_putb(*buf++, addr);
> > +	int i;
> > +	for (i = 0; i < bytelen; i++)
> > +		__arch_putb(buf[i], addr);
> > 
> >  }
> 
> This fixes the problem in these use cases, but leaves the door open.
> 
> Would it be better to change the __arch_putb macro into an extern inline
> function instead which would catch these and future cases?

Yes, but you'll need to do that on a much larger scale. Is anyone up for doing 
it ?

> 
> Nick.
Nick Thompson - Sept. 26, 2011, 9:44 a.m.
On 26/09/11 10:32, Marek Vasut wrote:
> On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote:
>> On 26/09/11 03:06, Marek Vasut wrote:
>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>> ---
>>>
>>>  arch/arm/include/asm/io.h |   30 ++++++++++++++++++------------
>>>  1 files changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
>>> index 1fbc531..61f4987 100644
>>> --- a/arch/arm/include/asm/io.h
>>> +++ b/arch/arm/include/asm/io.h
>>> @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void * vaddr)
>>>
>>>  extern inline void __raw_writesb(unsigned int addr, const void *data,
>>>  int bytelen) {
>>>  
>>>  	uint8_t *buf = (uint8_t *)data;
>>>
>>> -	while(bytelen--)
>>> -		__arch_putb(*buf++, addr);
>>> +	int i;
>>> +	for (i = 0; i < bytelen; i++)
>>> +		__arch_putb(buf[i], addr);
>>>
>>>  }
>> This fixes the problem in these use cases, but leaves the door open.
>>
>> Would it be better to change the __arch_putb macro into an extern inline
>> function instead which would catch these and future cases?
> Yes, but you'll need to do that on a much larger scale. Is anyone up for doing 
> it ?
>
I don't follow that. I found only three (identical) definitions in arm, sparc and sh. In
those three cases __raw_writeb were also (identical) macro 'aliases' for __arch_putb.

I guess you are referring to the testing required for all the boards in those three
arches, or even just arm, with changes to all the (get|set)(b|w|l) cases? Maybe I
see your point now...

Nick.
Marek Vasut - Sept. 26, 2011, 9:48 a.m.
On Monday, September 26, 2011 11:44:22 AM Nick Thompson wrote:
> On 26/09/11 10:32, Marek Vasut wrote:
> > On Monday, September 26, 2011 11:26:51 AM Nick Thompson wrote:
> >> On 26/09/11 03:06, Marek Vasut wrote:
> >>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> >>> ---
> >>> 
> >>>  arch/arm/include/asm/io.h |   30 ++++++++++++++++++------------
> >>>  1 files changed, 18 insertions(+), 12 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> >>> index 1fbc531..61f4987 100644
> >>> --- a/arch/arm/include/asm/io.h
> >>> +++ b/arch/arm/include/asm/io.h
> >>> @@ -78,43 +78,49 @@ static inline phys_addr_t virt_to_phys(void *
> >>> vaddr)
> >>> 
> >>>  extern inline void __raw_writesb(unsigned int addr, const void *data,
> >>>  int bytelen) {
> >>>  
> >>>  	uint8_t *buf = (uint8_t *)data;
> >>> 
> >>> -	while(bytelen--)
> >>> -		__arch_putb(*buf++, addr);
> >>> +	int i;
> >>> +	for (i = 0; i < bytelen; i++)
> >>> +		__arch_putb(buf[i], addr);
> >>> 
> >>>  }
> >> 
> >> This fixes the problem in these use cases, but leaves the door open.
> >> 
> >> Would it be better to change the __arch_putb macro into an extern inline
> >> function instead which would catch these and future cases?
> > 
> > Yes, but you'll need to do that on a much larger scale. Is anyone up for
> > doing it ?
> 
> I don't follow that. I found only three (identical) definitions in arm,
> sparc and sh. In those three cases __raw_writeb were also (identical)
> macro 'aliases' for __arch_putb.

Oh if it's only three arches then that's fine.

> 
> I guess you are referring to the testing required for all the boards in
> those three arches, or even just arm, with changes to all the
> (get|set)(b|w|l) cases? Maybe I see your point now...
> 
> Nick.
Wolfgang Denk - Sept. 26, 2011, 11:30 a.m.
Dear Marek Vasut,

In message <201109261132.43616.marek.vasut@gmail.com> you wrote:
>
> > Would it be better to change the __arch_putb macro into an extern inline
> > function instead which would catch these and future cases?
> 
> Yes, but you'll need to do that on a much larger scale. Is anyone up for doing 
> it ?

What exactly are the advantages, and what the disadvantages?

code size / memory footprint?

Best regards,

Wolfgang Denk
Marek Vasut - Sept. 26, 2011, 5:37 p.m.
On Monday, September 26, 2011 01:30:41 PM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109261132.43616.marek.vasut@gmail.com> you wrote:
> > > Would it be better to change the __arch_putb macro into an extern
> > > inline function instead which would catch these and future cases?
> > 
> > Yes, but you'll need to do that on a much larger scale. Is anyone up for
> > doing it ?
> 
> What exactly are the advantages, and what the disadvantages?
> 
> code size / memory footprint?

I assume converting those to inline function will yield no memory usage growth 
at all.
> 
> Best regards,
> 
> Wolfgang Denk
Wolfgang Denk - Sept. 26, 2011, 6:33 p.m.
Dear Marek Vasut,

In message <201109261937.01263.marek.vasut@gmail.com> you wrote:
>
> > What exactly are the advantages, and what the disadvantages?
> > 
> > code size / memory footprint?
> 
> I assume converting those to inline function will yield no memory usage growth 
> at all.

Please don't assume, measure it.  Thanks.

Best regards,

Wolfgang Denk
Marek Vasut - Sept. 26, 2011, 6:35 p.m.
On Monday, September 26, 2011 08:33:58 PM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109261937.01263.marek.vasut@gmail.com> you wrote:
> > > What exactly are the advantages, and what the disadvantages?
> > > 
> > > code size / memory footprint?
> > 
> > I assume converting those to inline function will yield no memory usage
> > growth at all.
> 
> Please don't assume, measure it.  Thanks.

Will do ... though I'd prefer to have this patch and the other one separate.

Cheers

> 
> Best regards,
> 
> Wolfgang Denk

Patch

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 1fbc531..61f4987 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -78,43 +78,49 @@  static inline phys_addr_t virt_to_phys(void * vaddr)
 extern inline void __raw_writesb(unsigned int addr, const void *data, int bytelen)
 {
 	uint8_t *buf = (uint8_t *)data;
-	while(bytelen--)
-		__arch_putb(*buf++, addr);
+	int i;
+	for (i = 0; i < bytelen; i++)
+		__arch_putb(buf[i], addr);
 }
 
 extern inline void __raw_writesw(unsigned int addr, const void *data, int wordlen)
 {
 	uint16_t *buf = (uint16_t *)data;
-	while(wordlen--)
-		__arch_putw(*buf++, addr);
+	int i;
+	for (i = 0; i < wordlen; i++)
+		__arch_putw(buf[i], addr);
 }
 
 extern inline void __raw_writesl(unsigned int addr, const void *data, int longlen)
 {
 	uint32_t *buf = (uint32_t *)data;
-	while(longlen--)
-		__arch_putl(*buf++, addr);
+	int i;
+	for (i = 0; i < longlen; i++)
+		__arch_putl(buf[i], addr);
 }
 
 extern inline void __raw_readsb(unsigned int addr, void *data, int bytelen)
 {
 	uint8_t *buf = (uint8_t *)data;
-	while(bytelen--)
-		*buf++ = __arch_getb(addr);
+	int i;
+	for (i = 0; i < bytelen; i++)
+		buf[i] = __arch_getb(addr);
 }
 
 extern inline void __raw_readsw(unsigned int addr, void *data, int wordlen)
 {
 	uint16_t *buf = (uint16_t *)data;
-	while(wordlen--)
-		*buf++ = __arch_getw(addr);
+	int i;
+	for (i = 0; i < wordlen; i++)
+		buf[i] = __arch_getw(addr);
 }
 
 extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)
 {
 	uint32_t *buf = (uint32_t *)data;
-	while(longlen--)
-		*buf++ = __arch_getl(addr);
+	int i;
+	for (i = 0; i < longlen; i++)
+		buf[i] = __arch_getl(addr);
 }
 
 #define __raw_writeb(v,a)	__arch_putb(v,a)