[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: svn commit: trunk/busybox/coreutils
From:       Rob Landley <rob () landley ! net>
Date:       2006-09-23 19:59:18
Message-ID: 200609231559.18588.rob () landley ! net
[Download RAW message or body]

On Saturday 23 September 2006 7:56 am, Bernhard Fischer wrote:
> On Fri, Sep 22, 2006 at 12:11:59PM -0700, landley@busybox.net wrote:
> > Author: landley
> > Date: 2006-09-22 12:11:59 -0700 (Fri, 22 Sep 2006)
> > New Revision: 16192
> > 
> > Log:
> > Follow-up to 16172: this also doesn't produce a warning for me on gcc 4.1,
> > without having to feed the compiler nonsense.
> 
> You aren't being defiant, are you?

Thanks ever so much.

> Read the code.

I did.

If there's an actual way for it to be used uninitialized (which I can't see) 
then don't add a comment "to humor gcc".  It's either a real problem, or it 
isn't.

> /tmp/busybox/coreutils/uudecode.c: In function 'uudecode_main':
> /tmp/busybox/coreutils/uudecode.c:144: warning: 'decode_fn_ptr' may be
> used uninitialized in this function

Reading the warning is not reading the code.  The code says:

	char *line_ptr = NULL;

        if (strncmp(line, "begin-base64 ", 13) == 0) {
            line_ptr = line + 13;
            decode_fn_ptr = read_base64;
        } else if (strncmp(line, "begin ", 6) == 0) {
            line_ptr = line + 6;
            decode_fn_ptr = read_stduu;
        }

        if (line_ptr) {

No way to get through that with line_ptr != NULL yet decode_fn_ptr 
uninitialized, and the only users of decode_fn_ptr are in the if.  The 
compiler's warning is _wrong_ here.  It's not following logic that requires 
paying attention to two variables to track.

> What version of gcc-4.1 do you have?

Configured 
with: ../configure --prefix=/opt/timesys/toolchains/i686-linux \
--mandir=/opt/timesys/toolchains/i686-linux/share/man \
--infodir=/opt/timesys/toolchains/i686-linux/share/info --enable-shared \
--enable-threads=posix --enable-checking=release --with-system-zlib \
--enable-__cxa_atexit --disable-libunwind-exceptions --enable-libgcj-multifile \
--enable-languages=c,c++ --with-cpu=generic --program-prefix=i686-linux- \
--build=i686-linux --host=i686-linux --target=i686-linux --disable-libgcj \
--disable-libgomp --disable-libmudflap --disable-libssp Thread model: posix
gcc version 4.1.0 20060304 ( 4.1.0-3)

> How do you use gcc-4.1 to compile busybox?

PATH=/opt/timesys/toolchains/i686-linux/bin:$PATH make CONFIG_STATIC=y \ 
CROSS=i686-uclibc-

It's pretty easy to tell when it used the wrong one, because it won't be 
linked against uClibc.

> Reverting that fix in the face of you having added -Werror is not a
> sensible thing to do. YMMV

No, reverting -Werror for the broken gcc 4.1.0 was the sensible thing to do, 
which I did back in July:

http://busybox.net/downloads/patches/svn-15761.patch

> r15761 | landley | 2006-07-31 18:56:17 -0400 (Mon, 31 Jul 2006) | 9 lines
> Changed paths:
> M /trunk/busybox/Rules.mak
...
> 3) Move the -Werror into the gcc 4.0 on i386 test, because gcc
> 4.1 is broken and produces warnings for things that provably aren't
> incorrect.

The warning generator in that version is _broken_, and changing our code to 
humor a broken warning generator is not something I'm interested in doing.

I can also check to make sure it's not added for gcc before 3.x, but those old 
versions aren't exactly a priority.

I haven't been paying attention to 2.95, since it's obsolete.

I'll take another stab at untangling the logic so the compiler can follow it 
more easily, though.  Possibly feeding it the third "else" with a continue to 
make the big if() go away...

Ok, try it now.

Rob
-- 
Never bet against the cheap plastic solution.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]