linux-mips
[Top] [All Lists]

Re: [PATCH 0/6] mips: diverse Makefile updates

To: Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH 0/6] mips: diverse Makefile updates
From: Sam Ravnborg <sam@ravnborg.org>
Date: Mon, 31 May 2010 12:55:50 +0200
Cc: linux-mips <linux-mips@linux-mips.org>, Wu Zhangjin <wuzhangjin@gmail.com>
In-reply-to: <20100531102954.GA12669@linux-mips.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20100530141939.GA22153@merkur.ravnborg.org> <20100530153939.GA22352@merkur.ravnborg.org> <20100530231954.GA318@linux-mips.org> <20100531102954.GA12669@linux-mips.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, May 31, 2010 at 11:29:54AM +0100, Ralf Baechle wrote:
> On Mon, May 31, 2010 at 12:19:54AM +0100, Ralf Baechle wrote:
> 
> > > > Note: I tried to test a little with bigsur_defconfig
> > > > but get_user() is buggy. Or at least my gcc thinks that
> > > > first argument may be used uninitialized.
> > > > I think mips needs to fix the 64 bit variant of get_user().
> > > > I took a quick look but ran away.
> > > 
> > > My gcc:
> > > mips-linux-gcc (GCC) 4.1.2
> > > Copyright (C) 2006 Free Software Foundation, Inc.
> 
> I played with it for a bit.  The warning is present in all gcc 4.1.0 to
> 4.1.2 and it is bogus.  When I first looked into this years ago I just
> gave up on gcc 4.1 as a newer version was already available.
> 
> The variable returned by get_user is undefined in case of an error, so
> what get_user() is doing is entirely legitimate.  This is different from
> copy_from_user() which in case of an error will clear the remainder of
> the destination area which couldn't not be copied from userspace.

What I looked at:

1)      u32 word;
2)      if (unlikely(get_user(word, header)))
3)              word = 0;
4)      if (word == magic.cmp)
5)              return PAGE_SIZE;

If gcc does not see an assignment to word in line 2) then
it complains about the use line 4).

If we look at the implementation of get_user it is more or less the
following:
({
        int err = -EFAULT;
        if (access_ok(VERIFY_READ, header))
                switch (sizeof(word)) {
                case 4:
                        word = *(u32 *)header;
                        err = 0;
                        break;
                default:
                        __get_user_unknown();
                        break;
                }
        err;
})

Simplified a lot - I know. But it shows my point.
(And simplifying the code also helped me understand the macros).

gcc needs to be smart enough to deduce that we always return != 0
in the cases where word is not assigned - in which case line 3)
will do the assignment.

So gcc is indeed wrong when is sas "uninitialized" but considering
the complexity of these macros I think it is excused.

The x86 version has the following assignment

    (val) = (__typeof__(*(addr))) __gu_tmp; 

unconditionally - so they avoid the " = 0" thing.
sparc has explicit "= 0" assignments.

So refactoring the macros may do the trick too.
But I do not think it is worth the effort.

        Sam

<Prev in Thread] Current Thread [Next in Thread>