On 08/27/2011 01:54 PM, Michael B?sch wrote:
> On Sat, 27 Aug 2011 13:26:00 -0500
> Larry Finger<[email protected]> wrote:
>
>> CHECK drivers/net/wireless/b43/main.c
>> drivers/net/wireless/b43/main.c +4115 b43_wireless_core_stop(7) warn: variable dereferenced before check 'dev'
>
>> This is next material.
>
> -next, just because the bug is not in the current kernel?
> Or because you think this is harmless?
>
> I'm not sure whether this is harmless. It effectively is that kind
> of bug that triggers dangerous compiler optimizations.
> I think the compiler has some freedom to assume dev can not be NULL when
> the function is entered and thus optimize out the !dev check.
No, the check has to be left in due to the changing of dev in the routine
followed by a 'goto redo'.
According to 'git blame', the commit that added the part that swatch does not
like was
36dbd954 (Michael Buesch 2009-09-04 22:51:29 +0200 4115)
That was when the branch back to redo was added. As your comment says
* Returns the current dev. This might be different from the passed in dev,
* because the core might be gone away while we unlocked the mutex. */
In fact, a bug in the original code is unlikely, but my patch did add one. I
think the code should be
if (!dev)
return NULL;
wl = dev->wl;
redo:
That should work correctly and satisfy swatch. Would that do too much damage to
the compiler's optimization?
Larry
On Sat, 27 Aug 2011 14:15:59 -0500
Larry Finger <[email protected]> wrote:
> On 08/27/2011 01:54 PM, Michael Büsch wrote:
> > On Sat, 27 Aug 2011 13:26:00 -0500
> > Larry Finger<[email protected]> wrote:
> >
> >> CHECK drivers/net/wireless/b43/main.c
> >> drivers/net/wireless/b43/main.c +4115 b43_wireless_core_stop(7) warn: variable dereferenced before check 'dev'
> >
> >> This is next material.
> >
> > -next, just because the bug is not in the current kernel?
> > Or because you think this is harmless?
> >
> > I'm not sure whether this is harmless. It effectively is that kind
> > of bug that triggers dangerous compiler optimizations.
> > I think the compiler has some freedom to assume dev can not be NULL when
> > the function is entered and thus optimize out the !dev check.
>
> No, the check has to be left in due to the changing of dev in the routine
> followed by a 'goto redo'.
I think the compiler is still free to emit the !dev check twice.
Or the other way around: Emit it correctly for the "goto" case and
don't emit it for the function-enter case (and directly check dev->status).
> In fact, a bug in the original code is unlikely, but my patch did add one. I
> think the code should be
>
> if (!dev)
> return NULL;
> wl = dev->wl;
> redo:
yes
--
Greetings, Michael.