In D45920#1074437, @george.karpenkov wrote:

I could also move RangedConstraintManager.h under include

We probably don't want to do that: currently it can only be imported by files in Core, and we probably should keep it that way

In D45517#1074422, @rnkovacs wrote:

In D45517#1074057, @NoQ wrote:

So, yeah, that's a good optimization that we're not invoking the solver on every node. But i don't think we should focus on improving this optimization further; instead, i think the next obvious step here is to implement it in such a way that we only needed to call the solver once for every report. We could simply collect all constraints from all states along the path and put them into the solver all together. This will work because symbols are not mutable and they don't reincarnate.

Won't collecting all constraints and solving a ~100ish equations at once take a long time? Maybe the timeout limit for Z3 will need to be slightly increased for refutation then.

In D45920#1074439, @george.karpenkov wrote:

Another approach would be to instead teach RangedConstraintManager to convert it's constraints to Z3. That would be an unwanted dependency, but the change would be much smaller, and the internals of the solver would not have to be exposed. @NoQ thoughts?

Guys, what do you think about a checker that warns on uninitialized fields only when at least one field is initialized? I'd be much more confident about turning such check on by default. We can still keep a pedantic version.

A more accurate extension check.

Address most comments.

Checked this out, seems to be safely ignored for now. The .plist format is pretty resistant to this sort of stuff because most APIs to handle it are almost treating it as native arrays/dictionaries, so i guess it should be fine.

The visitor currently checks states appearing as block edges in the exploded graph. The first idea was to filter states based on the shape of the exploded graph, by checking the number of successors of the parent node, but surprisingly, both succ_size() and pred_size() seemed to return 1 for each node in the graph (except for the root), even if there clearly were branchings in the code (and on the .dot picture). To my understanding, the exploded graph is fully constructed at the stage where visitors are run, so I must be missing something.

Looks great, thanks!

Hopefully a more portable way of testing this stuff.

In D45698#1069121, @xazax.hun wrote:

I am in favour of this approach. This is what I suggested back than in https://reviews.llvm.org/D23014 but it was somehow overlooked.

Committed in rC330375 but i misplaced the phabricator link again, sorry.

rC330375 has nothing to do with this review; i misplaced the link in the commit message again.

(also we'll need CFG dump tests)

Hmm, yeah, indeed, i must have overlooked it, nice one :) I'll see if i can fix the other place as well.

Sorry, overwhelmed a bit, i'll try to get to this as soon as possible.

(i'd much rather do the latter)

Wow, you actually did that.

Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory space should be a whitelist, not a blacklist, because we can easily enumerate all stack variables and see which of them can be modified at all from the loop. But until we have that, this looks like a reasonable workaround.

Add tests where the argument is passed by reference. These tests work correctly (i.e. they correctly identify the argument constructor as a temporary constructor) because such constructors would include a MaterializeTemporaryExpr that is not allowed in the middle of the partial construction context. For the same reason remove the defensive check in the branch of ConstructionContext::createFromLayers that deals with MaterializeTemporaryExpr-based layers.

Yup thanks!

We've found a crash on our internal buildbot, would you like to have a look?:

In D45532#1064685, @Szelethus wrote:

In D45532#1064670, @Szelethus wrote:

I wasn't too clear on this one: unknown fields are regarded as uninitialized, what I wrote was a temporary change in the code so I could check whether it would work.

Woops, I meant that unknown fields are regarded az initialized.

Ok, thanks, looking forward to it!

That's a lot of code, so it'll take me some time to understand what's going on here. You might be able to help me by the large patch in smaller logical chunks.

I mean, like, if we try to work with the existing AST then we're stuck with a prvalue expression that represents an lvalue and will be assigned a Loc value, which is pretty weird anyway. Getting rid of the ParentMap in favor of providing enough context (eg. in the CFG or in checker callbacks) whenever we might want to use it sounds like a much saner solution. There might be other "unknown unknowns" about rewriting the AST, but our current problem looks as safe as we'll ever get.

In D45416#1062901, @a.sidorin wrote:

The ultimate solution would probably be to add a fake cloned asm statement to the CFG (instead of the real asm statement) that would point to the correct output child-expression(s) that are untouched themselves but simply have their noop casts removed.

I have some concerns about this solution. It will result in difference between AST nodes and nodes that user will receive during analysis and I'm not sure that it is good. What is even worse here is that a lot of AST stuff won't work properly - ParentMap, for example.

Yep. But i'd rather avoid using the ParentMap. Also we already do this for DeclStmts that declare more than one VarDecl (split them up into single-decl statements), and the practical effect of such simplification is barely noticeable even though DeclStmts are so much more common.

@xazax.hun: do you think the approach you took in the Valist checker is applicable here? Did you like how it ended up working? Cause i'd love to see CallDescription and initializer lists used for readability here. And i'd love to see branches replaced with map lookups. And i'm not sure if anybody has written code that has all the stuff that i'd love to see.

Eww. Weird AST.

The output looks reasonable to me, but we'll need to see if other consumers of the plist output (IDEs that supports the analyzer, such as Xcode) will be able to accept the modified output (at least, will be able to ignore it). I'll have a look.

Nice, thanks.

LGTM!

More LocAsInteger problems. We need more flexible symbolic expressions.

The huge switch in getAsOffset() does really nothing good compared to a virtual function. I suspect that this alone could have made it faster, depending on how the switch is compiled. And with a virtual function, we could also easily add the new field and the lookup only to those 3-4 regions that aren't base regions - which might as well be faster. But this already looks good, we should have tried this earlier!

Test?

Substraction is an additive operation. Added tests for that. Added even more tests.

Use auto.

Fix a comment in tests.

Conditional operators, like call-expressions in D44273, may also be glvalues of record type instead of simply being reference-typed.

I guess the interesting part here is what happens if only a but not b is used (or vice versa).

In addition, memset can bind anything to the region, so getBindingForDerivedDefaultValue()'s logic needs some adjustment. The solution in this patch is certainly not correct.

Why do you need separate code for null and non-null character? The function's semantics doesn't seem to care.

This looks good with a super tiny nit regarding comments about signed integers. George, are you happy with the changes? (:

Fair enough!

E.g. we have llvm::make_shared<>(), so we could also have Allocator::make_obj<...>(...)

I'd rather keep the helper method within the class. This allows us to keep constructors private, which is something i forgot to do originally.

Add a comment for the confusing part.

Address review comments.

In D44557#1042792, @MTC wrote:

In D44557#1042357, @NoQ wrote:

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Thank you for the code review, @NoQ !

The regression is maybe caused by https://reviews.llvm.org/D44075, my patch! CheckBufferAccess() may not guarantee checkNonNull() will be called. If that's true, please revert https://reviews.llvm.org/rC326782 and I'll fix it.

• fix auto.
• don't use regexs yet because it's clean enough anyway; maybe later.

We've had a 4x analysis time increase on a specific file due to this code interacting in weird manner with temporary constructor inlining.

I am only wondering what is the policy regarding the tests?

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Just in case: we indeed do not guarantee that SymbolConjured corresponds to a statement; it is, however, not intended, but rather a bug. Consider:

 1	void clang_analyzer_eval(bool);
 2
 3	class C {
 4	  int &x;
 5	public:
 6	  C(int &x): x(x) {}
 7	  ~C();
 8	};
 9
10	void foo() {
11	  int x = 1;
12	  {
13	    C c(x);
14	  }
15	  int y = x;
16	  {
17	    C c(x);
18	  }
19	  int z = x;
20	  clang_analyzer_eval(y == z);
21	}