- User Since
- Sep 3 2015, 9:16 AM (280 w, 2 d)
Tue, Jan 12
Add a few comments here and there.
Mon, Jan 11
this would be a great flow-sensitive check for the clang static analyzer
Yup, i'm sorry. I even saw this failure locally but was like "this test doesn't look familiar, probably someone else broke it" which i wouldn't have said if i looked at it for more than 1 second as it's strikingly obvious. My bad.
Thu, Jan 7
My code seems to think that a variable print_count has the value INT_MAX for some reason
Wed, Jan 6
Drop broken attributes. Fix naming.
Handle conflicts across redeclarations! Fix typos and documentation formatting.
Tue, Jan 5
This patch adds a new core checker. I wouldn't be comfortable enabling it by default without a much more careful evaluation (than a single lit test). In particular, i'm worried about false positives due to constraint solving issues and our incorrect cast representation - it sounds like both of these may disproportionally affect this checker. I also still wonder about "intentional" use of signed overflows; it's likely that some developers would prefer to silence the check. At least we should have a rough idea about how loud it is.
Thu, Dec 24
Amazing, thank you. I'm happy with the analysis and i have nothing more to say really :)
Wed, Dec 23
Add an error for conflicting attributes!
Tue, Dec 22
I think you've found a very nice and compact 50% solution to the problem. I didn't think of this while i was looking for a proper fix. Very nice.
Mon, Dec 21
Uhm, yeah, indeed. Let's remove it, thanks!
Dec 17 2020
Dec 11 2020
Dec 9 2020
Dec 1 2020
Whoops, fix typos in header formalities.
Nov 29 2020
Aha, ok then! Thanks for cleaning this up.
Nov 24 2020
A few random comments here and there as i slowly wrap my head around the overall analysis algorithm.
Nov 23 2020
if the TCB-based functions are a small subset of the code then this attribute is better
Nov 20 2020
Nov 9 2020
Nov 6 2020
You cannot have an argument expression because there's no argument expression anywhere in the AST. There's an argument, but it's not computed as a value of any syntactic expression. If there was no argument, getArgExpr(0) would have crashed; but it returns a nullptr which indicates that there's no expression to return.
Nov 5 2020
Looks great, thanks!
Everything looks good to me here. The new-expression new int; has 1 implicit argument (allocation size passed to the implementation of operator new, the value is probably 4) and 0 placement arguments (the ones that are explicitly written down after new and before int). See also https://en.cppreference.com/w/cpp/language/new#Placement_new.
Nov 3 2020
Nov 2 2020
I don't think you actually need active support for invoking exit handlers path-sensitively at the end of main() in order to implement your checker. You can still find out in a path-insensitive manner whether any given function acts as an exit handler, like you already do. Then during path-sensitive analysis of that function you can warn at any invocation of exit().
Oct 27 2020
Since recently we started doing LIT tests for scan-build in test/Analysis/scan-build, if you find a good way to test this feature feel free to add a regression test.
Oct 23 2020
Looks correct, thanks!
Honestly i'd rather eliminate SymExpr and go with Symbol everywhere. It's an overloaded term but appending "Expr" to it doesn't really make it significantly less overloaded. We're also properly namespaced so there wouldn't be any linking issues with the rest of LLVM. "Symbol" is catchy and it's one of the most important concepts to grasp in the static analyzer, i'd love to keep it.
Oct 17 2020
Aha, great, sounds like all casting can be made more correct, not just casting on store retrieve! Maybe it's worth celebrating through extra unittests on evalCast(). Thank you for looking into this.
Oct 13 2020
Oct 12 2020
Oct 8 2020
This actually looks correct to me but i suggest a bit more investigation. Architecturally, it doesn't make sense to me that CastRetrievedVal() does anything beyond svalBuilder.dispatchCast(V, castTy). After all, there's only one way to cast a value from one type to another, and dispatchCast() was supposed to be the ultimate method to do so.
Oct 7 2020
Looks great, thank you! I think you can commit it :)
Oct 5 2020
i'm pretty worried about our ability to actually achieve that in the near future
With Z3 constraint manager you absolutely want your constraints to be as precise as possible. The only reason we don't add these casts is because it confuses the constraint manager a lot. With a better constraint manager we would have spared ourselves a lot of suffering in this area.
Oct 1 2020
Aha, ok, thanks, I now understand what the problem is because I was able to run the test before the patch and see how the patch changes the behavior.
A value of an expression should have the same type and value-kind as the expression.
No, you got it all wrong again. I don't want to explain this one more time so let's talk about some basics: A value of an expression should have the same type and value-kind as the expression. Can we get there? How?
But that's for a different bug, no? Even if that patch somehow dodges *this* crash, the underlying problem of us being unable to account for mutability of VLA sizes still remains to be addressed.
Sep 30 2020
(in the latest message by "load" i mean the first load that produces a pointer (i.e., an ElementRegion) as the result)
I'm trying to say that the value produced by the load should not be the same as the stored value, because these two values are of different types. When exactly does the first value change into the second value is a different story; the current grand vision around which the code is written says that it changes during load, therefore it's the load code (step #2) that's to blame.
Yup, that's pretty bad.
Thank you for reminding me of this patch. I still think it's a pretty important patch and i'm interested in getting it landed.
Sep 29 2020
A load from a region of type T should always yield a value of type T because that's what a load is.
Sep 28 2020
Nice, very interesting!
I also recommend using a specialized callback checkBind instead of pattern-matching binary operators specifically.
The code you've added looks correct. I suspect either heap invalidation or dead symbol elimination cleaning up the binding.
Sep 27 2020
Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.
We already have a similar simplification facility in SValBuilder created to solve the similar problem with iterator checkers. It fires up when it knows that the values it works with are limited to roughly 1/4 of their type's range and therefore none of the individual operations over them can potentially overflow (cf. D35109). It's currently off by default because performance was not properly evaluated and none of the on-by-default checkers rely on it. This is currently the intended approach to such issues. It was decided that constructing a custom solver for "non-overflowing but still bounded" arithmetic was not the right thing to do, mostly because it absolutely doesn't correspond to the actual run-time behavior of the program that we're supposed to be modeling.
Sep 24 2020
A VLA in a loop may have different size on each iteration of the loop. This looks very much related to https://bugs.llvm.org/show_bug.cgi?id=28450.
Sep 23 2020
Sep 22 2020
Sep 21 2020
Uh-oh, so you're saying that you're actively modeling every single operator + and - on every single integer in the program even when they don't represent any iterators at all? How can a raw integer be an iterator to begin with, given that you can't dereference an integer? Is this caused by D82185? Can you defend against such situations by checking that the operand is a pointer before doing any operations?
Aha, yup, thanks, this looks good!
Thank you, i think this is good to land!
Sep 19 2020
(I.e., i don't have strong opinions about high-level design and will happily push the green button as soon as minor nits are addressed)
Sep 18 2020
Sry! I'll try my best to keep up from now on.
My quick naive attempt of getting SVal of DeclRefExpr and checking if its MemRegion is in current stack frame didn't get very far because the returned SVal kind is undefined.
Sep 8 2020
Sep 5 2020
This looks correct to me. Thanks a lot. This hack was surprisingly crude and i'm very glad that we now consistently use expressions in our Environment.
Sep 3 2020
What are types of each term in this expression?
Sep 2 2020
Nice catch, thx!
Aug 27 2020
Yup, that's more like it!~
I have no other concerns, i think this is good to go!
Aug 25 2020
No-no, recursive locks are much more complicated than that, please do them separately. You'll have to discriminate between a real double lock / double unlock bug and an actual valid use of the recursive mutex in order to emit any warnings at all. That's completely different checker logic.
Looks amazing now.
Hans is pinging us on Bugzilla because this patch is marked as a release blocker (and i believe that it indeed is). I think we should land these patches.
Aug 24 2020
If the numbers are confirmed to be as good as what i've sneak-peeked so far, that should be pretty amazing. Also whoa, test coverage!~