Whoaaa, at a glance this looks absolutely fantastic. I'll get back to this tomorrow to take a more careful look.

*appreciates CFG tests*

Thanks from me as well.

Looks great, thank you!

Thanks! I'm happy to accept this as soon as other reviewers are happy.

In D66338#1633249, @xbolva00 wrote:

Side note: As shown in this example, attribute((const)) on declarations seems like a bad idea. User may add attribute((const)), then in the future introduce a side effect aaaand... compiler is happy, optimizers are happy, no warnings, weird binary. Can clang-tidy catch it? @aaron.ballman @NoQ GCC folks claim this is hard to do and say attribute((const)) is a best effort thing - you simply shouldn't do a mistake.

Thanks! This looks very useful.

In D66267#1630728, @Szelethus wrote:

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

This code is super messy. It'd be great to actually test this change on something to see if it has any unwanted side effects.

Cute!!

Thanks, i appreciate!

One more quick question: Who is Husi???

In D65182#1629192, @Szelethus wrote:

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config. But I don't insist :)

I didn't make much progress so far, but i marked the test as // UNSUPPORTED: darwin in rC368765, so the buildbot is now green starting with http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6519/.

Fxd.

A quick archaeological dig suggests that i had no concerns with turning on unrolling by default back in 2017: http://lists.llvm.org/pipermail/cfe-dev/2017-August/055221.html - and i don't think i had any new concerns since then. I guess it'll be beneficial to make one more large-codebase run to make sure it still works, but other than that the only reason it's not enabled by default yet is because we forgot to do that :( This patch is good but not a must-have to turn loop unrolling on by default.

Add a "not implemented yet" assertion for kinds of fixits that are not supported as of that patch (but will definitely need to be supported in the future).

For me ddd() doesn't call c::c(). I can fix it by adding the following code:

void VisitCXXConstructExpr(CXXConstructExpr *CE) {
  addCalledDecl(CE->getConstructor());
  VisitChildren(CE);
}

I don't see why it would work without that code, as CXXConstructExpr isn't a sub-class of CallExpr. So that's another obvious omission in the CallGraph.

Guys, thank you so much for working on establishing consensus on this change.
*bows*

Rebase!

Make the checkers independent and extract modeling into a dependency.

I'll merge D65180 into this patch because it's otherwise too painful to update. Like, D65180 untangles bug types and controls enabledness by initializing bug types, but in order to untangle the checkers into a pair with a common dependency, i already need such control, and i don't want to implement a temporary solution only to discard it immediately.

Merged into D64274.

Wish granted!

Do it! 8)

Fxd!

Hmm, the patch doesn't pass its own test for me, could you double-check?

I'm convinced, let's keep everything as is but turn this into an -analyzer-config flag. We generally wanted to reduce the number of frontend flags because they are more taxing on backwards compatibility, so it makes perfect sense.

In D66042#1627842, @Charusso wrote:

@Szelethus, here is a really cool example: https://clang.llvm.org/docs/ClangCommandLineReference.html.

In D66042#1627777, @Szelethus wrote:

Yup. We will be able to present the drastic improvement made on LLVM analyses, while giving us some breathing room to polish a long-term solution. I suspect -analyzer-config silence-checkers=core -analyzer-config silence-checkers=something.Else wont work (I believe the last value will be used in the invocation), but if only scan-build is using it, we can do -analyzer-config silence-checkers=core,something.Else.

Thanks!

In D66042#1626631, @Szelethus wrote:

In D66042#1626513, @Charusso wrote:

I really appreacite your ideas. It is unbelievable you guys bring up 20 different ideas for 5 LOC. I cannot really argue about any idea, as every of them could be a possible solution. I have to pick the right solution, and drop the other 19. I believe with that in mind what is an experimental feature and how we support to use the Analyzer, none of the 19 ideas would born. I did not want to refuse that many ideas, but I have to, because we could pick at most 1 to implement per patch. That is why I really try to emphasize it is under that experimental feature umbrella and we have to think no more about that patch from that point: since the beginning.

Given our discussion, we've thrown out all but 1 of the 4, by the way (fixing the actual problem, making this a config, creating checker/package options, solving this in scan-build only), ideas. Make this a config. You're correct, thats about 5 LOC change in this patch, at which point I'd be happy to accept :)

In D66042#1627193, @alexfh wrote:

Should this be different with clang and this patch?

I'd like to hear @Szelethus's opinion one more time on this patch. I'm perfectly fine with abandoning the idea and going for in-checker suppressions, simply because there's at least one strong opinion against it and i don't want to push this further, but i just honestly think this patch is a good idea. This discussion has so far been very useful regardless, at least to me personally.

Oh, these false positives! Thanks!!

use it locally

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

In D66042#1625000, @Szelethus wrote:

if we add this flag, people responsible for developing interafaces for the analyzer might end up using it.

In D66042#1625235, @alexfh wrote:

In D66042#1624081, @NoQ wrote:

+@alexfh because clang-tidy now finally has a way of safely disabling core checkers without causing crashes all over the place! Would you like to take the same approach as we picked in scan-build, i.e. when the user asks to disable a core checker, silence it instead?

clang-tidy's native way to enable/disable diagnostics is applied to the static analyzer twice: first time when the list of enabled checkers is created (and then core checkers are always added to that list), and the second time - to each diagnostic generated by the static analyzer (this time the original check name filter is applied, without core checkers). This already works consistently from a user's perspective: https://gcc.godbolt.org/z/MEvSsP

Are there any benefits in using the new CheckerSilenceVector mechanism in clang-tidy?

In D65575#1625738, @Szelethus wrote:

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

In D66042#1624697, @Charusso wrote:

StringRef("osx.cocoa.RetainCountBase").startswith("osx.cocoa.RetainCount") is true, so there is no real issue until we manage the prefixes well. I assume that the user who knows how to disable/silence a checker, knows where to read how to disable/silence it. At least with scan-build there will not be pitfalls with disabling the core modeling.

My idea here was that this new feature isn't going to be user-facing. We aren't promising to support all combinations of enabled-disabled-silenced-dependent-registercheckerhacks, but instead use the new feature when we know it'll do exactly what we want. It is going to be up to the user-facing UI to decide how to use this feature, but not up to the end-users who simply want to silence diagnostics.

I like where this is going! I'll add myself so that not to forget to review the CFG bits next week.

But i definitely like it how smooth this patch turned out to be!

+@alexfh because clang-tidy now finally has a way of safely disabling core checkers without causing crashes all over the place! Would you like to take the same approach as we picked in scan-build, i.e. when the user asks to disable a core checker, silence it instead?

Fix un-singed options.

Fxd!

• Hide all current fixits behind a checker option, have them off by default. They're not tested enough yet.
• Suppress the dead stores fixit when the initializer has a side effect.
• Implement fixits-as-remarks for testing purposes. This is better than nothing, but i'd rather have something similar to clang-tidy where the test applies fixits and tests the resulting code.
• Add documentation for BugReport::addFixItHint().

In D64274#1600834, @Szelethus wrote:

If either checker emits an error, the current implementation would say it originates from cplusplus.PureVirtualCall. Could you please create a new ProgramPointTag with the correct checker name for optin.cplusplus.VirtualCall? You may retrieve that name through CheckerManager::getCurrentCheckName().

• Fix checker names for consumers that display them (eg., clang-tidy). Add a test.
• Change bug descriptions: "Call to virtual function during construction or destruction" -> "Pure virtual method call" | "Unexpected loss of virtual dispatch"
• Remove the suggestion to explicitly qualify the call. It is incorrect for nested stack frames, as it may affect the behavior when the same function is called from elsewhere.
• Make the category reusable, fix capitalization to stay similar to other bug categories.

This will slightly skew Static Analyzer exploration order, not necessarily in a good way, as default arguments and (sometimes) default initializers are not modeled properly. But this doesn't sound dangerous, because whether functions will be analyzed at top level doesn't depend on where they're positioned in the CallGraph, but instead on whether they've been actually visited during symbolic execution. So it's mostly about order rather than coverage. So i'm not worried.

Do you have commit access or should i commit this for you?

Aha, ok!

Looking good now! I still recommend eventually adding tests with casts of references.

Accept as long as we agree on the wording as part of D65575.

Nice!!

Thx again!

Yay!

*bows down to those who bestow documentation upon this code*

Do it!

Aha, aha, looks reasonable. If we're describing a variable, we should only highlight that variable.

Ugh, there seems to be one more forgotten buildbot with plugins problems: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6406/consoleText

Fair enough!

I thereby confirm that i've no idea what was this code about.

Sry, should have approved this ages ago >.<

Fantastic! Let's open the wording bikeshed season?

Ah, omission of the century :/

In D62525#1607813, @Szelethus wrote:

In D62525#1523026, @baloghadamsoftware wrote:

The problem is that the transition is added in the top level function but iterator position adjustments happen in the lower level ones. Data flow is one-way, top-down. For example, an insertion happens into a vector that invalidates all the iterators at and after the position where it is inserted. In this case handleInsert() calls a function that iterates all the active iterator positions and calls other one that invalidates the ones affected by the insertion. To propagate back the list of iterators invalidated would be too complex. Increments and decrements are even worse. Thus here the correct way seems to find the relevant transitions in a visitor instead of trying to figure out what note tags to add at each transition.

Mhm, I personally find this reasoning sufficient.

In D65382#1605769, @baloghadamsoftware wrote:

Most managers are stateful because they also store the elements they manage (e.g. ProgramStateManager stores states, SValBuilder owns other managers such as SymbolManager that stores symbols etc.)

In D65182#1604280, @baloghadamsoftware wrote:

Hmm, I was thinking on the same for some time but I wonder how many checkers could find the correct fixits? Maybe the removal fixits of double frees or double file closes, but I am afraid that for most of our path-sensitive checks there are no obvious fixits. Even clang-tidy cannot provide a fixit for most of its findings. However, generally I like the idea, even for the few checkers it can be applied to.