- User Since
- Sep 17 2012, 3:16 AM (479 w, 5 d)
Thu, Nov 25
Thanks, it looks good to me. Most of my comments are just brainstorming, exploring alternative ideas. Feel free to ignore some/all of them.
Wed, Nov 24
I wonder if this feature should be part of CodeChecker/scan-build instead (potentially under an option).
Fri, Nov 19
Thanks for working on this! Publishing documentation alongside the feature is a great practice. I liked this documentation a lot. I have strong opinion about the ambiguous use of path condition in this document, otherwise I was only nitpicking.
Mon, Nov 15
Fri, Nov 12
Are there other classes in LLVM taking a single ArrayRef in the constructor? Are they using similar techniques to avoid potential problems?
While you have a large stack, some of these changes should be relatively independent. I'd recommend committing those regardless of the status of the rest of the stack.
Once I understand the motivation behind CallDescription::matches(), this patch looks good. The use of variadic matching is a nice cleanup.
I like the idea of introducing variadic versions of the matching. But what is the motivation behind introducing new methods that are discouraged to use? I think the motivation behind these changes should be included in the description and the commit message.
Alternatively, if we do not want the wildcard to match multiple names, I'd suggest using "?" which is more conventional to match a single item.
I like the overall idea of being strict by default and use wild cards to get the previous behavior. I am not convinced, however, about the current matching strategy.
Oct 28 2021
Oct 14 2021
Oct 13 2021
Jul 20 2021
Jul 19 2021
Commented some nits, but overall looks good to me.
Jul 5 2021
The latest version looks good to me, let's land this!
Jul 2 2021
Jun 25 2021
Jun 24 2021
Jun 22 2021
Jun 21 2021
Jun 19 2021
Jun 18 2021
Jun 17 2021
We probably did not update this PR with the discussions we had offline. Basically we had a bug with a sink node due to calling into a destructor of unique_ptr. The catch is that the ctor was evalcalled, so the store did not have the correct bindings. Thus, after inlining the dtor the analyzer though we are reading garbage values from memory and a sink node was generated. SuppressOnSink machinery suppressed the leak warning.
I believe there are a couple of comments that are done but not marked accordingly.
I agree with Artem, if we could craft code that fails due to not calling ctors, we should probably include them in the tests, even if they don't reflect the desired behavior they are a great source of documentation what is missing.
Jun 7 2021
Whoops, I totally missed your point with my suggestion (should not read it while sitting at meetings). But Artem has a great answer already.
Jun 6 2021
Since we have CallExpr, we can easily conjure up an SVal. But I don't see how I can do it similarly in this patch.
I am not being able to use this info since I don't have access to the raw pointer, so cannot create a SVal and then constrain the SVal to non-null.
Any suggestions @NoQ, @vsavchenko , @xazax.hun, @teemperor?
Jun 4 2021
I was wondering, if we could try something new with the tests. To increase our confidence that the expected behavior is correct, how about including a Z3 proof with each of the test cases?
Jun 1 2021
May 4 2021
May 3 2021
Don't mind my previous comment :) I missed the conversation about function by-ref arguments. :)
I'm arguing that it should scan for lambda captures by reference as well.
Feb 27 2021
Feb 26 2021
Feb 23 2021
It would be nice, if you could also open a PR for the github version: https://github.com/rizsotto/scan-build
Jan 14 2021
Jan 13 2021
Jan 12 2021
Dec 16 2020
Dec 15 2020
Yay! Deleting code is always nice.
Overall looks good to me, I have some minor nits and questions inline.
Dec 2 2020
Nov 26 2020
Thanks for working on this check! Please add the [analyzer] tag to the title of these patches.
Nov 21 2020
Nov 19 2020
Overall looks good, but please resolve the two nits before commiting.
I think I might have an idea of what is going on. The handles are released in a PostCall. This is intended, as the handles are in the released state AFTER the call. When you call an unknown function with a pointer, that function is free to modify the contents of the pointer. Since the analyzer has no knowledge about the body of the function, it will do a conservative evaluation, escape the symbols in the struct, and create new symbols. This is why new symbols might be introduced. I think you could subscribe to escapes and check if you access the old symbols there. If that is the case and the escape is the result of calling an annotated function, maybe you can correctly mark those symbols as deleted.
Nov 18 2020
Nov 16 2020
Nov 13 2020
I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.
Nov 2 2020
Oct 29 2020
I know that the current situation is a mess, but there is an alternative version of scan-build-py on Github, which is also distributed on pypi. Could you check if that version is also susceptible to this problem and open a pull request for the author in case it is?
Sep 11 2020
LGTM! Looks a lot cleaner this way.
So now all of the dependencies are landed and this should be ready for its prime time, right?
Aug 31 2020
Aug 24 2020
LGTM with a nit.
Is this related to https://bugs.llvm.org/show_bug.cgi?id=44493?
Aug 22 2020
Please add a test case, where the unique_ptr is initialized from a pointer parameter that has no assumptions. I think that case is not handled correctly.
This is what I had in mind, thanks!
Aug 21 2020
Aug 20 2020
MemRegion is a popular class to instantiate in the analyzer so it looks good to me.
But unless you add a comment one will probably replace the offset with an optional as the part of a refactoring.
Aug 19 2020
Exactly, but you could return a StringRef to static storage.