Home » Mailing list archives » freebsd-current » 2009 » June » 22

Re: [PATCH]: if (cond); foo() in firewire

Previous thread: Current: Error on make buildworld - Stop in /usr/src/secure/lib/libcrypto by Alex V. Petrov on Saturday, June 20, 2009 - 9:03 pm. (2 messages)

Next thread: [patch] trivial warning fix for ndis_events by Pawel Worach on Sunday, June 21, 2009 - 2:34 pm. (2 messages)
[view original message]
From: Roman Divacky
Subject: [PATCH]: if (cond); foo() in firewire
Date: Sunday, June 21, 2009 - 1:20 am

hi

is this patch correct? may I commit it?

Index: ../../../dev/firewire/fwdev.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- ../../../dev/firewire/fwdev.c       (revision 194573)
+++ ../../../dev/firewire/fwdev.c       (working copy)
@@ -443,7 +443,7 @@
        xfer->send.pay_len =3D uio->uio_resid;
        if (uio->uio_resid > 0) {
                if ((err =3D uiomove((caddr_t)&xfer->send.payload[0],
-                   uio->uio_resid, uio)));
+                   uio->uio_resid, uio)))
                        goto out;
        }
=20

another bug found by the "useless warnings in clang" ;)

roman

[ message continues ]
[view original message]
From: Gary Jennejohn
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Sunday, June 21, 2009 - 9:23 am

On Sun, 21 Jun 2009 10:20:22 +0200

Certainly looks like it should be corrected.  I'd say go ahead
and commit it in my guise as a former src-committer :)

---
Gary Jennejohn
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Adrian Chadd
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Sunday, June 21, 2009 - 9:04 pm

Is clang also evaluating all subsequent execution paths to tell you
what the change in program flow is? :)

I hate to be the harbinger of evilness, but I'd at least attempt a
cursory glance at the code to make sure subsequent code is doing the
right thing. (It certainly looks like a vanilla userland transfer!)

2c,


Adrian
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Steve Kargl
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Sunday, June 21, 2009 - 9:54 pm

I agree with you.  Nothing like side effects to screw up 
a persons clang.

#include <stdio.h>
#include <stdlib.h>

static int
side_effect(int *i)
{
   *i = 42;
   return 0;
}

int 
main(void)
{
   int i;
   if (side_effect(&i));
   if (i == 42)
      printf("%d\n", i);
   return 0;
}

-- 
Steve
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Andriy Gapon
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Monday, June 22, 2009 - 4:20 am

You confuse me. It is a "vanilla userland transfer", but so?
Current code always goes to "out" label regardless if uimove succeeded or not.
I think the idea was to go "out" only if uimove failed and execute some code


You confuse me, what this has to do with side-effects?
I think that Clang is right - 'if' without "then" is suspicious because either you
have a useless/redundant 'if' statement (as in you example below - just call
side_effect(&i) without putting it under if) or you accidentally put semicolon


-- 
Andriy Gapon
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Adrian Chadd
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Monday, June 22, 2009 - 4:10 pm

Because now you have a code path being run which hasn't been run for
quite a while.

I'm just saying be careful, and don't assume that "clang found a bug".
It found a bad code construct. Changing that bit of code changes the
flow of execution and may change things unexpectedly in later code.
It's the same with any bug - this "found by clang" bug should be
looked at by someone who knows the firewire code and they haven't
replied to this thread. :)

I'm glad clang has this lexical analysis magic. Shouldn't there be
some kind of weird, magical, standalone "lint" program to do this kind
of lexical checking for us? :)


Adrian




Adrian
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Andriy Gapon
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Monday, June 22, 2009 - 4:27 pm

I must agree with you, no other choice.
But my thinking is this: let's fix the obvious typo (I am sure-sure that this is
what it is) and then let any "real" bugs (if any) bite firewire guys the hard way.
I.e. if the choice is between:
1) fix the typo now and potentially provoke dormant bugs;
2) indefinitely wait and don't fix the typo until anybody comes forward and
declares that there are no dormant bugs in the vicinity;


I guess there should be one. But as simple as C language standard is :-) it
seems that even with a magnitude of tools we are bound to only approximate the
perfection.

-- 
Andriy Gapon
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Roman Divacky
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Tuesday, June 23, 2009 - 1:57 am

clang ships with static analysis
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Jung-uk Kim
Subject: Re: [PATCH]: if (cond); foo() in firewire
Date: Tuesday, June 23, 2009 - 10:26 am

Just for the record, it seems this bug was fixed in NetBSD long ago:

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ieee1394/fwdev.c.diff?r1=1.3&r2=1.4

I hope it boosts your confidence level a bit. :-)

Jung-uk Kim
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
Previous thread: Current: Error on make buildworld - Stop in /usr/src/secure/lib/libcrypto by Alex V. Petrov on Saturday, June 20, 2009 - 9:03 pm. (2 messages)

Next thread: [patch] trivial warning fix for ndis_events by Pawel Worach on Sunday, June 21, 2009 - 2:34 pm. (2 messages)