Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wednesday 05 January 2011 14:14:39 Ulrich Spörlein wrote: Now that I'm fairly confident that the stability issues with your.org's VMs have been resolved, I'd like to point you to the new and improved, semi-weekly analyzer runs at http://scan.freebsd.your.org/freebsd-head/ I came across an interesting false positive here: http://scan.freebsd.your.org/freebsd-head/sbin.geom.class/2011-01-15-amd64/report-1aAmgE.html#EndPath | assert((strcmp(type, keyfile) == 0 ctxp != NULL | passbuf == NULL passbufsize == 0) || | (strcmp(type, passfile) == 0 ctxp == NULL | passbuf != NULL passbufsize 0)); | assert(strcmp(type, keyfile) == 0 || passbuf[0] == '\0'); | | Within the expansion of the macro 'assert': | Array access (from variable 'passbuf') results in a null pointer | dereference. I think the problem here is that the analyser allows strcmp(type, keyfile) to return zero in the first assertion and nonzero in the second. This cannot happen of course and the analyser should know that because strcmp has been declared __pure__ in string.h. A workaround in this case would be to merge the two assertions. signature.asc Description: This is a digitally signed message part.
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Sun, 09.01.2011 at 01:13:54 +0100, Jilles Tjoelker wrote: On Wed, Jan 05, 2011 at 10:30:43PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 20:36:53 +0100, Jilles Tjoelker wrote: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Cause IMHO it adds clutter, is noisy and needs to be maintained manually, when we have these computer things that should deduct this by themselves. Yes, but to me it seems the only realistic option of your three. Upstream is unlikely to add IPA to the checker and other kinds of annotation are probably either similar to __dead2 with the same problems and an additional one that gcc does not check it or very specific to a particular complaint from the checker. I have done this in some cases because it leads to better code with gcc (the system version in 9-current). See SVN commit r212508 to bin/sh/parser.c. Although synexpect() and synerror() are static, adding __dead2 to both makes the executable 576 bytes smaller on i386 (these functions are called many times). Adding __dead2 to synexpect() only causes a warning noreturn function does return (it calls synerror()). Adding __dead2 to synerror() only also makes the executable smaller but not as much as adding it to both. Reordering the functions in the file does not help to make gcc see that the functions do not return. This is too bad and really makes me sad. It shouldn't be necessary to hand-hold the compilers like that. Could you try some tests with gcc 4.5 to confirm this is still required? gcc 4.5 still needs it. gcc 4.6 and clang (the compiler) do not need it. (For gcc, used ports gcc and compiled head bin/sh with some patches on stable/8. For clang, used base clang and compiled head bin/sh on head.) Thank you for confirming this, this is good to know. Looks like I need to stop worrying and learn to love the __dead2! :D Uli ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, Jan 05, 2011 at 10:30:43PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 20:36:53 +0100, Jilles Tjoelker wrote: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Cause IMHO it adds clutter, is noisy and needs to be maintained manually, when we have these computer things that should deduct this by themselves. Yes, but to me it seems the only realistic option of your three. Upstream is unlikely to add IPA to the checker and other kinds of annotation are probably either similar to __dead2 with the same problems and an additional one that gcc does not check it or very specific to a particular complaint from the checker. I have done this in some cases because it leads to better code with gcc (the system version in 9-current). See SVN commit r212508 to bin/sh/parser.c. Although synexpect() and synerror() are static, adding __dead2 to both makes the executable 576 bytes smaller on i386 (these functions are called many times). Adding __dead2 to synexpect() only causes a warning noreturn function does return (it calls synerror()). Adding __dead2 to synerror() only also makes the executable smaller but not as much as adding it to both. Reordering the functions in the file does not help to make gcc see that the functions do not return. This is too bad and really makes me sad. It shouldn't be necessary to hand-hold the compilers like that. Could you try some tests with gcc 4.5 to confirm this is still required? gcc 4.5 still needs it. gcc 4.6 and clang (the compiler) do not need it. (For gcc, used ports gcc and compiled head bin/sh with some patches on stable/8. For clang, used base clang and compiled head bin/sh on head.) -- Jilles Tjoelker ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
Den 05/01/2011 kl. 20.36 skrev Jilles Tjoelker: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Because the analyzer is supposed to find bugs. Only the function that really doesn't return should be marked as such. If we begin spewing __dead2's everywhere, it's bound to silence a valid bug somewhere down the line when e.g. a conditional in a print_help() function is changed subtly so it doesn't always reach exit(). Erik
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Thursday 06 January 2011 09:01:09 Erik Cederstrand wrote: Den 05/01/2011 kl. 20.36 skrev Jilles Tjoelker: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Because the analyzer is supposed to find bugs. Only the function that really doesn't return should be marked as such. If we begin spewing __dead2's everywhere, it's bound to silence a valid bug somewhere down the line when e.g. a conditional in a print_help() function is changed subtly so it doesn't always reach exit(). On the other hand you can't really expect the compiler/analyser to know what a procedure in another file does, so in that case you need __dead2 anyway. For procedures in the same file it would be nice if the compiler automatically optimised non-returning calls, but I'm not sure the analyser should do that. If the code relies on the fact that a procedure doesn't return, which is the case here, it's a good thing to declare it as such exactly to prevent bugs from creeping in. You can't add a return statement to a non-returning procedure without the compiler complaining about it and I'm sure the analyser would complain about it as well. So you won't be hiding other bugs by adding __dead2 here and there. signature.asc Description: This is a digitally signed message part.
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
Den 06/01/2011 kl. 20.56 skrev Tijl Coosemans: On Thursday 06 January 2011 09:01:09 Erik Cederstrand wrote: Den 05/01/2011 kl. 20.36 skrev Jilles Tjoelker: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Because the analyzer is supposed to find bugs. Only the function that really doesn't return should be marked as such. If we begin spewing __dead2's everywhere, it's bound to silence a valid bug somewhere down the line when e.g. a conditional in a print_help() function is changed subtly so it doesn't always reach exit(). On the other hand you can't really expect the compiler/analyser to know what a procedure in another file does, so in that case you need __dead2 anyway. [...] I have high expectations of LLVM :-) LLVM already has some knowledge of what's going on in other files (see LTO) so why shouldn't it be able to detect the __noreturn__ ? All the necessary information should be readily available. Erik
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Thu, Jan 06, 2011 at 11:59:07PM +0100, Erik Cederstrand wrote: Den 06/01/2011 kl. 20.56 skrev Tijl Coosemans: On Thursday 06 January 2011 09:01:09 Erik Cederstrand wrote: Den 05/01/2011 kl. 20.36 skrev Jilles Tjoelker: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Sp?rlein wrote: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Because the analyzer is supposed to find bugs. Only the function that really doesn't return should be marked as such. If we begin spewing __dead2's everywhere, it's bound to silence a valid bug somewhere down the line when e.g. a conditional in a print_help() function is changed subtly so it doesn't always reach exit(). On the other hand you can't really expect the compiler/analyser to know what a procedure in another file does, so in that case you need __dead2 anyway. [...] I have high expectations of LLVM :-) LLVM already has some knowledge of what's going on in other files (see LTO) so why shouldn't it be able to detect the __noreturn__ ? All the necessary information should be readily available. the static analyzer has nothing to do with LLVM. it's a clang component and uses only the info that clang knows. and clang (ie. the C frontend) does not perform any analysis of this kind, thus it does not know that stuff is dead. fwiw - my trivial (but working) patch brought down the number of reports by mere 5% so the bulk is probably somewhere else... ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
Den 05/01/2011 kl. 14.56 skrev Erik Cederstrand: Ignoring contrib code for the moment, I decided to look at usr.sbin.pw from 2011-01-05. There's one report (http://scan.freebsd.your.org/freebsd-head/usr.sbin.pw/2011-01-05-amd64/report-KkilQ3.html#EndPath) which turns out to be a false positive: * Step 6 calls cmdhelp() on line 168; * cmdhelp() ends with exit(EXIT_FAILURE); on line 432 which I assume is exit(3) from libc * The analyzer doesn't know that this function never returns and continues to flag a null dereference in step 8 The same is true of err(), verr(), errc(), verrc(), errx(), and verrx() which is also causing false positive reports. They ultimately call exit(3). Erik
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
Den 05/01/2011 kl. 14.14 skrev Ulrich Spörlein: Hello folks, Now that I'm fairly confident that the stability issues with your.org's VMs have been resolved, I'd like to point you to the new and improved, semi-weekly analyzer runs at http://scan.freebsd.your.org/freebsd-head/ I had a look at this again. There are over 9.000 reports so it's a bit overwhelming, but I suspect there's a lot of collateral damage. Ignoring contrib code for the moment, I decided to look at usr.sbin.pw from 2011-01-05. There's one report (http://scan.freebsd.your.org/freebsd-head/usr.sbin.pw/2011-01-05-amd64/report-KkilQ3.html#EndPath) which turns out to be a false positive: * Step 6 calls cmdhelp() on line 168; * cmdhelp() ends with exit(EXIT_FAILURE); on line 432 which I assume is exit(3) from libc * The analyzer doesn't know that this function never returns and continues to flag a null dereference in step 8 What's the fix here? I think the reports are an excellent way to get acquainted with FreeBSD code. Marking and fixing the false positives would make bug-hunting in the remaining reports more motivating :-) Thanks, Erik
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wednesday, January 05, 2011 9:11:50 am Erik Cederstrand wrote: Den 05/01/2011 kl. 14.56 skrev Erik Cederstrand: Ignoring contrib code for the moment, I decided to look at usr.sbin.pw from 2011-01-05. There's one report (http://scan.freebsd.your.org/freebsd-head/usr.sbin.pw/2011-01-05-amd64/report-KkilQ3.html#EndPath) which turns out to be a false positive: * Step 6 calls cmdhelp() on line 168; * cmdhelp() ends with exit(EXIT_FAILURE); on line 432 which I assume is exit(3) from libc * The analyzer doesn't know that this function never returns and continues to flag a null dereference in step 8 The same is true of err(), verr(), errc(), verrc(), errx(), and verrx() which is also causing false positive reports. They ultimately call exit(3). These are all marked as __dead2, so the compiler should know that these do not return. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: On Wednesday, January 05, 2011 9:11:50 am Erik Cederstrand wrote: Den 05/01/2011 kl. 14.56 skrev Erik Cederstrand: Ignoring contrib code for the moment, I decided to look at usr.sbin.pw from 2011-01-05. There's one report (http://scan.freebsd.your.org/freebsd-head/usr.sbin.pw/2011-01-05-amd64/report-KkilQ3.html#EndPath) which turns out to be a false positive: * Step 6 calls cmdhelp() on line 168; * cmdhelp() ends with exit(EXIT_FAILURE); on line 432 which I assume is exit(3) from libc * The analyzer doesn't know that this function never returns and continues to flag a null dereference in step 8 The same is true of err(), verr(), errc(), verrc(), errx(), and verrx() which is also causing false positive reports. They ultimately call exit(3). These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) - come up with a way to mark the false positives (kinda impossible with the way scan-build currently works) All interested parties with src access are encouraged to take a look at our Coverity Prevent installation (which is down for maintenance, but should be up soon). Regards, Uli ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Sp??rlein wrote: On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: On Wednesday, January 05, 2011 9:11:50 am Erik Cederstrand wrote: Den 05/01/2011 kl. 14.56 skrev Erik Cederstrand: Ignoring contrib code for the moment, I decided to look at usr.sbin.pw from 2011-01-05. There's one report (http://scan.freebsd.your.org/freebsd-head/usr.sbin.pw/2011-01-05-amd64/report-KkilQ3.html#EndPath) which turns out to be a false positive: * Step 6 calls cmdhelp() on line 168; * cmdhelp() ends with exit(EXIT_FAILURE); on line 432 which I assume is exit(3) from libc * The analyzer doesn't know that this function never returns and continues to flag a null dereference in step 8 The same is true of err(), verr(), errc(), verrc(), errx(), and verrx() which is also causing false positive reports. They ultimately call exit(3). These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) - come up with a way to mark the false positives (kinda impossible with the way scan-build currently works) The problem is that while exit() is __dead2 the actual cmdhelp() is not. At least clang does not see it as such. Thus the static analyzer just sees a call to a normal function (it does not recurse into it) and produces this false positive... I wonder how how hard would it to be to add some trivial IPA that analyzes cases like this.. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? I have done this in some cases because it leads to better code with gcc (the system version in 9-current). See SVN commit r212508 to bin/sh/parser.c. Although synexpect() and synerror() are static, adding __dead2 to both makes the executable 576 bytes smaller on i386 (these functions are called many times). Adding __dead2 to synexpect() only causes a warning noreturn function does return (it calls synerror()). Adding __dead2 to synerror() only also makes the executable smaller but not as much as adding it to both. Reordering the functions in the file does not help to make gcc see that the functions do not return. - come up with a way to mark the false positives (kinda impossible with the way scan-build currently works) -- Jilles Tjoelker ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
Den 05/01/2011 kl. 17.55 skrev Ulrich Spörlein: And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug I filed a bug with LLVM (http://llvm.org/bugs/show_bug.cgi?id=8914) but it seems IPA bugs filed on the analyzer have been rejected in the past. Erik
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, Jan 05, 2011 at 09:22:42PM +0100, Erik Cederstrand wrote: Den 05/01/2011 kl. 17.55 skrev Ulrich Sp?rlein: And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug I filed a bug with LLVM (http://llvm.org/bugs/show_bug.cgi?id=8914) but it seems IPA bugs filed on the analyzer have been rejected in the past. I have a dumb patch that may help here... can someone test it? http://lev.vlakno.cz/~rdivacky/clang-checker-no-return.patch it may slow down the analysis a lot, if it does please add a recursion limit there... roman ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: FYI: clang static analyzer page has moved to http://scan.freebsd.your.org/freebsd-head/
On Wed, 05.01.2011 at 20:36:53 +0100, Jilles Tjoelker wrote: On Wed, Jan 05, 2011 at 05:55:45PM +0100, Ulrich Spörlein wrote: On Wed, 05.01.2011 at 09:34:49 -0500, John Baldwin wrote: These are all marked as __dead2, so the compiler should know that these do not return. And clang did the right thing here in the past. Beware that it does no inter-procedural analysis yet, so it will usually miss that usage() calls exit unconditionally. *But*, it should grok that for err(3) and exit(3). Now there are some possible remedies: - get IPA to work with clang, or at least file a bug - mark functions as __dead2 (please don't do that) Why not? Cause IMHO it adds clutter, is noisy and needs to be maintained manually, when we have these computer things that should deduct this by themselves. I have done this in some cases because it leads to better code with gcc (the system version in 9-current). See SVN commit r212508 to bin/sh/parser.c. Although synexpect() and synerror() are static, adding __dead2 to both makes the executable 576 bytes smaller on i386 (these functions are called many times). Adding __dead2 to synexpect() only causes a warning noreturn function does return (it calls synerror()). Adding __dead2 to synerror() only also makes the executable smaller but not as much as adding it to both. Reordering the functions in the file does not help to make gcc see that the functions do not return. This is too bad and really makes me sad. It shouldn't be necessary to hand-hold the compilers like that. Could you try some tests with gcc 4.5 to confirm this is still required? Uli ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
