In D72705#2274568, @balazske wrote:

I am not sure if this checker is worth to further development. A part of the found bugs can be detected with other checkers too, specially if the preconditions of many standard functions are checked (more than what is done now) and by modeling the function calls with assumed error return value.

@martong has been working hard on libc support for the analyzer, I'll add him :)

Congrats on your GSoC! Unless I missed it, it seems like you haven't posted your final evaluation on cfe-dev, even though its an amazing looking document with a lot of examples and explanations. I think it would be great to spread the message, its a work to be proud of!

Yup, we've talked about this before. This is indeed a better interface design. LGTM!

A joy of reviewing C++ code is that you get to marvel in all the great things the language has, without having to pull fistfuls of hair out to get get to that point. These patches are always a treat. LGTM!

LGTM! Thanks!

The fix is great, thank you so much! The test seems to be annoying, it might be worth looking into it some other time, but that is definitely orthogonal to this patch.

I figured you're still working on this, sorry! I'd really like to chat about my earlier comment D71524#1917251, as it kind of challenges the high level idea.

There are some immediate observation to the patch, like a new kind of check definitely warrants a different warning message. However, I see that this is WIP patch, so I'll try to keep this high level.

Post-commit LGTM on the post-commit changes!

Sorry for the slack (which is kind of ironic with my attention grabbing title :) ). Landed in rG791b7e9f73e0064153a7c3db8045a7333a8c390c. Thanks everyone!

@balazske may have some closing words.

Rename the live statements checker to live expressions checker. The test file added in a revert commit changed rather heavily, but it makes sense that these entries are removed IMO. Unless somebody objects, I intend to land this as-is.

Reinstate the assert removed in rG032b78a0762bee129f33e4255ada6d374aa70c71, add a test. Enforce that Environment only makes an exception for ReturnStmt in terms of non-expression statements (and hope that isn't going to stay that way for long).

In D86736#2257880, @NoQ wrote:

i'm very glad that we now consistently use expressions in our Environment.

In D79431#2263693, @martong wrote:

In D79431#2263690, @martong wrote:

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

LGTM on my end!

In D86135#2259325, @steakhal wrote:

Perfectly clear, thank you. However, I would still rely on the others to accept this :|

BTW why does the plist-macros-with-expansion.cpp.plist change? It makes the diff somewhat noisy :s

This is exactly how I imagines weak dependencies to work. LGTM on my end.

Added some documentation to the code snippet pointed out by @martong.

Added a line about D78933.

I can only imagine how long it took for you to write all this, because reading it wasn't that short either. Thank you so much! It really gave me a perspective on how you see this problem, as well as what is actually happening (and should happen) in the checker.

@NoQ in particular has been working hard on the common infrastructure in between the static analyzer and clang-tidy, I'll add him :)

I'm in favor of most, if not all of the changes, though I will admit that this patch seems pretty cluttered, you are doing a lot of refactoring under the same hood. You're moving, adding, removing and changing helper functions and their invocations. Would be possible to make this patch a bit leaner?

@balazske seems to be very involved, he might have some closing words -- from my end, LGTM!

The tests look great, thanks! I still lack the confidence to accept, unfortunately.

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

LGTM! Nice!

In D72705#2241542, @balazske wrote:

The summary of this last discussion is that it is not acceptable to have only the simple check for the explicit comparison with a fixed constant. At least not for return types where the "implicit" check (a check that is always true or false for the error return value) is possible, for example the char* case.

In D86736#2249920, @martong wrote:

I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days.

Before we dive into this too much, if you can point to discussion that explains why we have a 2 versions of the same checker, that would be nice. Why did you chose to work on this one, and not the other? I recall us talking about this in a meeting, but its always great to have it set in stone.

I'd prefer to have a couple more green lights. In particular, another look on the constraint manager and Objective C stuff would be great.

In D85351#2247037, @baloghadamsoftware wrote:

In D85351#2215547, @Szelethus wrote:

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Why should we? This is just a fix for cases not covered, but it is the same functionality (retrieving the return value under construction). I added the missed cases to the test of this exact functionality.

In D86736#2244365, @martong wrote:

For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one

Shouldn't we try to fix the ObjCForCollectionStmt in the AST rather? By adding the condition to that as a (sub)expression? There are already many discrepancies and inconsistencies between supposedly similar AST nodes, and people do fix them occasionally (e.g. D81787).

I'm not thrilled by this solution. As I understand it, the assertion was put there to enforce our ability to create a new assumption, and its great that we had it, because we managed to catch a fault in that. Seems like this patch is treating the symptoms instead of curing the illness. Given that the code would probably work fine in builds with assertions disabled, I'd prefer to take a look at where the actual problem is. If it turns out to be unsolvable or require unreasonable amount of work, we can still decide to land this as-is.

Fixes according to reviewer comments!

Nice, thank you! Did you stumble across this, or found it with a tool?

Fixes according to reviewer comments!

I debated this report with @bruntib, depending from where you look at it, its either a false positive or not -- in short, this is what happens:

char *p = /* some assumed to be valid c string */
end = strchr(p, '\n');
if (end - p > 4) { // <-- warning here on end's invalid use
}

the guarded code wouldn't be executed, should end be zero. Now, if we interpret the rule in a very literal sense, that

I'll get the nits I didn't object to fixed, thats the status you're looking for :)

@vsavchenko I just realized a significant portion of your work for this release is the following list:

git log llvmorg-11-init..llvmorg-11.0.0-rc2 --oneline -- clang/utils/analyzer/

Is it a correct guess that while your primary audience are the analyzer developers, it wouldn't hurt to mention it in the release notes?

We will need to push this to the 11.0.0. release branch as well, sorry for the trouble.

Thanks! I'll get these fixed.

Documentation under clang/docs/analyzer/checkers.rst seems to be missing. Could we get that fixed before 11.0.0 releases?

In D86135#2233611, @martong wrote:

The fundamental problem is, we simply can't ask Preprocessor what a macro expands into without hacking really hard.

Can you summarize what is the exact problem (or give a link to a discussion, etc)? Is it an architectural problem in Clang itself?

What a legacy.

Do I sense correctly that the only information CSrtingLengthModeling.cpp requires from the actual CStringChecker is a checker tag? Because if so, I think we should just separate them even more cleanly -- we could just make a CStringLengthModeling checker implement the checker callbacks in that cpp file and we wouldn't need to move CStringChecker to a header. Not that moving it there is fundamentally bad, but in this instance it seems like we're legalizing bloating checkers instead of separating them. Also, this patch seems to have a significant overlap with D84979 -- is this intentional?

I really like the patch, but have nothing to add to what other reviewers already said. Nice!

In D67422#2216137, @NoQ wrote:

I agree that there's something here and also that it's not that urgent/critical to rename things at this point. It's not that people suffer horribly from having to link to only some of these things.

Unless anyone else objects, I'm happy to see this landed! Nice work! ^-^

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Lets make sure we invite the wider community to see whats going on. Otherwise, LGTM!

Tests c:

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.

Layering violations are a running theme in the analyzer -- CheckerRegistry and the entire MallocChecker fiasco are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry about it much.

In D72705#2199333, @balazske wrote:

Test results for tmux are available here. (Could not manage to get other results uploaded.)

Yeah, this looks straightforward, but how come you didn't manage to get a small test case? creduce gave up?

LGTM on my end, but please wait for approval from either @gamesh411 or @NoQ.

Would it be possible to publish these results on a public CodeChecker server?

We definitely need more tests. The idea overall however is amazing, thanks!

In D67422#2192407, @NoQ wrote:

One bit that's not directly related but i decided to keep it from the original patch was moving the plist-html diagnostic builder function into its own file.

Let me double down -- just because we let some select people know about an option, I still don't think it should be user facing. I'm strongly against the philosophy that a core-modifying decision, such as configuring a state split should be presented on the default interface. Having "smart users" isn't an excuse to that either -- the last thing I'd like to see broadcasted is that you need to be a genius* to use or configure a tool that is suppose to serve you, not the other way around.

Lets make sure that we invite others from the community to participate, here and on other patches as well. Otherwise, LGTM.

In D82598#2172416, @NoQ wrote:

I still wonder what made this statement live in my example. There must have been some non-trivial liveness analysis going on that caused a statement to be live; probably something to do with the C++ destructor elements.

Wow, I never realized I accidentally landed that assert (D82122#2172360), but I guess its great to have that covered. Would you prefer to have that reverted as I'm looking to fix this for good?

This was accidentally committed as a part of rGb6cbe6cb0399d4671e5384dcc326af56bc6bd122.

Thank you so much, you really went out of your way to dig that out! I'll try to patch this somehow.

In D84316#2171270, @NoQ wrote:

Imagine something like re-using the state trait implementation between MallocChecker and StreamChecker because they both model "resources that can be deallocated twice or leaked" - regardless of the specific nature of these resources. These checkers can implement their own API modeling maps, escape rules, warning messages, maybe model additional aspects of their problems, but fundamentally they're solving the same problem: finding leaks and overreleases of resources. This problem should ideally be solved once. This is why i advocate for abstract, generalized, "half-baked" state trait boilerplate implementations that can be re-used across checkers.