Aha, great, sounds like all casting can be made more correct, not just casting on store retrieve! Maybe it's worth celebrating through extra unittests on evalCast(). Thank you for looking into this.

In D88332#2299912, @jkorous wrote:

In D88332#2297374, @NoQ wrote:

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

Sounds interesting but I lack context to imagine the details. Would the map be still initialized by some ASTVisitor?

This actually looks correct to me but i suggest a bit more investigation. Architecturally, it doesn't make sense to me that CastRetrievedVal() does anything beyond svalBuilder.dispatchCast(V, castTy). After all, there's only one way to cast a value from one type to another, and svalBuilder.dispatchCast(V, castTy) was supposed to be the ultimate method to do so.

Looks great, thank you! I think you can commit it :)

i'm pretty worried about our ability to actually achieve that in the near future

With Z3 constraint manager you absolutely want your constraints to be as precise as possible. The only reason we don't add these casts is because it confuses the constraint manager a lot. With a better constraint manager we would have spared ourselves a lot of suffering in this area.

Aha, ok, thanks, I now understand what the problem is because I was able to run the test before the patch and see how the patch changes the behavior.

A value of an expression should have the same type and value-kind as the expression.

No, you got it all wrong again. I don't want to explain this one more time so let's talk about some basics: A value of an expression should have the same type and value-kind as the expression. Can we get there? How?

But that's for a different bug, no? Even if that patch somehow dodges *this* crash, the underlying problem of us being unable to account for mutability of VLA sizes still remains to be addressed.

(in the latest message by "load" i mean the first load that produces a pointer (i.e., an ElementRegion) as the result)

I'm trying to say that the value produced by the load should not be the same as the stored value, because these two values are of different types. When exactly does the first value change into the second value is a different story; the current grand vision around which the code is written says that it changes during load, therefore it's the load code (step #2) that's to blame.

In D88477#2303376, @steakhal wrote:

In the Summary's example, at location #2 the value of **p is &Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char} - which is exactly what we stored at line #1.

void test(int *a, char ***b, float *c) {
  *(unsigned char **)b = (unsigned char *)a; // #1
  if (**b == 0) // no-crash, #2
    ;
  // ...
}

Yup, that's pretty bad.

The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

Thank you for reminding me of this patch. I still think it's a pretty important patch and i'm interested in getting it landed.

A load from a region of type T should always yield a value of type T because that's what a load is.

Nice, very interesting!

I also recommend using a specialized callback checkBind instead of pattern-matching binary operators specifically.

The code you've added looks correct. I suspect either heap invalidation or dead symbol elimination cleaning up the binding.

In D87800#2286971, @jkorous wrote:

Now, the current implementation still has false positives in the sense that it warns at every suspicious assignment without checking what is the actual state of LHS of such assignment at the point of return from the function.

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

We already have a similar simplification facility in SValBuilder created to solve the similar problem with iterator checkers. It fires up when it knows that the values it works with are limited to roughly 1/4 of their type's range and therefore none of the individual operations over them can potentially overflow (cf. D35109). It's currently off by default because performance was not properly evaluated and none of the on-by-default checkers rely on it. This is currently the intended approach to such issues. It was decided that constructing a custom solver for "non-overflowing but still bounded" arithmetic was not the right thing to do, mostly because it absolutely doesn't correspond to the actual run-time behavior of the program that we're supposed to be modeling.

A VLA in a loop may have different size on each iteration of the loop. This looks very much related to https://bugs.llvm.org/show_bug.cgi?id=28450.

Uh-oh, so you're saying that you're actively modeling every single operator + and - on every single integer in the program even when they don't represent any iterators at all? How can a raw integer be an iterator to begin with, given that you can't dereference an integer? Is this caused by D82185? Can you defend against such situations by checking that the operand is a pointer before doing any operations?

Aha, yup, thanks, this looks good!

Thank you, i think this is good to land!

(I.e., i don't have strong opinions about high-level design and will happily push the green button as soon as minor nits are addressed)

Sry! I'll try my best to keep up from now on.

My quick naive attempt of getting SVal of DeclRefExpr and checking if its MemRegion is in current stack frame didn't get very far because the returned SVal kind is undefined.

This looks correct to me. Thanks a lot. This hack was surprisingly crude and i'm very glad that we now consistently use expressions in our Environment.

What are types of each term in this expression?

Nice catch, thx!

Yup, that's more like it!~

I have no other concerns, i think this is good to go!

No-no, recursive locks are much more complicated than that, please do them separately. You'll have to discriminate between a real double lock / double unlock bug and an actual valid use of the recursive mutex in order to emit any warnings at all. That's completely different checker logic.

Looks amazing now.

Hans is pinging us on Bugzilla because this patch is marked as a release blocker (and i believe that it indeed is). I think we should land these patches.

If the numbers are confirmed to be as good as what i've sneak-peeked so far, that should be pretty amazing. Also whoa, test coverage!~

This patch looks correct to me at a glance. I think we should land it as is and debug/improve later.

In D86295#2230603, @steakhal wrote:

In D86295#2229712, @NoQ wrote:

Heh, nice! Did you try to measure the actual impact of this change on memory and/or performance?

Eh, I don't really know how to bench this.

This looks outright correct to me. I have random suggestions on note text here and there.

This is easier than D86293 because there's no old value in the freshly constructed smart pointer, right? I suspect that you can still re-use a lot of the implementation with D86293, at least partially.

Heh, nice! Did you try to measure the actual impact of this change on memory and/or performance?

Thanks!~

Excellent, thanks!

In D67422#2215371, @Szelethus wrote:

The patch looks great. I guess the only remaining discussion remains as to whether this should be in libAnalysis, or somewhere else. Here is my take: Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized variable warnings) are static analyzers within the same infrastructure, it makes sense for them to have a common library. I see that diagnostics aren't really analyses. libFrontend or libDriver, which, as I understand it contains most the diagnostics machinery for clang aren't viable alternatives because of the library dependencies. So, I think if libAnalysis was called libStaticAnalysisCommon, we would be golden, but that would screw over downstream developers for negligible gain. While I'm not terribly experiences in library layout design, for the time being, the move to libAnalysis seems appropriate.

Aha, i see, fair enough!

That's a fix for https://bugs.llvm.org/show_bug.cgi?id=46264.

I'm having second thoughts on re-using the existing builder. Most other runCheckers...() methods are building their own. Given how confusing this entire node builder business is, i believe we should not deviate from known working patterns.

Could you also unforget the diff context? >.<

Yes, i completely agree that this is the right decision, thanks!

You're on the right track but your checker repeats PthreadLockChecker word-by-word. Like, you can find answers to all your questions (eg., "how to use isLiveRegion?") by reading that checker. C++ functions aren't any different from C functions; that's an extremely minor difference that doesn't justify developing a new checker from scratch.

Also thanks, let's land!

This patch has already landed as rG152bc7ffdcd8f62b2279803642f162610154cd2e. I forgot the magic word.

Let's land this! Damn, we've learned a lot from this patch.

In D84520#2206077, @balazske wrote:

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

Umm, why don't you extend PthreadLockChecker instead?

In D85528#2204664, @steakhal wrote:

In D85528#2203325, @NoQ wrote:

Because this patch changes the behavior of regular analysis (without Z3), i expect tests to reflect that.

What do you expect exactly?

REQUIRES: z3 is necessary for the refutation.
However, adding this requirement would not mean that this test will run if you have Z3 installed though.
You should add the extra llvm-lit param to enable such tests.
I don't want to repeat myself too much but D83677 describes all the details of this test infra fiasco.
I would appreciate some feedback there.

I can't say any of my ideas about better naming are exceptionally bright. The existing scale seems perfectly reasonable.

In D85545#2203661, @xbolva00 wrote:

Parentheses could work here

Yay great! Let's add such test?

What if the user did actually want to concatenate the strings? Eg., one of the strings in the list is long and clang-format suggests breaking it up for the 80-column limit which causes the new warning to appear. How would the user suppress the warning in this case?

Whoops fxd!

Let's not change warning text without good reasons. Too few of us speak English well enough to check the articles. The original text seems concise and on-point.