Neat! Tests?

The other way round, i guess. I like the test change, it's easier to understand, so it's better to have it before starting to understand :)

Ideas behind both hashing change and new testing mechanism look great to me.

Because i didn't get back to this in a while, and similar crashes keep coming, i decided to leave this refactoring as a FIXME.

Last time i was running on WebKit; i already lost my results, so i'd try to reproduce the results on the fixed checker and follow up. Apart from D31538, i've seen a few cases where a method was safe to be called on a moved-from object (which led me to believe that we'd need to be safer here), and a few weird cases where a moved-from object was accidentally copied implicitly, which seemed to be a non-issue.

In D35216#886212, @rsmith wrote:

This is precisely how the rest of the compiler handles CXXStdInitializerListExpr

Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for us to pattern-match the implementations as well (the problems are still there for other STL stuff).

Escape into array and dictionary literals, add relevant tests. Fix the null statement check.

In D35216#886212, @rsmith wrote:

This is precisely how the rest of the compiler handles CXXStdInitializerListExpr

Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for us to pattern-match the implementations as well (the problems are still there for other STL stuff).

Whoops forgot to submit inline comments.

Yep, looks good.

Yeah, nice catch. So we need to either always tell the checkers to specify their CharTy when they are dealing with void pointers, or to do our substitution consistently, not only for SymbolicRegion but also for AllocaRegion (did that in this diff).

Add @alexfh's small reproducer test case. It was so small i never noticed it until now!

Add a forgotten comment.

Add no-crash test cases from https://bugs.llvm.org/show_bug.cgi?id=34373 and https://bugs.llvm.org/show_bug.cgi?id=34731 .

@dcoughlin: You're right, my reasoning and understanding was not correct, and your explanation is much more clear. My code still makes sense to me though, so i updated the comments to match. And moved the unusual logic for the lvalue-to-rvalue cast unwrap to the bottom of the function.

Update to use .rst formatting.

Fix some comments in tests.

Looks good!

The overall idea makes sense to me. I'd like you to join the effort with Peter who during his work on loop widening came up with a matcher-based procedure for finding out if a variable is changed anywhere; it currently lives in LoopUnrolling.cpp and we need only once implementation of that.

Yeah, sounds good to me as well.

*approves more active use of DEBUG()*

Remove the changes in tests for now. I guess they'd need more cleanup anyway.

Neat! I've seen this code multiple times but never noticed this bug.

The header declares and the cpp file defines multiple visitors. However, unless you are the BugReporter, you only care about the possibility of defining their own visitor, and don't care about re-using other visitors. I guess that's how these names came to be.

Don't forget to check that the function is a global C function in post-call.

Or maybe just add an example with blocks below your text?:

Yeah, in D22856 i intended -analyze-function to work in pair with -analyzer-display-progress - you can always copy-paste the fully qualified name from there, even if it's an ObjC method or even an anonymous block. But i guess i failed to explain this in D22874.

I've seen this recently, and while i agree that the fix is correct, i'm not entirely sure that the test cases are correct. As weird as this may sound, null dereference is not an attempt to read from or write to memory address 0. Instead, it is about using a null pointer as if it was pointing to an actual object in memory, even if accessing it by a non-zero offset. For example, in

struct S {
  int x, y;
};

Heh, i never noticed. Thanks.

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

This is all wrong. While RetainCountChecker is more function-local than, say, MallocChecker, we still can't say for sure that it is the bottom frame's function (or block) that should be owning the object in this case. Ideally it should, but that's not the pattern that the checker is de facto trying to find. The checker is usually fine seeing a pointer allocated in a top function and released in a sub-block. If the bottom frame is over-releasing, we'd warn, but it wouldn't be simply because the bottom frame is over-releasing, so mentioning the particular stack frame may end up being misleading.

In D35216#856093, @rsmith wrote:

The CXXStdInitializerListExpr node has pretty simple evaluation semantics: it takes a glvalue array expression, and constructs a std::initializer_list<T> from it as if by filling in the two members with a pointer to the array and either the size of the array or a pointer to its end. Can we not model those semantics directly here?

Avoid creating a new RefVal kind.

Fix a bit of ObjCBoxedExpr behavior.

I believe a custom operator new (or new[], regardless) can always return anything it wants. It can return a pointer to a concrete global variable, for example. So the only thing we can assert is that our operator is custom.

We can probably land D37189 first, and then land this patch with a test?

Thanks, LGTM!

Thank you for the review!

LGTM, thanks! Minor nit included. I hope our matchers actually cover all the cases (not a big deal if they don't though).

I guess later it'd be great to comment on every numTimesReached() why is it supposed to be reached exactly that many times, as it's often unclear, especially with tricky control flow. Because if somebody accidentally breaks the test, they'd have no idea what it tests or why it should work this way.

LGTM!

Yeah, LocAsInteger is supported very poorly in most places. We can only get away with it because people rarely cast pointers to integers.

I guess we'd need more assertions to catch invalid symbols, will have a look.

All right then, i approve!

Yeah, looks good.

The heuristic makes perfect sense to me.

I see, it seems that we've constructed $x - 1 somewhere, where $x is a pointer, while such stuff normally looks as &element{SymRegion{$x}, -1}. I guess i'd have to take a more careful look at it soon.

Yeah, line 86.

Thanks, works now! Apart from the minor comment on the hanging header file in the tests, this looks good and i have no further nits :)

Most importantly, do you think we can enable the checker by default now? Was this checker evaluated on any large codebase, and if so, how many true/false positives did it find, probably compared to the old checker?

Tests are still not working - they pass now, but they don't actually test anything. Please make sure that tests actually work. Which means that

1. Tests pass when you run make -j4 check-clang-analysis;
2. Tests start failing when you change your code or expected-warning directives.

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!