David Blaikie via llvm-dev
2016-May-05 18:20 UTC
[llvm-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...
Hi Apelete, Thanks for trying to help cleanup the LLVM codebase of Clang Static Analyzer warnings. But it seems a lot of the fixes that are being proposed are somewhat mechanical and may be doing the wrong thing in a few ways. * Initializing variables that are only used when initialized through some existing codepath - this can make tools like Memory Sanitizer less useful, because now the value is initialized even in some path where that value is never intended to be used * Adding assertions for pointers that are known to be non-null - in some cases this is helpful, when the algorithm that ensures the non-null-ness is sufficiently opaque. But for function parameters - we have /lots/ of non-null function parameters. I think it'd be impractical to assert on all of them. But if we want to do something like this we should probably have some community discussion about how how to go about any of these things across the codebase. Maybe using nonnull attributes would be a better approach to the parameter issue, for example - but it'll still be a lot of code churn that there should probably be general consensus on rather than approaching it piecemeal with reviewers in different parts of the codebase. Thanks, - Dave -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160505/e3b717dd/attachment.html>
Mehdi Amini via llvm-dev
2016-May-05 18:50 UTC
[llvm-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...
> On May 5, 2016, at 11:20 AM, David Blaikie via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi Apelete, > > Thanks for trying to help cleanup the LLVM codebase of Clang Static Analyzer warnings. > > But it seems a lot of the fixes that are being proposed are somewhat mechanical and may be doing the wrong thing in a few ways. > > * Initializing variables that are only used when initialized through some existing codepath - this can make tools like Memory Sanitizer less useful, because now the value is initialized even in some path where that value is never intended to be used > > * Adding assertions for pointers that are known to be non-null - in some cases this is helpful, when the algorithm that ensures the non-null-ness is sufficiently opaque. But for function parameters - we have /lots/ of non-null function parameters. I think it'd be impractical to assert on all of them.Any reason we don't use more references? I like references because when I read code it is very clear that the argument *can't* be null and I don't have to think if I need to handle the null case. When I made the DataLayout "mandatory" on the Module last year, I moved every single use of the DataLayout to be a reference, and this helped to remove each and every places that were null-checking the DL. -- Mehdi> > But if we want to do something like this we should probably have some community discussion about how how to go about any of these things across the codebase. Maybe using nonnull attributes would be a better approach to the parameter issue, for example - but it'll still be a lot of code churn that there should probably be general consensus on rather than approaching it piecemeal with reviewers in different parts of the codebase. > > Thanks, > - Dave > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
David Blaikie via llvm-dev
2016-May-05 19:42 UTC
[llvm-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...
On Thu, May 5, 2016 at 11:50 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:> > > On May 5, 2016, at 11:20 AM, David Blaikie via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > > > Hi Apelete, > > > > Thanks for trying to help cleanup the LLVM codebase of Clang Static > Analyzer warnings. > > > > But it seems a lot of the fixes that are being proposed are somewhat > mechanical and may be doing the wrong thing in a few ways. > > > > * Initializing variables that are only used when initialized through > some existing codepath - this can make tools like Memory Sanitizer less > useful, because now the value is initialized even in some path where that > value is never intended to be used > > > > * Adding assertions for pointers that are known to be non-null - in some > cases this is helpful, when the algorithm that ensures the non-null-ness is > sufficiently opaque. But for function parameters - we have /lots/ of > non-null function parameters. I think it'd be impractical to assert on all > of them. > > Any reason we don't use more references? I like references because when I > read code it is very clear that the argument *can't* be null and I don't > have to think if I need to handle the null case. > When I made the DataLayout "mandatory" on the Module last year, I moved > every single use of the DataLayout to be a reference, and this helped to > remove each and every places that were null-checking the DL. >I'm a big fan of references as well - but there's still some (read: a lot of) community/codebase momentum behind certain entities being referenced by pointer because their pointer identity is so intrinsic to their nature (comparing instruction pointers, etc). Not that one can't take the address of a thing and compare it, stuff it in a container, etc. - Dave> > -- > Mehdi > > > > > > But if we want to do something like this we should probably have some > community discussion about how how to go about any of these things across > the codebase. Maybe using nonnull attributes would be a better approach to > the parameter issue, for example - but it'll still be a lot of code churn > that there should probably be general consensus on rather than approaching > it piecemeal with reviewers in different parts of the codebase. > > > > Thanks, > > - Dave > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160505/11ada551/attachment.html>
Apelete Seketeli via llvm-dev
2016-May-06 10:09 UTC
[llvm-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...
Hi David, On Thu, May-05-2016 at 11:20:06 AM -0700, David Blaikie wrote:> Hi Apelete, > > Thanks for trying to help cleanup the LLVM codebase of Clang Static > Analyzer warnings. > > But it seems a lot of the fixes that are being proposed are somewhat > mechanical and may be doing the wrong thing in a few ways.>From the beginning I wondered for each fix if it was the right way ofsolving the possible underlying issue pointed to by the Analyzer. Since I'm not familiar with the codebase, when I was not able to decide by reading the code I ended-up trusting the tool until advised otherwise by the reviewers. Once put together, the fixes do look somewhat mechanical indeed due to the fact that the same patterns were found throughout the codebase by the analyzer. As of this writing, these patterns result in 90+ warnings still emitted when scanning LLVM+Clang+Compiler-RT (down from 150+ warnings when I fooled myself into starting to look at it a few weeks ago). I'm glad you pointed out some of the patterns in the fixes, let's see how I can address those:> * Initializing variables that are only used when initialized through some > existing codepath - this can make tools like Memory Sanitizer less useful, > because now the value is initialized even in some path where that value is > never intended to be used* I guess this case could be related to http://reviews.llvm.org/D19971 and http://reviews.llvm.org/D19975 for instance. I wasn't happy with the fixes in the first place but I ended-up trusting the tool because it proposed a codepath where the variables were used without being initialized. I don't know about msan (yet :) but if the warnings are false positives is there a way to tell scan-build in such cases ?> * Adding assertions for pointers that are known to be non-null - in some > cases this is helpful, when the algorithm that ensures the non-null-ness is > sufficiently opaque. But for function parameters - we have /lots/ of > non-null function parameters. I think it'd be impractical to assert on all > of them.* This is where I ended-up asserting function parameters in a mechanical manner to some extent (as a result of 40+ warnings about some object pointers being null). Let's take http://reviews.llvm.org/D19385 for instance. The right fix in that case was to pass the non-null parameter by reference instead of asserting its value, not unlike what you were discussing in the previous post (sorry I'm not quoting the right post here). For the cases where using references might not be possible, how do we deal with the warnings ?> But if we want to do something like this we should probably have some > community discussion about how how to go about any of these things across > the codebase. Maybe using nonnull attributes would be a better approach to > the parameter issue, for example - but it'll still be a lot of code churn > that there should probably be general consensus on rather than approaching > it piecemeal with reviewers in different parts of the codebase.I agree we should discuss the bigger issue of how to deal with each type of warning emitted by Clang Static Analyzer. I would prefer to propose fixes that solve the underlying issues instead of just suppressing the warnings. Thanks again for taking the time to suggest a different approach. Cheers. -- Apelete
David Blaikie via llvm-dev
2016-May-12 18:33 UTC
[llvm-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...
On Fri, May 6, 2016 at 3:09 AM, Apelete Seketeli <apelete at seketeli.net> wrote:> Hi David, > > On Thu, May-05-2016 at 11:20:06 AM -0700, David Blaikie wrote: > > Hi Apelete, > > > > Thanks for trying to help cleanup the LLVM codebase of Clang Static > > Analyzer warnings. > > > > But it seems a lot of the fixes that are being proposed are somewhat > > mechanical and may be doing the wrong thing in a few ways. > > From the beginning I wondered for each fix if it was the right way of > solving the possible underlying issue pointed to by the Analyzer. > Since I'm not familiar with the codebase, when I was not able to > decide by reading the code I ended-up trusting the tool until advised > otherwise by the reviewers. > > Once put together, the fixes do look somewhat mechanical indeed due to > the fact that the same patterns were found throughout the codebase by > the analyzer. > As of this writing, these patterns result in 90+ warnings still > emitted when scanning LLVM+Clang+Compiler-RT (down from 150+ warnings > when I fooled myself into starting to look at it a few weeks ago). > > I'm glad you pointed out some of the patterns in the fixes, let's see > how I can address those: > > > * Initializing variables that are only used when initialized through some > > existing codepath - this can make tools like Memory Sanitizer less > useful, > > because now the value is initialized even in some path where that value > is > > never intended to be used > > * I guess this case could be related to http://reviews.llvm.org/D19971 > and http://reviews.llvm.org/D19975 for instance. > I wasn't happy with the fixes in the first place but I > ended-up trusting the tool because it proposed a codepath where the > variables were used without being initialized. I don't know about > msan (yet :) but if the warnings are false positives is there a way > to tell scan-build in such cases ? >My first guess with any warning like this would be that there's some complex invariant (eg: A implies B, so if the variable is conditionally initialized under A then used under B it's actually fine) that is obvious enough in the code, but opaque to the static analyzer. My first guess would be to assert this (assert the condition of A in the use under B) and only if we find a case where that fails, add a test case and initialize the variable as appropriate. Adding the variable initialization without a test case seems suspect. MSan is a tool for dynamic analysis - it would track the uninitialized memory and tell us when/where we've used it without initializing it.> > > * Adding assertions for pointers that are known to be non-null - in some > > cases this is helpful, when the algorithm that ensures the non-null-ness > is > > sufficiently opaque. But for function parameters - we have /lots/ of > > non-null function parameters. I think it'd be impractical to assert on > all > > of them. > > * This is where I ended-up asserting function parameters in a > mechanical manner to some extent (as a result of 40+ warnings about > some object pointers being null). Let's take > http://reviews.llvm.org/D19385 > for instance. > The right fix in that case was to pass the non-null parameter by > reference instead of asserting its value, not unlike what you were > discussing in the previous post (sorry I'm not quoting the right > post here). For the cases where using references might not be > possible, how do we deal with the warnings ? >I think the tool needs to be fixed, or not used on the LLVM codebase if it assumes all pointer parameters may be null. But it seems like it doesn't assume that or I would expect way more errors from the tool, no? I was pretty sure the Clang Static Analyzer had been designed to only warn about null dereferences if it saw a pointer null test somewhere in the function for that pointer. Do you know what's going on here?> > > But if we want to do something like this we should probably have some > > community discussion about how how to go about any of these things across > > the codebase. Maybe using nonnull attributes would be a better approach > to > > the parameter issue, for example - but it'll still be a lot of code churn > > that there should probably be general consensus on rather than > approaching > > it piecemeal with reviewers in different parts of the codebase. > > I agree we should discuss the bigger issue of how to deal with each > type of warning emitted by Clang Static Analyzer. I would prefer to > propose fixes that solve the underlying issues instead of just > suppressing the warnings. > > Thanks again for taking the time to suggest a different approach. > > Cheers. > -- > Apelete >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160512/176d29c4/attachment.html>