First of all, thanks everybody for working on this. I'd see what i can do.

After the FIXME about record layout is addressed, i guess the next action item is to make sure the trivial constructor for the empty base is not evaluated conservatively (doh!). Because whatever bindings we make, for now they'd be blown away immediately during "construction".

Aha, right, makes sense. Yeah, there must be more problems with this mode. Which doesn't make the current patch less useful :)

Actually update the diff.

We've discussed it in person with Devin, and he provided more points to think about:

Totally makes sense, thanks!

Do i understand correctly that your code does widening for simple loops but leaves complex loops intact, unless the old widening is also turned on?

Looks great now!

Hmm, looks correct. Thanks! I guess this problem appeared when we enabled SymSymExprs, otherwise (End - Begin) would have been UnknownVal.

In D35109#837673, @zaks.anna wrote:

What do you suggest? Should we widen the type of the difference, or abandon this patch and revert back to the local solution I originally used in the iterator checker?

Does the local solution you used in the iterator checker not have the same problem?

Am i understanding correctly that this patch is waiting to be rebased on top of D35684?

Woohoo, looks much nicer and cleaner :3

CFG construction tests seem to have accidentally disappeared from the recent revisions.

Yay, looks cool.

Maybe use decl matcher as the parameter, and pass either equalsNode(Foo) or equalsBoundNode("foo") as argument value?

Cool, i totally forgot that we have valgrind! :)

It seems that this check is more powerful because it works by knowing the dynamic type of the object. However, i still suspect that -Wnon-virtual-dtor (the other one, without delete-, that simply asks to make the destructor of polymorphic classes virtual) covers most practical cases. The only thing i see not covered by -Wnon-virtual-dtor but covered by this checker is the situation when the destructor is private. Reka, would you confirm our understanding?

Yeah, right, good point =)

Hmm, what prevents us from reusing existing matchers against S in isPossiblyEscaped()? Also i'd probably prefer to avoid recursion (we don't want stack traces of 225k frames).

I suspect that the usage of the CFGStmtMap* caused the undefined behaviour since its lifetime was depending on its LocationContext.

Yeah, it lives in AnalysisDeclContextManager, which lives in AnalysisManager, which lives throughout a single analysis (top-level function) and then dies (when the next top-level function is picked that wasn't covered during inlining) to clean up the allocators.

These comments of mine are mostly random ideas for follow-up patches, i agree with Gabor this patch is good to land.

Did the checker flag any intentionally underpopulated enums, eg.

enum {
  Unset = -1
} NumberOfChickens = 15;

(such code is bad, but it is not necessarily buggy - as far as i understand from the CERT rule, we're talking about "unspecified behavior" which some implementations may define and document, and then some people may rely)?

Yeah, this looks good. How does this compare to alpha.core.BoolConversion, should we maybe merge them?

Bring back the test that was accidentally lost.

Rebase. Turn the comment into a FIXME.

Reduce stack operations as Alexey suggested.

If we are to help with looking into this remaining bug, could you try to extract the info that's needed to reproduce it (eg. preprocessed file and analyzer run-line)?

The new tests look great, thanks. I think it's time to land :) You do already have commit access, don't you?

In D35216#814124, @dcoughlin wrote:

In this case, I would be fine with some sort of AbstractStorageMemoryRegion that meant "here is a memory region and somewhere reachable from here exists another region of type T". Or even multiple regions with different identifiers. This wouldn't specify how the memory is reachable, but it would allow for transfer functions to get at those regions and it would allow for invalidation.

Approach (2): We could teach the Store to scan itself for bindings to metadata-symbolic-based regions during scanReachableSymbols() whenever a region turns out to be reachable. This requires no work on checker side, but it sounds performance-heavy.

It'd look good in clang-tidy (especially if extended to provide fixits), but if Daniel is interested in having this feature in the analyzer (and picked by clang-tidy from there), i wouldn't mind.

I'd have a look soon.

I think we'd rather delay the unsigned difference feature (as far as i understand, you don't rely on it in the iterator checker), than introduce the artificial cast to signed which may have unexpected consequences, because that's not how unsigned difference normally behaves. I'd think a bit more on this as well.

I think the remaining switch-related code seems to be about C++17 switch condition variables, i.e. switch (int x = ...)(?)

I think you might also need to convert APSInts to an appropriate type, as done above. Type of right-hand-side APSInts do not necessarily coincide with the type of the left-hand-side symbol or of the whole expression. APSInt operations crash when signedness doesn't match (and in a few other cases).

These are some great questions, i guess it'd be better to discuss them more openly. As a quick dump of my current mood:

This looks great, thank you very much.

Or maybe it wasn't a good idea to make two diffs in one place. Dunno.

This other diff implements approach (1) through a draft of a checker (that doesn't model much yet). I had to additionally make sure we already have a region to construct metadata for, in spirit of D27202.

In D16403#803518, @m.ostapenko wrote:

There is one more thing I'd like to clarify: since e.g. BreakStmt can terminate multiple scopes, do I need to create ScopeEnd marks for all of them to make analyzer's work easier? Because right now only one "cumulative" block is generated and I'm not sure that it's acceptable for analyzer.

Yeah, it seems that if the same loop is encountered on different paths, it gets counted twice, but if it's encountered multiple times on the same path, it's counted once. At least this is how the new NumTimesLoopUnrolled statistic seems to work.

Thanks for publishing this. It seems that your code is already huge, and you have already added a lot of workarounds for various problems (known and new) in our APIs, while it'd sound great to actually solve these problems and simplify the API to make writing checkers easier (as in http://lists.llvm.org/pipermail/cfe-dev/2017-June/054457.html ). This technical debt seems to slow down any progress on the checkers dramatically - and i'm totally sorry about that.

I think next time we should add examples of warning messages here - not just "//warn" but how they literally may look - so that people were able to find this page, and not doxygen/github source code, when they google up their warning messages.

Thanks, this looks great!

In D35041#801073, @alexshap wrote:

evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch

Because this patch affects the default behavior, i think it's necessary to understand the performance impact on a large codebase. I may lend a hand eventually, but if you're in a hurry you could probably at least run an overnight analysis over llvm and sqlite with range constraint manager and see how performance changes with this patch, and ideally also if we get new reports or lose old reports.

Thanks, these are very helpful.

Looks good. To think: maybe we need a better name (and/or a place to live) for this API, now that it's not only related to clone detection, but is useful for more stuff?

This looks negligible compared to compilation time.
I wonder if we're now scalable enough to try finding whole-translation-unit clones.

Totally makes sense :)

Hmm, what else remains to be fixed before we should try to deliver CloneChecker to the users (move out of alpha, either on by default or into optin)?

We should probably add a test that shows that the current default value of MinimumCloneComplexity is large. Like, test that the positive we're trying to avoid with this patch is indeed gone.

So how much of this would be thrown away once we have scopes?

While this wasn't necessary in the first patch, i think here we should start cleaning up the UnrolledLoops map when we exit the loop. Because next time we encounter the loop, we may no longer want to unroll it - the value may no longer be concrete.

In D34102#788138, @zaks.anna wrote:

I think it's better to have inconsistency than having checks applicable to windows in a package named portability.unix.

In a perfect world, the analyzer's report would work like a debugger: jumping through stack frames, or even through diagnostic pieces, and printing symbolic values of variables at every piece would be really great.

In D32642#789004, @baloghadamsoftware wrote:

Now I can improve SValBuilder to compare {conj_X}+n to conj_X}+m, but I am not sure if it helps to simplify compare() much. How to handle cases where I have to compare {conj_X}+n to {conj_Y}+m, an we have a range [k..k] for {conj_X}-{conj_Y} in the constraint manager. I still need to decompose the two expressions, retrieve the single length range and adjust one of the sides of the comparison. I think I should not add such complicated code (i.e. retrieving single length range from the constrain manager) to SValBuilder.

Maxim, totally thanks for picking this up!

For example,

In D32642#788822, @baloghadamsoftware wrote:

I'm sure that simplification (($x + N) + M) ~> ($x + (M + N)) is already working in SValBuilder.

No, it is unfortunately not working: I tried to increase ${conj_X} by 1, then again by 1, and I got symbolic expression (${conj_X}+1)+1).

In D32642#787913, @baloghadamsoftware wrote:

I tried SValBuilder::evalBinOp() first but it did not help too much. It could decide only if I compared the same conjured symbols or different ones, but nothing more. It always gave me UnknownVal. Even if comparing ${conj_X} == ${conj_X} + n where n was a concrete integer. So I have to compare the symbol part and the concrete integer part separately.

In any case, it's not right to have two SValBuilders. Your code simplifies symbolic expressions of numeric types, SValBuilder does the same thing, there's no need to duplicate. It would be much better to move the functionality you need (and already have implemented) directly to SValBuilder.

Regarding serializing vs not serializing and now vs later.

Hmm, yeah, right :)

In D34102#783161, @zaks.anna wrote:

eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc

Is the checker you are moving to portability off and not useful on Windows?

Gabor makes such a good point. Maybe we should commit the zombie symbols patch as well (:

Hmm, should there be new tests that demonstrate that we cover the new APIs?

I'm sorry, i'd try to get back to this and unblock your progress as soon as possible.

Thanks, this was very useful, please commit!

Yeah, right, i guess it's mostly about the CFG patch; i was just saying that if, in the end, we don't get scope contexts for all the scopes, this is probably fine - as long as we have *some* of them.

In D34260#783863, @a.sidorin wrote:

Now to the eureka - am i the only one here who totally forgot about D16403/D19979?

We're working on bringing it back to life right now. But it takes time.

That's totally cool.

Hmm, curious, having a look. A couple of blind guesses before i actually understand what's going on:

This was an mixture of internal apple projects (user apps, drivers, deamons, whatever) with a relatively balanced selection of languages and levels of analyzer adoption. They amounted to ~16 hours of analysis CPU time (i.e. 4 hours on a quad-core machine per run). I've also ran it on LLVM separately, and had similar observations. I'm totally welcoming the feedback from other codebases!

A historical retrospective for Peter: there have been quite a few people, at different moments of time, who wanted a checker callback for entering and exiting "scopes". For example, StackAddrEscapeChecker finds local variables that escape a function by reference, but doesn't find local variables that escape an if-branch or a loop body by reference. The proposed approach involved creating a ScopeContext, a counterpart of StackFrameContext that gets constructed and put on the stack of location contexts when a scope is entered and popped when the scope is exited, similarly to how StackFrameContext is put on the stack of location contexts to represent the function call. Because a lot of events occur at scope boundaries, that'd be a great addition. However, the patches i mentioned are not yet done because some corner-cases were not handled. Whether you should complete these patches, as part of your GSoC project - i do not know, it may be harder than it seems. But they'd certainly serve as a good source of inspiration.

Uhm, it just striked me where this is going. So, generally, we're bound to track when we're entering the loop and exiting the loop. And we have troubles with that, because just by looking at the block we can't say in what loops we are.

Whoops, forgot to actually attach the patch. Here.

Here's a version of the patch without .unix. I'd still hate it to re-add the subpackages if we decide to turn some portability checkers on and off depending on language/platform (eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc.).