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.

There should be no UndefinedVals in function arguments unless you disable core checkers (which isn't supported).

Aha, ok, sounds like the right thing to do. Like, for Z3 it's actually the wrong thing to do (you'd prefer to evaluate the cast perfectly by adding SymbolCast) but for pure RangeConstraintManager this is the lesser of two evils.

This is one of the scripts that really deserves tests outside of buildbots. Like, it's not uncommon to use it "by hand".

Fxd.

In D85319#2197104, @rjmccall wrote:

This seems a huge architectural change that we need to talk about.

Whoops! Looks great now, i have no further comments, let's land :)

Get rid of the "V1" suffix. Indeed, we'll just rename the API.

Generally i think this is shaping up nicely, with the couple of minor nits addressed i'm happy to land this. Like, there are a few cornercases that we could address but we could do that in follow-up patches. Folks, do you see anything else here?

Aaaaand rebase. Addressed comments. With @Szelethus's refactoring work the patch actually looks much cleaner than it used to. 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.

These patches look like they're done, maybe let's land them?

These patches look like they're done, maybe let's land them?

That's a fair point.

*standing ovation*

Re-do the comments. Rename GetIssueHash() to getIssueHashV1 because there's a high chance we'll want more different kinds of hashes (also starts with a small letter, while we're at it).

Aha, ok, yeah, so that's what caused it!

We absolutely needed this :)

Thank you!

BuiltinBug is deprecated and we should be using BugType anyway

Whoops, i meant to post the comment in https://reviews.llvm.org/rG22a084cfa337d5e5ea90eba5261f7937e28d250b#937772 here. Anyway, i found a crash caused by this patch.

This patch is causing a crash when path-insensitive checkers emit the same report multiple times in a multiple instantiations of the same template. Here's an example:

I think the overall idea is correct.

I believe i clarified above how this checker should work. For testing purposes (which is the only reason to ever use the minimal output) i prefer to have it at the end of path, because that tells more about the bug path and therefore more important to test.

Update comments with the updated investigation details.

In D84453#2171548, @njames93 wrote:

I was having issues with this test case under macos in D82188.
It would fail for seemingly no apparent reason until I disable a test in a different translation unit.
This made me think there is a subtle bug in the linker used on macos. That could also explain why asan is having a hard time with this as well.
I got as far as seeing that runCheckOnCode was called, the check was instantiated but its matchers never got registered effectively meaning it didn't run.

In D83678#2155302, @bruntib wrote:

I was also thinking of merging the functionality of alpha.security.ReturnPtrRange into an existing out-of-bound checker. I found that the implementation of alpha.security.ArrayBound is 90% copy-paste of this ReturnPtrChecker, so this seems to be a good candidate. The main difference is that ArrayPtrRange checks all memory usages (Checker<check::Location>) and ReturnPtrRange checks return statements specifically (Checker< check::PreStmt<ReturnStmt> >). The former one is supposed to be more general, nevertheless, ReturnPtrRange emits reports on some inputs where ArrayBound remains silent (Of course I'll check the other direction later too). I inspected such a report and the reason was that the returning pointer was not used by the caller. I think it is not painful losing these (false positive?) reports of RetunPtrRange, since in practice it is safe to return the address of an undefined location as long as it's not used.

So would you agree if I'd continue the investigation in this direction, where the solution is the removal of ReturnPtrRange checker and maybe some improvement of ArrayBound checker? Moving ArrayBound out from alpha state could be a next step later.

In D83961#2166903, @balazske wrote:

Every other test failure comes from RetainCount checker except malloc-plist.c.

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.

Ok, here's the crashing example with ObjCForCollectionStmt. It should be saved as an .mm file and it crashes under pure --analyze.

@interface Item
// ...
@end

In D84316#2168730, @steakhal wrote:

In D84316#2168462, @NoQ wrote:

Such separation also looks amazing because ultimately the first part can be made checker-inspecific (i.e., a reusable half-baked trait that can be instantiated multiple times to track various things regardless of their meaning).

Could you elaborate on the latter part? (instantiated multiple times...)

In D84316#2168730, @steakhal wrote:

I wanted to have a separate class for bookkeeping while minimalizing the necessary changes.
What do you think would be the best way to organize this separation?

Few notes:

• Checkers are implemented in the anonymous namespace, so only the given file has access to them.
• I wanted to separate the bookkeeping logic from the reporting/function modeling logic in different files.
• I like the fact that after the change the CStringChecker implements only the evalCall checker callback.

Let me know if I misunderstood something.

Shared accessors look amazing.

In D83660#2166917, @steakhal wrote:

Although I'm not the most experienced one here, I think it's good to go.

In D83660#2149470, @OikawaKirie wrote:

It seems to be better to write a checker to report assertion failures.

In D82598#2164363, @Szelethus wrote:

Hmm, interesting. I don't really understand why do we need to keep that block live, as we definitely won't use any of the value it provides (since it does not provide a value at all).

Actually, what I said initially is true:

[...] the only non-expression statements it queried are ObjCForCollectionStmts [...]

so I think it'd be okay to simply drop this.

Forgot the phabricator link T_T
Committed in rG0b2a9222463.

In D84084#2160252, @Szelethus wrote:

In D84084#2160089, @Charusso wrote:

I do not get why my solution -analyzer-config silence-checkers=core.NullDereference -analyzer-config silence-checkers=core.DivideZero was insufficient, whereas -analyzer-config silence-checkers=core.NullDereference;core.DivideZero is working.

Because analyzer configs only hold the last value to be friendly towards scripts (just like most other configs/flags in clang).

I think this looks great!

Sizes assigned to the projects in this commit, do not directly
correspond to the number of lines or files in the project.

These will be updated if the code change looks good.

In D83120#2152919, @Szelethus wrote:

Unless @NoQ has anything else to add :)

Remove the ShouldDisplayPathNotes option as it never was an AnalyzerOption to begin with and it only makes things worse.

@vsavchenko and others just joining: The backstory and the overall plan are in this thread: http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html. Continued at http://lists.llvm.org/pipermail/cfe-dev/2019-September/063229.html.

Rebase!! This work was on hiatus but i plan to continue.

Looks like a copy-paste error indeed.

Sorry!! I was sure i pushed this. Thank you Valeriy.

Box-and-whisker diagrams 👍

Maybe spend a few minutes remeasuring libsoundio more carefully, just in case? Also i really wish we had per-TU data of that kind, to see if any particular TUs have regressed significantly.

This is a good patch!

Folks, please announce your overall plan for Z3 (unless i missed it). It looks like you are planning to actively support Z3 as a constraint manager, i.e. invoke it on every assume()? Did you consider my usual suggestion to only apply it to refuting non-buggy states in order to combat your false negatives similarly to how our current Z3 refutation refutes buggy states to combat false positives?

Before i forget again: commit messages (and, therefore, review titles) are traditionally written in imperative mood, i.e. "Using" -> "Use" as if you ask git to change something in the project.

AFAIK the only difference between %clang_cc1 and %clang_analyze_cc1 that the latter inserts a placeholder which is going to be replaced by the appropriate constraint manager.

ToolSubst('%clang_analyze_cc1', command='%clang_cc1', extra_args=['-analyze', '%analyze', '-setup-static-analyzer']+additional_flags),

That looks much better, thanks! I've a few more tiny annoying nits.

I think that the problem is actually with the second '/'

For my own education, what was wrong with join?

This looks great to me but i'd very much rather have someone else have a look, just because of the nature of the problem (: