<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 12, 2016 at 2:05 PM, Apelete Seketeli <span dir="ltr"><<a href="mailto:apelete@seketeli.net" target="_blank">apelete@seketeli.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, May-12-2016 at 11:33:28 AM -0700, David Blaikie wrote:<br>
> On Fri, May 6, 2016 at 3:09 AM, Apelete Seketeli <<a href="mailto:apelete@seketeli.net">apelete@seketeli.net</a>><br>
> wrote:<br>
><br>
> > Hi David,<br>
> ><br>
> > On Thu, May-05-2016 at 11:20:06 AM -0700, David Blaikie wrote:<br>
> ><br>
</span><span class="">> > > * Initializing variables that are only used when initialized through some<br>
> > > existing codepath - this can make tools like Memory Sanitizer less useful,<br>
</span>> > > because now the value is initialized even in some path where that valu is<br>
<span class="">> > > never intended to be used<br>
> ><br>
> > * I guess this case could be related to <a href="http://reviews.llvm.org/D19971" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19971</a><br>
> > and <a href="http://reviews.llvm.org/D19975" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19975</a> for instance.<br>
> > I wasn't happy with the fixes in the first place but I<br>
> > ended-up trusting the tool because it proposed a codepath where the<br>
> > variables were used without being initialized. I don't know about<br>
> > msan (yet :) but if the warnings are false positives is there a way<br>
> > to tell scan-build in such cases ?<br>
> ><br>
><br>
> My first guess with any warning like this would be that there's some<br>
> complex invariant (eg: A implies B, so if the variable is conditionally<br>
> initialized under A then used under B it's actually fine) that is obvious<br>
> enough in the code, but opaque to the static analyzer.<br>
><br>
> My first guess would be to assert this (assert the condition of A in the<br>
> use under B) and only if we find a case where that fails, add a test case<br>
> and initialize the variable as appropriate. Adding the variable<br>
> initialization without a test case seems suspect.<br>
><br>
> MSan is a tool for dynamic analysis - it would track the uninitialized<br>
> memory and tell us when/where we've used it without initializing it.<br>
<br>
</span>Ok, the invariant example is a good insight I wish I kept in mind when<br>
I started. I took interest in Clang Static Analyzer reports because I<br>
wondered how the tool works (and how well). Knowing how to treat the<br>
warnings will definitely help making better decisions (and better<br>
patches).<br>
<br>
Any further advice will be greatly appreciated :).<br>
<span class=""><br>
> > > * Adding assertions for pointers that are known to be non-null - in some<br>
> > > cases this is helpful, when the algorithm that ensures the non-null-ness is<br>
> > > sufficiently opaque. But for function parameters - we have /lots/ of<br>
> > > non-null function parameters. I think it'd be impractical to assert on all<br>
> > > of them.<br>
> ><br>
> > * This is where I ended-up asserting function parameters in a<br>
> > mechanical manner to some extent (as a result of 40+ warnings about<br>
> > some object pointers being null). Let's take <a href="http://reviews.llvm.org/D19385" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19385</a><br>
> > for instance.<br>
> > The right fix in that case was to pass the non-null parameter by<br>
> > reference instead of asserting its value, not unlike what you were<br>
> > discussing in the previous post (sorry I'm not quoting the right<br>
> > post here). For the cases where using references might not be<br>
> > possible, how do we deal with the warnings ?<br>
> ><br>
><br>
> I think the tool needs to be fixed, or not used on the LLVM codebase if it<br>
> assumes all pointer parameters may be null. But it seems like it doesn't<br>
> assume that or I would expect way more errors from the tool, no? I was<br>
> pretty sure the Clang Static Analyzer had been designed to only warn about<br>
> null dereferences if it saw a pointer null test somewhere in the function<br>
> for that pointer.<br>
><br>
> Do you know what's going on here?<br>
<br>
</span>Judging from the 55 warnings I got by running the analyzer on<br>
LLVM+Clang+Compiler-RT codebase when I started, it does *not* seem to<br>
assume all pointer parameters may be null indeed.<br>
However, the tool seems to be able to warn not only about "null<br>
dereferences if it saw a pointer null test", but also about null<br>
dereferences if it saw a may-be-null pointer passed as a parameter<br>
in a function call in the same compilation unit.<br>
<br>
This last sentence should read: the tool also seems to be able to follow the<br>
function call flow inside a compilation unit and warn about null<br>
dereferences at any point during that call flow (e.g.<br>
<a href="http://reviews.llvm.org/D19084" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19084</a> in lib/AST/NestedNameSpecifier.cpp).<br></blockquote><div><br></div><div>Yes, the static analyzer is interprocedural within a single translation unit, so this might be the issue (it sees a null test of a pointer inside one function A, then assumes that the parameter to function B (which calls A and passes along the pointer) must also be possibly null). It would be good to test that, then fix the analyzer to not make this assumption - it seems incorrect. Just because a pointer may be null in one function, doesn't imply much of anything about it in callers to that function - those callers may have guaranteed non-null pointers still.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Some of these latter warnings may well be false positives I agree, but<br>
what makes it hard to ignore is when the tool actually suggests a call<br>
path to the null dereference. Ignoring it then looks to me like a<br>
case of "the pointer is *expected* to be non-null, so the warning<br>
should be ignored regardless of the hints provided by the tool".<br>
<br>
I'd rather add an assert in a defensive stance in that case, but<br>
that's just me; in the end I take the reviewer's advice into account :).<br></blockquote><div><br></div><div>My argument against this is that it seems we'd end up with a very haphazard set of assertions - the significant majority of cases where we have non-null pointers aren't being diagnosed by the tool, so some semi-arbitrary subset of those cases would end up with asserts and the rest not. I'm not sure how constructive that would be, which is why I'm inclined to push back a bit on that approach due to the lack of consistency in the end result we'd get.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888">--<br>
Apelete<br>
</font></span></blockquote></div><br></div></div>