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++?

This is cool!

I'd prefer to have this functionality committed together its first actual use with tests.

Also, I see no tests :)

If a given parameter in a FunctionDecl has a nonull attribute then the NonNull constraint in StdCLibraryFunctionsChecker has the same effect as NonNullParamChecker.

LGTM!

I am a bit unsure what the purpose of these Max summaries are? As far as I understand the Max represents the largest value for the type of the formal parameter.

I think testing summaries this way can be really hard to manage in the future.
I see two possible ways forward to make this easier:
a) Make something like https://reviews.llvm.org/D78118 that will also dump the actual summary in a textual form, not only the fact that a summary was loaded and check for that textual form.
b) Make a small helper library for testing summaries. E.g., most of the buffer handling functions have similar summaries, so we could have only a couple of functions for testing buffer related summaries and we could just pass in function pointers (as templates or params) instead of duplicating code. Similarly, testing output ranges could be generalized.

Overall looks good to me, I have one question inline.

Nice catch, LGTM!

Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.

Overall looks good for me, thanks for tackling this problem! I think this should be good to go once Eli's comment is fixed.

I think this should not be blocked on the gtest update. If getting an updated gtest into the repo takes to much time and the reviewers are happy, I'm fine with doing that change as a follow-up.

LGTM, thanks!

Actually, sorry. I just realized that the alignof problem is introduced by this patch. I'd love to see the solution committed together with this patch (it could be a separate patch but preferably, they should be committed together.)

Thanks! Having more tests is always welcome!

Thanks, this looks much better now.
Could you also update the description of the revision to match the current status? (E.g. type aliases are now supported.)
If you do not plan to solve the alignof issue in the near future, maybe it would be worth to also open a bug report.
Please wait for one more approval before commiting.

Ok, looks good to me.

In D78453#1991384, @steakhal wrote:

In D78453#1991294, @xazax.hun wrote:

Mostly looks good to me. I do believe that this is an optimization but it would be nice to have a small benchmark :) In case it is too much work, I do not insist though.

I'm curious too.

Though I'm not sure how to accomplish that. AFAIK you measured such.
How should I measure this?
Should I use eg. the Clang SA and analyze different projects? (which, how many, etc)
Wouldn't have this method introduce too much noise?
Or should I measure this by a microbenchmark?

I'm really in favor of the microbenchmark approach.

One way to test this change would be to add a statistic that is bumped each time a path is refuted (a report to be refuted we need all of the paths refuted, so using refuted reports might not be fine-grained enough). We can test on some big projects if the refuted paths increase or decrease after the change.

In D78457#1991780, @martong wrote:

If a symbol is unused and garbage collected then that is not part of the path constraint that leads to the ErrorNode, is it? So why should we care about constraints on an unused symbol?

Mostly looks good to me. I do believe that this is an optimization but it would be nice to have a small benchmark :) In case it is too much work, I do not insist though.

As turned out we don't even need a BugReporterVisitor for doing the crosscheck.
We should simply use the constraints of the ErrorNode since that has all the necessary information.

LGTM! Thanks!

I am not an expert when it comes to VLAs but I do see some problems here.

In D63279#1939349, @Szelethus wrote:

(note: I forgot to submit this a couple weeks ago)

LLVM is crashing on me due to the issue mentioned in D75678, but on Bitcoin, Xerces, CppCheck and Rtags I observed no difference in between the 2 runs. I recall that others mentioned that @szepet used to run his analyses with other configurations. I'll read final report and take another look later.

http://lists.llvm.org/pipermail/cfe-dev/2017-August/055259.html

• Rebase.
• Address some review comments.

In D76287#1929221, @Szelethus wrote:d:

Variable could be practically anything that can be written, but due to the nature of what we can work this, we have to make severe restrictions.

• We don't really have a good points-to-analysis. This discards pointees of any kind pretty much immediately. I don't have long term plans to add support for them.

Added some more reviewers who might be interested.

In D75842#1912364, @baloghadamsoftware wrote:

In D75842#1912293, @xazax.hun wrote:

I am not sure if I would call this a bugfix. Enabling a checker without one of its dependency will potentially cause the checker to misbehave. I am uncomfortable changing the current behavior to a more dangerous one without any diagnostics. Including the diagnostic with this patch should not make this too big.

This is a bugfix. The current behavior is wrong due to a mistake. A checker is registered which should not and this may cause crashes.

I am not sure if I would call this a bugfix. Enabling a checker without one of its dependency will potentially cause the checker to misbehave. I am uncomfortable changing the current behavior to a more dangerous one without any diagnostics. Including the diagnostic with this patch should not make this too big.

If we disable the dependency of a checker explicitly I think we should at least emit a warning.

In D75163#1902921, @balazske wrote:

In D75163#1902816, @xazax.hun wrote:

If we were to refactor this check I wonder if it would make sense to port evalCall to postCall, so the analyzer engine will conjure the symbol for us.
I wonder what @NoQ thinks about the pros and cons of the approaches.

Once I wanted to remove evalCall but it was no success:
https://reviews.llvm.org/D69662

Other than a nit looks good to me but wait for @NoQ before committing.

This might be a silly question but is CXXDeleteExpr the only way to invoke a deallocator? What would happen if the user would call it by hand? Should we expect ExprEngine to trigger a callback in that case?

Some nits inline.