Re: [PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[Aaron Puchert via cfe-commits]

Am 29.04.20 um 23:11 schrieb Mandeep Singh Grang:
> My previous email details why we are doing
> this: http://lists.llvm.org/pipermail/llvm-dev/2020-April/141167.html
> Basically, we ran the PREfast static analysis tool on LLVM/Clang and
> it reported a lot of warnings. I guess some of them are false
> positives after all.
Thanks for the link. There is nothing wrong with running additional
tools on LLVM.
> As David suggests that maybe I should validate these warnings by also
> running the clang static analyzer.
You could have a look at http://llvm.org/reports/scan-build/. It's not
terribly up-to-date though, so you might also run it yourself.
> There are a lot of other warnings reported by the tool. Here is the
> full
> list: 
> https://docs.google.com/spreadsheets/d/1h_3tHxsgBampxb7PXoB5lgwiBSpTty9RLe5maIQxnTk/edit?usp=sharing.

In my opinion you're probably not losing a lot by filtering out the
types of issues that the Clang static analyzer can find as well. So for
example, ignore the null dereferences since Clang has essentially the
same check. Of course it could be that the tool finds additional issues,
I can't really say that. But you can see in the report that there quite
a few issues of a similar kind. (This one isn't among them, however.)

For analyzing the issues in the list, it would be good to note the git
commit of your analysis run, otherwise it might be hard to follow the
reports.

> If the community is interested in getting those fixed I can upstream
> patches.

Improvements are always welcome, but unfortunately no static analysis
tool only reports actual issues. Otherwise one could hope that the Clang
findings would have been all fixed by now.

So I think you need to carefully inspect the reports. It's not a bad
idea to start with a random sample of the issues reported by every
check. Avoid stylistic issues like shadowing: people have different
opinions on that. Then try to find out if there is a real issue, or if
you can think of a way to improve the code. (That's often subjective, of
course. But if you can rewrite code a bit to make it more obvious that a
certain bad thing can't happen, you'll find open ears.)

The best thing is of course if you can use the report to construct
failing test cases, but I wouldn't put the bar that high.

Best regards,
Aaron

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[Mandeep Singh Grang via cfe-commits]

My previous email details why we are doing this:
http://lists.llvm.org/pipermail/llvm-dev/2020-April/141167.html
Basically, we ran the PREfast static analysis tool on LLVM/Clang and it
reported a lot of warnings. I guess some of them are false positives after
all.

As David suggests that maybe I should validate these warnings by also
running the clang static analyzer. There are a lot of other warnings
reported by the tool. Here is the full list:
https://docs.google.com/spreadsheets/d/1h_3tHxsgBampxb7PXoB5lgwiBSpTty9RLe5maIQxnTk/edit?usp=sharing.
If the community is interested in getting those fixed I can upstream
patches.

--Mandeep

On Tue, Apr 28, 2020 at 10:44 AM Aaron Puchert via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaronpuchert added a comment.
>
> Could you explain why we are doing this? Clang has its own static analyzer
> , which can find null dereferences <
> https://clang.llvm.org/docs/analyzer/checkers.html#core-nulldereference-c-c-objc>
> as well but also tracks constraints to prevent false positives like this.
>
> There is a page somewhere with current analysis runs on LLVM, although I
> can't find it right now.
>
>
>
> 
> Comment at: clang/lib/Analysis/ThreadSafety.cpp:1938-1940
> +// The default value for the argument VD to the current function is
> +// nullptr. So we assert that VD is non null because we deref VD here.
> +assert(VD && "!VD");
> 
> dblaikie wrote:
> > Doesn't seem like the most informative comment or assertion string - the
> invariant  "isScopedVar implies VD is non-null" is established earlier in
> the function, where isScopedVar only becomes true under the "VD is
> non-null" condition at 1809.
> >
> > Would it be better to improve whatever static analysis you're using to
> be able to track that correlation, rather than adding lots of extra
> assertions to LLVM? (can the Clang Static Analyzer understand this code and
> avoid warning on it, for instance - that'd be a good existence proof for
> such "smarts" being reasonably possible for static analysis)
> I haven't tested it, but the Clang static analyzer shouldn't complain
> here. It explores different code paths and collects constraints along them.
> All code paths including the assignment `isScopedVar = true` should have
> `VD` being non-null as constraint, because that's the condition to get
> there.
>
> I know that solving constraints can be hard in general, but here the
> constraint is exactly what we need to disprove null references from
> happening.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D78853/new/
>
> https://reviews.llvm.org/D78853
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Could you explain why we are doing this? Clang has its own static analyzer 
, which can find null dereferences 

 as well but also tracks constraints to prevent false positives like this.

There is a page somewhere with current analysis runs on LLVM, although I can't 
find it right now.




Comment at: clang/lib/Analysis/ThreadSafety.cpp:1938-1940
+// The default value for the argument VD to the current function is
+// nullptr. So we assert that VD is non null because we deref VD here.
+assert(VD && "!VD");

dblaikie wrote:
> Doesn't seem like the most informative comment or assertion string - the 
> invariant  "isScopedVar implies VD is non-null" is established earlier in the 
> function, where isScopedVar only becomes true under the "VD is non-null" 
> condition at 1809.
> 
> Would it be better to improve whatever static analysis you're using to be 
> able to track that correlation, rather than adding lots of extra assertions 
> to LLVM? (can the Clang Static Analyzer understand this code and avoid 
> warning on it, for instance - that'd be a good existence proof for such 
> "smarts" being reasonably possible for static analysis)
I haven't tested it, but the Clang static analyzer shouldn't complain here. It 
explores different code paths and collects constraints along them. All code 
paths including the assignment `isScopedVar = true` should have `VD` being 
non-null as constraint, because that's the condition to get there.

I know that solving constraints can be hard in general, but here the constraint 
is exactly what we need to disprove null references from happening.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78853/new/

https://reviews.llvm.org/D78853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:1938-1940
+// The default value for the argument VD to the current function is
+// nullptr. So we assert that VD is non null because we deref VD here.
+assert(VD && "!VD");

Doesn't seem like the most informative comment or assertion string - the 
invariant  "isScopedVar implies VD is non-null" is established earlier in the 
function, where isScopedVar only becomes true under the "VD is non-null" 
condition at 1809.

Would it be better to improve whatever static analysis you're using to be able 
to track that correlation, rather than adding lots of extra assertions to LLVM? 
(can the Clang Static Analyzer understand this code and avoid warning on it, 
for instance - that'd be a good existence proof for such "smarts" being 
reasonably possible for static analysis)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78853/new/

https://reviews.llvm.org/D78853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang updated this revision to Diff 260517.
mgrang retitled this revision from "[Sema] Fix null pointer dereference 
warnings [1/n]" to "[Analysis] Fix null pointer dereference warnings [1/n]".

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78853/new/

https://reviews.llvm.org/D78853

Files:
  clang/lib/Analysis/ThreadSafety.cpp


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1935,6 +1935,10 @@
   CapDiagKind);
 
   if (isScopedVar) {
+// The default value for the argument VD to the current function is
+// nullptr. So we assert that VD is non null because we deref VD here.
+assert(VD && "!VD");
+
 // Add the managing object as a dummy mutex, mapped to the underlying 
mutex.
 SourceLocation MLoc = VD->getLocation();
 DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1935,6 +1935,10 @@
   CapDiagKind);
 
   if (isScopedVar) {
+// The default value for the argument VD to the current function is
+// nullptr. So we assert that VD is non null because we deref VD here.
+assert(VD && "!VD");
+
 // Add the managing object as a dummy mutex, mapped to the underlying mutex.
 SourceLocation MLoc = VD->getLocation();
 DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits