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.).

Turn the comment into an assertion.

The code looks good now! A few minor comments and we can commit this :)

An idea. I believe the safest way to find the bugs you mentioned would be to replace extent-as-a-symbol with extent-as-a-trait.

I have just one comment and i think it'd be good to land.

Fix comments.

Yep, fixed indeed.

We've broken something:

Uhm, messed up the phabricator link in rL304162, which should have been pointing here but points to D28445 instead.

Uhm, messed up the phabricator link in rL304162, which should have been pointing to D30909 but points here instead.

I'll land this. Thanks again for working on all that stuff!

I believe we can move on to the next one :) Just hope we didn't screw up the rebase too much here. Thanks again!

I'd commit your patch without the .gitignore change, as it deserves a separate commit and more attention; will have a look at it myself - llvm's and clang's .gitignores have diverged quite a bit.

Sorry, got carried away by GSoC and critical stuff...

No-no, all i was trying to say is that there's code in PthreadLockChecker.cpp that you haven't changed, but accidentally reformatted - and this is something we normally try to avoid. Like, for example, changing enum LockingSemantics {...} from vertical to horizontal - that wasn't your intention, it just accidentally happened because you auto-reformatted the whole file. I don't mind these changes, and i didn't mean they introduce any merge conflicts for now, though they tend to do so in the future for other people working on the same code, as we have a few downstream users, and magenta guys who are working on this checker as part of D26342), so most of the time it's better not to introduce unnecessary changes.

Thanks, this is great! Two more things:

Thanks! Your code looks very clear now, and it seems correct to me.

Todo:

• See if the extra __kindofs leak into diagnostic messages.
• Test how this behaves in cases (2) and (4), add tests.
• Add tests for the recursive __kindof specifier merge (i.e. properly merge the second __kindof in NSSet<__kindof NSArray<__kindof NSString>>).

Added a bit more info in the code comments.

I think i found a relatively clean way of storing the kindof specifiers, which is within the most-specialized type object itself.

Automatically pop up from bodyfarm-originated code in PathDiagnosticLocation::getStmt().

Cover more cases and be more conservative.

I guess i was planning for C++ support here, but never reached that far.

Great, thanks!

Hmm, shouldn't this be part of BuiltinFunctionChecker aka core.builtin.BuiltinFunctions? We already have __builtin_assume_aligned here (though it doesn't seem to assume anything because that particular assumption is hard to model).

This is fantastic, thanks! I really like the shape of how it turned out to work.

In D32437#750622, @zaks.anna wrote:

That wouldn't work this way because we'd have the completely redundant "calling property accessor" piece before that, and "returning..." after that.

I think we should not print "calling" and "returning" for calling into and returning from autogenerated code,

What are these? Is there a PR?

Noticed a few more things.

I see the checker going towards what we call "metadata symbols", which seems logical.

Thanks for uploading this to phabricator and sorry again that i was lost for a while.

Uhm, i need to do something about this duplicate account. Sorry, I have completely forgotten that the review is already up...

Added a reusable bug category.

Thanks! Because LLVM's Illinois license is rather permissive than copyleft, we try to avoid stuff copied from GPL/LGPL software (such as Qt or K3B) in our codebase. The code doesn't seem to have been copy-pasted from a copyleft project, but I guess it's better to rename the file to avoid referencing to K3B, and i added inline comments regarding moc intro comments.

Could you add a test, like a regular test in test/Analysis/copypaste/? Like, make a file with _moc in it, and demonstrate that the checker doesn't warn, despite having obvious clones otherwise.

Looks good, thanks!

I've a feeling we need to roll this back a little bit.

Yep, this sounds logical!

Thank you very much!

I've seen the new reviews. A lot of thanks for working on the patch split. I believe it'd eventually benefit everyone, similarly to how everybody benefits from code quality or something like that. I'd try to keep up with the "to split or not to split" discussion a bit below, not sure if it's of much interest since we're already trying this out practically and we'll see how much benefit it brings at the end :) And in any case, it's quite hard to see at first that the "incremental" mindset actually works in practice, as i do know that initially it feels strange at times.

This looks great, thanks a lot!

I had a look at how hard it is to split the patch.

Fix comments!

1. Could you do renaming the checker file in a separate patch, so that we saw an actual diff, not a whole greenish file, here on phabricator?
2. The invalidated iterator checker looks relatively small (a single check and a few rounds of iterator invalidations), would it be hard to split it up into a separate patch? That'd leave us with just one checker added.

Yay, now that's a lot cleaner!

I think for this case it'd be great to (instead) add comments all over the place (especially in checker callbacks, eg. check[Pre|Post]Call() function bodies) to indicate what check every piece of code is for. That'd be equally easy to review, at least for me, but it'd also help greatly if we start splitting up checkers into modelling and checking parts.

Wow, so you're doing the binding thing now? Thanks! It was not critical for landing this patch, so you could have fixed comments here, allowing us to commit what's already done, and then proceed with further improvements. It's also easier to review and aligns with the LLVM's policy of incremental development.

Hmm, i've been thinking of writing a test for this via ExprInspection's clang_analyzer_printState(), however printState() functionality is only enabled in debug builds, and i'm not seeing how to enable the test only on debug builds (there's REQUIRES: but it only seems to have an asserts flag).

This looks correct! Thanks for taking this up.

Remove accidentally added braces in unrelated code.

In D31886#724796, @xazax.hun wrote:

maybe we want to skip this kind of simplification in case of Z3?

Thanks for the comments! Updated.

One more obvious observation regarding scan-build: because you are already reading a compilation database, the whole tool is essentially usable in combination with the current scan-build-py (which can create compilation databases). So it's already quite usable, but you're forced to do regular analysis before cross-translation-unit analysis. So all we need is an extra mode in scan-build-py that does the interception and leaves the rest of the work to us, either by piping commands to us directly, or by providing us with a compilation database (if --intercept-first is passed). Having some kind of --intercept-only would solve half of the problems. Of course, ideally the logic that adds the -analyzer* options should also be re-used, but for usability it isn't immediately necessary.

Yeah, of course, ideally sticking this into scan-build, one way or another, would be great. I understand that it'd require quite a bit of communication with Laszlo. Otherwise we're just duplicating a whole lot of things.