Thanks, it looks good to me. Most of my comments are just brainstorming, exploring alternative ideas. Feel free to ignore some/all of them.

I wonder if this feature should be part of CodeChecker/scan-build instead (potentially under an option).

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.

In D111795#3127849, @steakhal wrote:

I wouldn't mind, but it would make the matching even more difficult, potentially backtracking (?) which I'm not really a fan of.

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.

Nice cleanup!

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.

In D112646#3092619, @whisperity wrote:

@xazax.hun I think you did something similar wrt. empty(), right? Could you take a look at this?

LGTM!

Commented some nits, but overall looks good to me.

The latest version looks good to me, let's land this!

In D104616#2835030, @RedDocMD wrote:

Looks like I have wasted a good deal of effort. :(

In D104616#2834714, @NoQ wrote:

Why not simply delegate this job to assume(evalBinOp(...)) over raw pointer values, which already has all this logic written down nicely?

In D104616#2829705, @RedDocMD wrote:

If (ptr1 == ptr2) is false, we can't say anything really.

In D103750#2828995, @RedDocMD wrote:

What about this?

In D103750#2827566, @RedDocMD wrote:

Do you want the new failing test to be marked expected to fail?

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.

Whoops, I totally missed your point with my suggestion (should not read it while sitting at meetings). But Artem has a great answer already.

In D103750#2803516, @RedDocMD wrote:

In D103750#2803499, @NoQ wrote:

Ugh, this entire checkBind hack was so unexpected that I didn't even recognize evalCall. What prevents you from doing everything in evalCall? No state traits, no nothing, just directly take the this-region and attach the value to it?

Because in evalCall, I don't have access to the ThisRegion. At least I think I don't.

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?

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?

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.

It would be nice, if you could also open a PR for the github version: https://github.com/rizsotto/scan-build

In D93224#2497632, @steakhal wrote:

Do you think it's acceptable?
@xazax.hun @NoQ

In D93224#2495404, @steakhal wrote:

Do we really promise ABI compatibility for AST dumps? If we support it to some extent, then we would have a problem - which would mean that we can not target clang-12 anymore with this patch stack.

In D93224#2493434, @steakhal wrote:

How should I continue to get this working with CTU?

Yay! Deleting code is always nice.

Overall looks good to me, I have some minor nits and questions inline.

LGTM, thanks!

LGTM!

Thanks for working on this check! Please add the [analyzer] tag to the title of these patches.

LGTM!

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.

In D91223#2402038, @aabbaabb wrote:

I also have some questions:

1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?

In D91223#2395368, @aabbaabb wrote:

In D91223#2393921, @xazax.hun wrote:

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

In D90362#2369394, @phosek wrote:

What's the plan regarding updating scan-build-py to match the upstream? It seems like this issue has already been address in the upstream version.

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?

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?

LG! Thanks!

LGTM with a nit.

Is this related to https://bugs.llvm.org/show_bug.cgi?id=44493?

In D85528#2229309, @steakhal wrote:

It would be nice to have this fix in clang11.
Do you think it's qualified for it?

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!

LGTM, thanks!

In D86223#2228855, @steakhal wrote:

I wanted to conditionally, aka. in debug configuration override the base implementation of the SymbolData::getKindStr

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.

In D86223#2227353, @steakhal wrote:

Eh, I'm not sure if it worth it to put these into virtual functions - as conditionally overriding functions is not really a good idea.

Exactly, but you could return a StringRef to static storage.