Whoops, sry, yeah, looks good, please commit!

Looks good otherwise, please commit.

I'd also rather stick to integer arithmetic and avoid using floats even in intermediate calculations. It'd be hard to make sure that no rounding errors kick in if we use floats.

A call to Derived::Derived() previously emitted no warnings. However, with these changes, a warning is emitted for Base::a.

Yay less code.

Looks good!

You might need to annotate the unused parameter to avoid a compiler warning.

Z3Solver takes Z3Context by value, but then stores a reference. It should take it by reference.

Mm, wait, nvm, it's a pointer.

SMTContext::getContext() returns the context by value, which means making a copy. It should return by reference.

Actually verify the CFG in the test.

Outdated by D44131.

Address my own comments.

Looks good with minor comments.

Looks pretty great!

Looks great, thanks!

Much symbols!

Uhm, so we had an alpha checker enabled all along? Thanks for patching this up!

That'd be a hell for you because when the container is updated you won't be able to easily find iterators all that iterate over it. Normally what you want to do is keep mapping iterators to container regions, and when the region dies, "freeze" the data (make sure it can no longer be mutated, through an assertion or, ideally, via the type system) and re-map the iterator to data directly.

Am I correct to assume that, since if (nodep) is undefined behaviour in this path (nodep is uninitialized), the static analyzer doesn't evaluate it?

That's right. You only need to mark "atomic" symbols (SymbolData) as live, and expressions that contain them would automatically become live. So i think you should just iterate through a symbol_iterator and mark all SymbolData symbols you encounter as live.

Yep, this definitely looks safe and sound in the current shape.

The case here is that the subtraction is generated by the CSA, as there is no subtraction in the example.

Code re-use!

Actually, yeah, add the comment.

Code re-use!

I think this looks good. There's a problem with missing construction contexts, but i guess that's not the checker's fault, so let's add a FIXME and commit.

Aha, ok, yeah, we should have seen this coming. Whenever a checker tracks pairs of objects, like containers and iterators, or strings objects and their internal buffers (D48522), or return values and out-parameters of the same function call (D32449), we should expect races on which of the two dies first. Such races are inevitable because any of the two may be arbitrarily stored for an indefinite amount of time. The test that you see failing is one of the tricky cases to understand because it's about liveness of a parameter region, which is a bit counter-intuitive.

As usual, the analyzer, unlike the compiler, should be prepared to handle three different situations:

Thank you!! Please commit.

+Adam because that's a lot of stuff he's interested in and his tests are affected.

I think @xazax.hun has a point. We should not be intersecting any ranges because older ranges (that are closer to the root of the graph) are always super-sets of the newer ranges. Essentially, for every symbol we need to take either the final range (if it's present in the last program state) or the latest range (from the last state in which it's present) and feed it to the solver. That's all. No intersections. No PreStmtPurgeDeadSymbols program points. That's a fairly normal visitor workflow - that's exactly we have a pair of nodes as arguments to VisitNode. Just observe how state changes.

I like such commits.

I think we need to finish our dialog on who's responsible for initialization and why do we need to filter constructors at all, cause it's kinda hanging (i.e. D45532#inline-422673).

Looks good! One minor comment.

Looks great, thanks!

Make BugReporter great again. One bug report at a time.

We noticed this when debugging D47856, because the visitor was saying "va_list ended" right before the report, because the key is indeed removed from the program state just before reporting the issue, so it's not there in the issue state and is there in the state before that.

Only minor nits, as usual :)

Looks good tho!

Well, i guess that's just one of those mildly infuriating aspects of the AST and/or the standard :)

Ok, code makes sense to me now!

Aha, yeah, i see. It only invalidates the current stack frame, and additionally it's impossible to bring the reference into the current stack frame by reference, because, well, it's already a reference and you can't mutate a reference.

Also, great, and can i has tests?^^

Whoops, that was an old patch.

Ok, let's land this one and see how it goes! I'm looking forward to seeing the follow-up patches.

I still don't think i fully understand your concern. Could you provide an example and point out what exactly goes wrong?

In the iterator checkers we do not know anything about the rearranged expressions, it has no access to the sum/difference, the whole purpose of your proposal was to put in into the infrastructure.

This is supposed to suppress a few Inlined-Defensive-Checks-related false positives that accidentally spiked up during my testing of copy elision.

P.S. It seems that one of my currently-on-review patches has introduced a performance regression, i'm investigating it.

I added an option to disable copy elision on CFG side in D47616. The analyzer makes use of it automagically: elided constructors are replaced with temporary constructors in the CFG and the old behavior is restored.

Rebase.

Add a flag to disable copy elision. It's convenient to have such flag in the CFG because this way all clients will be able to transparently handle it. When the option is turned off, elided construction contexts will be replaced with simple temporary object construction contexts which need to be handled anyway. When construction contexts are disabled entirely, the option has no effect.

Fxd.