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.

I wonder whether having a virtual method for symbols to get the prefix would be cleaner (something like getKindCStr), but I do not insist.

Thanks! Sorry for not noticing this earlier. The fix looks good to me and indeed the original intention was to report dead functions with ORE.

In D85528#2205799, @NoQ wrote:

I expect at least one LIT test without -analyzer-config crosscheck-with-z3=true

Looks reasonable to me, but I am not very familiar with the impacts of the additional casts. Do we lose some modeling power when we are using the regular constraint solver?

I think all the problems that left should be solved in a separate patch or even out of the scope for the GSoC.

I think adding code snippets that are affected by this patch would make it much easier to evaluate the changes and whether this is a good idea or not.

In D82598#2164363, @Szelethus wrote:

Actually, what I said initially is true:

[...] the only non-expression statements it queried are ObjCForCollectionStmts [...]

In D82598#2162239, @Szelethus wrote:

I chased my own tail for weeks before realizing that there is indeed another instance when a live statement is stored, other then ObjCForCollectionStmt...

void clang_analyzer_eval(bool);

void test_lambda_refcapture() {
  int a = 6;
  [&](int &a) { a = 42; }(a);
  clang_analyzer_eval(a == 42); // expected-warning{{TRUE}}
}

LGTM, thanks!

In D83286#2149912, @vsavchenko wrote:

@xazax.hun You were interested in performance ⏫

These results here compare this patch together with D82445 against master.

Thanks, LGTM!

Yay! This looks good to me and I love statistics, so a huge +1.
I have one question though. Isn't this counting all the reports in an equivalence class? I.e. if the analyzer finds something multiple times it will only be displayed once but here it will be counted multiple times. I think it might be worth to have two statistics, one for all bug reports and one for equivalence classes. What do you think?

@Szelethus
The patch looks good to me and I find it a welcome change that should make things easier to understand and maybe even a bit more efficient, I hope this won't break ObjC :) My discussion with Artem is orthogonal to this change.

In D82598#2115656, @NoQ wrote:

We could just kill all subexpr at the end of the full expression

I suspect that it would be pretty bad if you, say, kill the condition of the if-statement before picking the branch. Or kill the initializer in the DeclStmt before putting it into the variable (same for CXXCtorInitializer which isn't even a Stmt!).

In D82561#2116194, @gamesh411 wrote:

In D82561#2116091, @balazske wrote:

That means perform a get CTU definition if the TU to be imported (where the function comes from) is small? Otherwise it does not matter how small the function is, it can result in importing of large amount of code. Determining parameters (like "smallness") of the TU is probably not simple.

Measuring the smallness of a function is currently not trivial if the function is in another TU. But theoretically, the creation process of the CTU index has access to the number of declarations in a function, which could be used as a metric for complexity.

I always wondered if we actually need to track the liveness of exprs at all. We could just kill all subexpr at the end of the full expression without precomputing anything. But I might miss something.

The analyzer inlines small functions within a TU regardless of the thresholds. I think it would be sensible to do the same across TUs in the case we don't do this already.

I only checked the test cases and the comments so far and it looks very useful and promising. I really hope that the performance will be ok :) I'll look at the actual code changes later.

I would not call the results of the measurement within the margin of error but the results do not look bad. Unless there is some objection from someone else I am ok with committing this but please change the title of revision before committing. When one says optimization we often think about performance. It should say something like reasoning about more comparison operations.

Please add the [analyzer] tag in front of your patches as some folks have automated scripts based on that tag to add themselves as subscriber/reviewer.

In D77866#2069144, @NoQ wrote:

Like, i mean, the tree of packages that we currently have is a wrong abstraction.

A high level comment.

In D78933#2054195, @ASDenysPetrov wrote:

@xazax.hun, any thoughts?

Thanks for finding this bug! Adding some reviewers.

I still have some nits inline, but overall the implementation looks good to me.
I think, however, that we should not only focus on the implementation but on the high-level questions.
Is this the way forward we want? Can we do it more efficiently? What is the performance penalty of this solution?

Overall looks good to me some questions and nits inline.

In D79423#2025001, @martong wrote:

1. Some function types contain non-builtin types. E.g. FILE*. We cannot get this type as easily as we do with builtin types (we can get builtins simply from the ASTContext). In case of such a compound type, we should be digging up the type from the AST, and that can be done by doing a lookup.

In D79423#2022812, @martong wrote:

I don't think that could be a concern.
Actually, redefinition of a reserved name either in the C or in the C++ standard library is undefined behavior:

In D78933#2022684, @ASDenysPetrov wrote:

I have never run them before. How I can do this? What project to use as a sample?

I do agree that the warning message is not the best but it is not horrible either :)

Thanks for working on this, I do believe the analyzer would greatly profit from better constraint solving capabilities. Unfortunately, we had some troubles in the past trying to improve upon the current status and we had to revert multiple patches. This is why the community is super cautious when it comes to changes like this.

In D78933#2022288, @ASDenysPetrov wrote:

Guys,
@xazax.hun, @Charusso, @steakhal, @vsavchenko, @NoQ, @baloghadamsoftware,
please, tell something. What do you think of this improvement?

I am a bit unsure about this change. C libraries might be consumed in C++ projects and C++ projects might be free to overload those functions. Does the effort needed to specify the signatures justify not supporting that (probably unintentional) name collisions in C++?