Patchwork [U-Boot,7/7] GCC4.6: Squash warning in cmd_mem.c

login
register
mail settings
Submitter Marek Vasut
Date Sept. 26, 2011, 12:26 a.m.
Message ID <1316996766-14248-7-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/116337/
State Accepted
Commit f3b3c3df189f4d35c903c3472619017d0bd1baa7
Headers show

Comments

Marek Vasut - Sept. 26, 2011, 12:26 a.m.
cmd_mem.c: In function ‘do_mem_loop’:
cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
[-Wunused-but-set-variable]

The assigned variable can be removed because the pointers are volatile so
accesses to their addresses are always generated.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 common/cmd_mem.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Mike Frysinger - Sept. 26, 2011, 3:41 a.m.
On Sunday, September 25, 2011 20:26:06 Marek Vasut wrote:
> cmd_mem.c: In function ‘do_mem_loop’:
> cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
> [-Wunused-but-set-variable]
> 
> The assigned variable can be removed because the pointers are volatile so
> accesses to their addresses are always generated.

i think the stores to a var were added to avoid a gcc warning, but if newer 
versions don't warn anymore, should be fine.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
Wolfgang Denk - Sept. 26, 2011, 7:25 a.m.
Dear Mike Frysinger,

In message <201109252341.30328.vapier@gentoo.org> you wrote:
> 
> > The assigned variable can be removed because the pointers are volatile so
> > accesses to their addresses are always generated.
>
> i think the stores to a var were added to avoid a gcc warning, but if newer 
> versions don't warn anymore, should be fine.

Obviously we have to check if older tools still accept this.


Marek, which tool chains did you use for testing?

Best regards,

Wolfgang Denk
Marek Vasut - Sept. 26, 2011, 9:03 a.m.
On Monday, September 26, 2011 09:25:36 AM Wolfgang Denk wrote:
> Dear Mike Frysinger,
> 
> In message <201109252341.30328.vapier@gentoo.org> you wrote:
> > > The assigned variable can be removed because the pointers are volatile
> > > so accesses to their addresses are always generated.
> > 
> > i think the stores to a var were added to avoid a gcc warning, but if
> > newer versions don't warn anymore, should be fine.
> 
> Obviously we have to check if older tools still accept this.
> 

Hi,

as I was never completely sure about this file from the begining, I'm all for 
it.

Btw. who's supposed to merge the rest of the series anyway ?

> 
> Marek, which tool chains did you use for testing?

Hand-built gcc4.6.0+debian patches and gcc4.4.6+debian patches:

arm-linux-gnueabi-gcc-4.4 (Debian 4.4.6-7) 4.4.6
arm-linux-gnueabi-gcc-4.6 (Debian 4.6.0-14) 4.6.1 20110616 (prerelease)

There was someone on the irc who (unsuccessfully) tried to compile uboot with 
gcc 2.xx recently. Maybe we should impose some lower bound for compiler version, 
though I guess 4.4 is too high. Suggestions ?

Cheers
> 
> Best regards,
> 
> Wolfgang Denk
Mike Frysinger - Sept. 26, 2011, 4:10 p.m.
On Monday, September 26, 2011 05:03:46 Marek Vasut wrote:
> On Monday, September 26, 2011 09:25:36 AM Wolfgang Denk wrote:
> > Mike Frysinger wrote:
> > > > The assigned variable can be removed because the pointers are
> > > > volatile so accesses to their addresses are always generated.
> > > 
> > > i think the stores to a var were added to avoid a gcc warning, but if
> > > newer versions don't warn anymore, should be fine.
> > 
> > Obviously we have to check if older tools still accept this.
> 
> Hi,
> 
> as I was never completely sure about this file from the begining, I'm all
> for it.
> 
> Btw. who's supposed to merge the rest of the series anyway ?
> 
> > Marek, which tool chains did you use for testing?
> 
> Hand-built gcc4.6.0+debian patches and gcc4.4.6+debian patches:
> 
> arm-linux-gnueabi-gcc-4.4 (Debian 4.4.6-7) 4.4.6
> arm-linux-gnueabi-gcc-4.6 (Debian 4.6.0-14) 4.6.1 20110616 (prerelease)
> 
> There was someone on the irc who (unsuccessfully) tried to compile uboot
> with gcc 2.xx recently. Maybe we should impose some lower bound for
> compiler version, though I guess 4.4 is too high. Suggestions ?

i think gcc-3.x has been broken for a while but no one has noticed
-mike
Marek Vasut - Sept. 26, 2011, 5:31 p.m.
On Monday, September 26, 2011 06:10:37 PM Mike Frysinger wrote:
> On Monday, September 26, 2011 05:03:46 Marek Vasut wrote:
> > On Monday, September 26, 2011 09:25:36 AM Wolfgang Denk wrote:
> > > Mike Frysinger wrote:
> > > > > The assigned variable can be removed because the pointers are
> > > > > volatile so accesses to their addresses are always generated.
> > > > 
> > > > i think the stores to a var were added to avoid a gcc warning, but if
> > > > newer versions don't warn anymore, should be fine.
> > > 
> > > Obviously we have to check if older tools still accept this.
> > 
> > Hi,
> > 
> > as I was never completely sure about this file from the begining, I'm all
> > for it.
> > 
> > Btw. who's supposed to merge the rest of the series anyway ?
> > 
> > > Marek, which tool chains did you use for testing?
> > 
> > Hand-built gcc4.6.0+debian patches and gcc4.4.6+debian patches:
> > 
> > arm-linux-gnueabi-gcc-4.4 (Debian 4.4.6-7) 4.4.6
> > arm-linux-gnueabi-gcc-4.6 (Debian 4.6.0-14) 4.6.1 20110616 (prerelease)
> > 
> > There was someone on the irc who (unsuccessfully) tried to compile uboot
> > with gcc 2.xx recently. Maybe we should impose some lower bound for
> > compiler version, though I guess 4.4 is too high. Suggestions ?
> 
> i think gcc-3.x has been broken for a while but no one has noticed

I guess we should go the kernel way -- make it 4.2 and be done with it.

> -mike
Wolfgang Denk - Sept. 26, 2011, 6:03 p.m.
Dear Marek Vasut,

In message <201109261931.24045.marek.vasut@gmail.com> you wrote:
>
> > i think gcc-3.x has been broken for a while but no one has noticed
> 
> I guess we should go the kernel way -- make it 4.2 and be done with it.

You are off by one major version.  The kernel README says:

	Make sure you have at least gcc 3.2 available.

Note that this will NOT build any half-way recent U-Boot version.

Best regards,

Wolfgang Denk
Simon Glass - Sept. 26, 2011, 6:05 p.m.
Hi Marek,

On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> cmd_mem.c: In function ‘do_mem_loop’:
> cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
> [-Wunused-but-set-variable]
>
> The assigned variable can be removed because the pointers are volatile so
> accesses to their addresses are always generated.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  common/cmd_mem.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> index 4daa1b3..e84cc4e 100644
> --- a/common/cmd_mem.c
> +++ b/common/cmd_mem.c
> @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
>  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> -       ulong   addr, length, i, junk;
> +       ulong   addr, length, i;
>        int     size;
>        volatile uint   *longp;
>        volatile ushort *shortp;
> @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                        longp = (uint *)addr;
>                        i = length;
>                        while (i-- > 0)
> -                               junk = *longp++;
> +                               *longp++;
>                }
>        }
>        if (size == 2) {
> @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                        shortp = (ushort *)addr;
>                        i = length;
>                        while (i-- > 0)
> -                               junk = *shortp++;
> +                               *shortp++;
>                }
>        }
>        for (;;) {
>                cp = (u_char *)addr;
>                i = length;
>                while (i-- > 0)
> -                       junk = *cp++;
> +                       *cp++;
>        }
>  }

I checked the ARM assembler output and it looks fine.

Acked-by: Simon Glass <sjg@chromium.org>

Re the 'length == 1' case (above your patch) site, it is assigning to
'i' here. Could/should we remove that assignment also?

Regards,
Simon

>
> --
> 1.7.5.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Marek Vasut - Sept. 26, 2011, 6:24 p.m.
On Monday, September 26, 2011 08:05:43 PM Simon Glass wrote:
> Hi Marek,
> 
> On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > cmd_mem.c: In function ‘do_mem_loop’:
> > cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
> > [-Wunused-but-set-variable]
> > 
> > The assigned variable can be removed because the pointers are volatile so
> > accesses to their addresses are always generated.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > ---
> >  common/cmd_mem.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> > index 4daa1b3..e84cc4e 100644
> > --- a/common/cmd_mem.c
> > +++ b/common/cmd_mem.c
> > @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag, int
> > argc, char * const argv[])
> > 
> >  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[]) {
> > -       ulong   addr, length, i, junk;
> > +       ulong   addr, length, i;
> >        int     size;
> >        volatile uint   *longp;
> >        volatile ushort *shortp;
> > @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
> > argc, char * const argv[]) longp = (uint *)addr;
> >                        i = length;
> >                        while (i-- > 0)
> > -                               junk = *longp++;
> > +                               *longp++;
> >                }
> >        }
> >        if (size == 2) {
> > @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
> > argc, char * const argv[]) shortp = (ushort *)addr;
> >                        i = length;
> >                        while (i-- > 0)
> > -                               junk = *shortp++;
> > +                               *shortp++;
> >                }
> >        }
> >        for (;;) {
> >                cp = (u_char *)addr;
> >                i = length;
> >                while (i-- > 0)
> > -                       junk = *cp++;
> > +                       *cp++;
> >        }
> >  }
> 
> I checked the ARM assembler output and it looks fine.
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 
> Re the 'length == 1' case (above your patch) site, it is assigning to
> 'i' here. Could/should we remove that assignment also?

The loop below depends on that ... ?

> 
> Regards,
> Simon
> 
> > --
> > 1.7.5.4
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
Simon Glass - Sept. 26, 2011, 6:29 p.m.
Hi Merek,

On Mon, Sep 26, 2011 at 11:24 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, September 26, 2011 08:05:43 PM Simon Glass wrote:
>> Hi Marek,
>>
>> On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > cmd_mem.c: In function ‘do_mem_loop’:
>> > cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
>> > [-Wunused-but-set-variable]
>> >
>> > The assigned variable can be removed because the pointers are volatile so
>> > accesses to their addresses are always generated.
>> >
>> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> > ---
>> >  common/cmd_mem.c |    8 ++++----
>> >  1 files changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/common/cmd_mem.c b/common/cmd_mem.c
>> > index 4daa1b3..e84cc4e 100644
>> > --- a/common/cmd_mem.c
>> > +++ b/common/cmd_mem.c
>> > @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag, int
>> > argc, char * const argv[])
>> >
>> >  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const
>> > argv[]) {
>> > -       ulong   addr, length, i, junk;
>> > +       ulong   addr, length, i;
>> >        int     size;
>> >        volatile uint   *longp;
>> >        volatile ushort *shortp;
>> > @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
>> > argc, char * const argv[]) longp = (uint *)addr;
>> >                        i = length;
>> >                        while (i-- > 0)
>> > -                               junk = *longp++;
>> > +                               *longp++;
>> >                }
>> >        }
>> >        if (size == 2) {
>> > @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
>> > argc, char * const argv[]) shortp = (ushort *)addr;
>> >                        i = length;
>> >                        while (i-- > 0)
>> > -                               junk = *shortp++;
>> > +                               *shortp++;
>> >                }
>> >        }
>> >        for (;;) {
>> >                cp = (u_char *)addr;
>> >                i = length;
>> >                while (i-- > 0)
>> > -                       junk = *cp++;
>> > +                       *cp++;
>> >        }
>> >  }
>>
>> I checked the ARM assembler output and it looks fine.
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> Re the 'length == 1' case (above your patch) site, it is assigning to
>> 'i' here. Could/should we remove that assignment also?
>
> The loop below depends on that ... ?

Which loop? It doesn't look like the value of i is used?

Regards,
Simon

>
>>
>> Regards,
>> Simon
>>
>> > --
>> > 1.7.5.4
>> >
>> > _______________________________________________
>> > U-Boot mailing list
>> > U-Boot@lists.denx.de
>> > http://lists.denx.de/mailman/listinfo/u-boot
>
Marek Vasut - Sept. 26, 2011, 6:29 p.m.
On Monday, September 26, 2011 08:03:45 PM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109261931.24045.marek.vasut@gmail.com> you wrote:
> > > i think gcc-3.x has been broken for a while but no one has noticed
> > 
> > I guess we should go the kernel way -- make it 4.2 and be done with it.
> 
> You are off by one major version.  The kernel README says:
> 
> 	Make sure you have at least gcc 3.2 available.
> 
> Note that this will NOT build any half-way recent U-Boot version.

It might have been my imagination then ... but 4.2 sounded quite logical to me, 
sorry for confusion.

Anyway, is there still anyone using 3.x ? If there isn't any such users, we 
should simply discard it, add gcc version check and set the lower bound to some 
not-too-buggy version of gcc4.

Best regards,

Marek Vasut
Marek Vasut - Sept. 26, 2011, 6:34 p.m.
On Monday, September 26, 2011 08:29:22 PM Simon Glass wrote:
> Hi Merek,
> 
> On Mon, Sep 26, 2011 at 11:24 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Monday, September 26, 2011 08:05:43 PM Simon Glass wrote:
> >> Hi Marek,
> >> 
> >> On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> > cmd_mem.c: In function ‘do_mem_loop’:
> >> > cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
> >> > [-Wunused-but-set-variable]
> >> > 
> >> > The assigned variable can be removed because the pointers are volatile
> >> > so accesses to their addresses are always generated.
> >> > 
> >> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> >> > ---
> >> >  common/cmd_mem.c |    8 ++++----
> >> >  1 files changed, 4 insertions(+), 4 deletions(-)
> >> > 
> >> > diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> >> > index 4daa1b3..e84cc4e 100644
> >> > --- a/common/cmd_mem.c
> >> > +++ b/common/cmd_mem.c
> >> > @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag, int
> >> > argc, char * const argv[])
> >> > 
> >> >  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const
> >> > argv[]) {
> >> > -       ulong   addr, length, i, junk;
> >> > +       ulong   addr, length, i;
> >> >        int     size;
> >> >        volatile uint   *longp;
> >> >        volatile ushort *shortp;
> >> > @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
> >> > argc, char * const argv[]) longp = (uint *)addr;
> >> >                        i = length;
> >> >                        while (i-- > 0)
> >> > -                               junk = *longp++;
> >> > +                               *longp++;
> >> >                }
> >> >        }
> >> >        if (size == 2) {
> >> > @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
> >> > argc, char * const argv[]) shortp = (ushort *)addr;
> >> >                        i = length;
> >> >                        while (i-- > 0)
> >> > -                               junk = *shortp++;
> >> > +                               *shortp++;
> >> >                }
> >> >        }
> >> >        for (;;) {
> >> >                cp = (u_char *)addr;
> >> >                i = length;
> >> >                while (i-- > 0)
> >> > -                       junk = *cp++;
> >> > +                       *cp++;
> >> >        }
> >> >  }
> >> 
> >> I checked the ARM assembler output and it looks fine.
> >> 
> >> Acked-by: Simon Glass <sjg@chromium.org>
> >> 
> >> Re the 'length == 1' case (above your patch) site, it is assigning to
> >> 'i' here. Could/should we remove that assignment also?
> > 
> > The loop below depends on that ... ?
> 
> Which loop? It doesn't look like the value of i is used?

i = length;
while (i-- > 0)

This loop

> 
> Regards,
> Simon
> 
> >> Regards,
> >> Simon
> >> 
> >> > --
> >> > 1.7.5.4
> >> > 
> >> > _______________________________________________
> >> > U-Boot mailing list
> >> > U-Boot@lists.denx.de
> >> > http://lists.denx.de/mailman/listinfo/u-boot
Wolfgang Denk - Sept. 26, 2011, 6:49 p.m.
Dear Marek Vasut,

In message <201109262029.24934.marek.vasut@gmail.com> you wrote:
>
> Anyway, is there still anyone using 3.x ? If there isn't any such users, we 
> should simply discard it, add gcc version check and set the lower bound to some 
> not-too-buggy version of gcc4.

To be honest: yes, there are users.  We recently had customers where
we had to dig out ELDK 2.1.0 (gcc 2.95) to build thir code.

But obviously these don't use any recent code either.

Best regards,

Wolfgang Denk
Simon Glass - Sept. 26, 2011, 7:01 p.m.
Hi Marek,

On Mon, Sep 26, 2011 at 11:34 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, September 26, 2011 08:29:22 PM Simon Glass wrote:
>> Hi Merek,
>>
>> On Mon, Sep 26, 2011 at 11:24 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > On Monday, September 26, 2011 08:05:43 PM Simon Glass wrote:
>> >> Hi Marek,
>> >>
>> >> On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> >> > cmd_mem.c: In function ‘do_mem_loop’:
>> >> > cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
>> >> > [-Wunused-but-set-variable]
>> >> >
>> >> > The assigned variable can be removed because the pointers are volatile
>> >> > so accesses to their addresses are always generated.
>> >> >
>> >> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> >> > ---
>> >> >  common/cmd_mem.c |    8 ++++----
>> >> >  1 files changed, 4 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/common/cmd_mem.c b/common/cmd_mem.c
>> >> > index 4daa1b3..e84cc4e 100644
>> >> > --- a/common/cmd_mem.c
>> >> > +++ b/common/cmd_mem.c
>> >> > @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag, int
>> >> > argc, char * const argv[])
>> >> >
>> >> >  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const
>> >> > argv[]) {
>> >> > -       ulong   addr, length, i, junk;
>> >> > +       ulong   addr, length, i;
>> >> >        int     size;
>> >> >        volatile uint   *longp;
>> >> >        volatile ushort *shortp;
>> >> > @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
>> >> > argc, char * const argv[]) longp = (uint *)addr;
>> >> >                        i = length;
>> >> >                        while (i-- > 0)
>> >> > -                               junk = *longp++;
>> >> > +                               *longp++;
>> >> >                }
>> >> >        }
>> >> >        if (size == 2) {
>> >> > @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int
>> >> > argc, char * const argv[]) shortp = (ushort *)addr;
>> >> >                        i = length;
>> >> >                        while (i-- > 0)
>> >> > -                               junk = *shortp++;
>> >> > +                               *shortp++;
>> >> >                }
>> >> >        }
>> >> >        for (;;) {
>> >> >                cp = (u_char *)addr;
>> >> >                i = length;
>> >> >                while (i-- > 0)
>> >> > -                       junk = *cp++;
>> >> > +                       *cp++;
>> >> >        }
>> >> >  }
>> >>
>> >> I checked the ARM assembler output and it looks fine.
>> >>
>> >> Acked-by: Simon Glass <sjg@chromium.org>
>> >>
>> >> Re the 'length == 1' case (above your patch) site, it is assigning to
>> >> 'i' here. Could/should we remove that assignment also?
>> >
>> > The loop below depends on that ... ?
>>
>> Which loop? It doesn't look like the value of i is used?
>
> i = length;
> while (i-- > 0)
>
> This loop

The assignment to i I was referring to is here:

	if (length == 1) {
		if (size == 4) {
			longp = (uint *)addr;
			for (;;)
				i = *longp;
                              ^^^ this line

		}
		if (size == 2) {
			shortp = (ushort *)addr;
			for (;;)
				i = *shortp;
                              ^^^ this line

		}
		cp = (u_char *)addr;
		for (;;)
			i = *cp;
                              ^^^ this line

	}

I was wondering if we need to assign to i? The output code appears
unchanged with my compiler if the 'i =' is removed.

Regards,
Simon
Marek Vasut - Sept. 26, 2011, 7:10 p.m.
On Monday, September 26, 2011 09:01:16 PM Simon Glass wrote:
> Hi Marek,
> 
> On Mon, Sep 26, 2011 at 11:34 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Monday, September 26, 2011 08:29:22 PM Simon Glass wrote:
> >> Hi Merek,
> >> 
> >> On Mon, Sep 26, 2011 at 11:24 AM, Marek Vasut <marek.vasut@gmail.com> 
wrote:
> >> > On Monday, September 26, 2011 08:05:43 PM Simon Glass wrote:
> >> >> Hi Marek,
> >> >> 
> >> >> On Sun, Sep 25, 2011 at 5:26 PM, Marek Vasut <marek.vasut@gmail.com> 
wrote:
> >> >> > cmd_mem.c: In function ‘do_mem_loop’:
> >> >> > cmd_mem.c:474:25: warning: variable ‘junk’ set but not used
> >> >> > [-Wunused-but-set-variable]
> >> >> > 
> >> >> > The assigned variable can be removed because the pointers are
> >> >> > volatile so accesses to their addresses are always generated.
> >> >> > 
> >> >> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> >> >> > ---
> >> >> >  common/cmd_mem.c |    8 ++++----
> >> >> >  1 files changed, 4 insertions(+), 4 deletions(-)
> >> >> > 
> >> >> > diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> >> >> > index 4daa1b3..e84cc4e 100644
> >> >> > --- a/common/cmd_mem.c
> >> >> > +++ b/common/cmd_mem.c
> >> >> > @@ -471,7 +471,7 @@ int do_mem_base (cmd_tbl_t *cmdtp, int flag,
> >> >> > int argc, char * const argv[])
> >> >> > 
> >> >> >  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char *
> >> >> > const argv[]) {
> >> >> > -       ulong   addr, length, i, junk;
> >> >> > +       ulong   addr, length, i;
> >> >> >        int     size;
> >> >> >        volatile uint   *longp;
> >> >> >        volatile ushort *shortp;
> >> >> > @@ -518,7 +518,7 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag,
> >> >> > int argc, char * const argv[]) longp = (uint *)addr;
> >> >> >                        i = length;
> >> >> >                        while (i-- > 0)
> >> >> > -                               junk = *longp++;
> >> >> > +                               *longp++;
> >> >> >                }
> >> >> >        }
> >> >> >        if (size == 2) {
> >> >> > @@ -526,14 +526,14 @@ int do_mem_loop (cmd_tbl_t *cmdtp, int flag,
> >> >> > int argc, char * const argv[]) shortp = (ushort *)addr;
> >> >> >                        i = length;
> >> >> >                        while (i-- > 0)
> >> >> > -                               junk = *shortp++;
> >> >> > +                               *shortp++;
> >> >> >                }
> >> >> >        }
> >> >> >        for (;;) {
> >> >> >                cp = (u_char *)addr;
> >> >> >                i = length;
> >> >> >                while (i-- > 0)
> >> >> > -                       junk = *cp++;
> >> >> > +                       *cp++;
> >> >> >        }
> >> >> >  }
> >> >> 
> >> >> I checked the ARM assembler output and it looks fine.
> >> >> 
> >> >> Acked-by: Simon Glass <sjg@chromium.org>
> >> >> 
> >> >> Re the 'length == 1' case (above your patch) site, it is assigning to
> >> >> 'i' here. Could/should we remove that assignment also?
> >> > 
> >> > The loop below depends on that ... ?
> >> 
> >> Which loop? It doesn't look like the value of i is used?
> > 
> > i = length;
> > while (i-- > 0)
> > 
> > This loop
> 
> The assignment to i I was referring to is here:
> 
> 	if (length == 1) {
> 		if (size == 4) {
> 			longp = (uint *)addr;
> 			for (;;)
> 				i = *longp;
>                               ^^^ this line
> 
> 		}
> 		if (size == 2) {
> 			shortp = (ushort *)addr;
> 			for (;;)
> 				i = *shortp;
>                               ^^^ this line
> 
> 		}
> 		cp = (u_char *)addr;
> 		for (;;)
> 			i = *cp;
>                               ^^^ this line
> 
> 	}
> 
> I was wondering if we need to assign to i? The output code appears
> unchanged with my compiler if the 'i =' is removed.

Oh, right ... this can be removed. That code seems quite legacy and in a urgent 
need of cleanup. Shall we wrap this change into this patch or do a subsequent 
one ?

Cheers
> 
> Regards,
> Simon
Simon Glass - Sept. 26, 2011, 7:13 p.m.
Hi Marek,

On Mon, Sep 26, 2011 at 12:10 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, September 26, 2011 09:01:16 PM Simon Glass wrote:
>> Hi Marek,
>>
>>
>> The assignment to i I was referring to is here:
>>
>>       if (length == 1) {
>>               if (size == 4) {
>>                       longp = (uint *)addr;
>>                       for (;;)
>>                               i = *longp;
>>                               ^^^ this line
>>
>>               }
>>               if (size == 2) {
>>                       shortp = (ushort *)addr;
>>                       for (;;)
>>                               i = *shortp;
>>                               ^^^ this line
>>
>>               }
>>               cp = (u_char *)addr;
>>               for (;;)
>>                       i = *cp;
>>                               ^^^ this line
>>
>>       }
>>
>> I was wondering if we need to assign to i? The output code appears
>> unchanged with my compiler if the 'i =' is removed.
>
> Oh, right ... this can be removed. That code seems quite legacy and in a urgent
> need of cleanup. Shall we wrap this change into this patch or do a subsequent
> one ?

I'm sure you know better than me, but it feels like a separate commit
if only because your commit msg is about GCC 4.6 warnings.

Regards,
Simon

>
> Cheers
Mike Frysinger - Sept. 26, 2011, 7:52 p.m.
On Monday, September 26, 2011 14:29:24 Marek Vasut wrote:
> On Monday, September 26, 2011 08:03:45 PM Wolfgang Denk wrote:
> > Marek Vasut wrote:
> > > > i think gcc-3.x has been broken for a while but no one has noticed
> > > 
> > > I guess we should go the kernel way -- make it 4.2 and be done with it.
> > 
> > You are off by one major version.  The kernel README says:
> > 	Make sure you have at least gcc 3.2 available.
> > 
> > Note that this will NOT build any half-way recent U-Boot version.
> 
> It might have been my imagination then ... but 4.2 sounded quite logical to
> me, sorry for confusion.
> 
> Anyway, is there still anyone using 3.x ? If there isn't any such users, we
> should simply discard it, add gcc version check and set the lower bound to
> some not-too-buggy version of gcc4.

we had someone post a fix for using gcc-3.x in current master
-mike
Wolfgang Denk - Oct. 1, 2011, 9:27 p.m.
Dear Marek Vasut,

In message <1316996766-14248-7-git-send-email-marek.vasut@gmail.com> you wrote:
> cmd_mem.c: In function `do_mem_loop´:
> cmd_mem.c:474:25: warning: variable `junk´ set but not used
> [-Wunused-but-set-variable]
> 
> The assigned variable can be removed because the pointers are volatile so
> accesses to their addresses are always generated.
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  common/cmd_mem.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Patch

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index 4daa1b3..e84cc4e 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -471,7 +471,7 @@  int do_mem_base (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	ulong	addr, length, i, junk;
+	ulong	addr, length, i;
 	int	size;
 	volatile uint	*longp;
 	volatile ushort *shortp;
@@ -518,7 +518,7 @@  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			longp = (uint *)addr;
 			i = length;
 			while (i-- > 0)
-				junk = *longp++;
+				*longp++;
 		}
 	}
 	if (size == 2) {
@@ -526,14 +526,14 @@  int do_mem_loop (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			shortp = (ushort *)addr;
 			i = length;
 			while (i-- > 0)
-				junk = *shortp++;
+				*shortp++;
 		}
 	}
 	for (;;) {
 		cp = (u_char *)addr;
 		i = length;
 		while (i-- > 0)
-			junk = *cp++;
+			*cp++;
 	}
 }