Thanks!

• Fix.

• Try to do the math.
• Create a consistent dynamic type API.

Hm, I wanted to upload the new patch here to see the changes with the diff-mode, but it does not work, sorry.

After a while I try to make this patch arrive. I wanted to split it up to multiple patches, but everything tied together so I decided to fix false positives instead with improving the existing APIs. Please visit the diff of the test cases and the documentation to see the changes.

• Fix.

I like the isolation of this kind of stuff. Thanks you!

In D66721#1644531, @NoQ wrote:

Now you're silencing the *whole* checker. This is equivalent to passing an -analyzer-silence-checker flag.

• Fix.

In D66593#1642253, @Charusso wrote:

Wow, it is great you could address every of the edge cases. Thanks you so much! I believe only one problem left: we need to return false instead of plain return in order to let the Analyzer model the function call. I could fix it easily, thanks!

Wow, it is great you could address every of the edge cases. Thanks you so much! I believe only one problem left: we need to return false instead of plain return in order to let the Analyzer model the function call. I could fix it easily, thanks!

• The null MemRegion is a 0 (Loc), try to avoid to store it.

In overall I wanted to keep the [A, B] shape of the tests, but now they are more precise, thanks!

• Fix.

I am not sure how would I fix them, so I just commented them out.

Thanks for the review!

• Fix.

In D66325#1640483, @thakis wrote:

Thanks! Looks like it builds fine now, but the tests are failing: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19297/steps/test-check-all/logs/stdio
Failing Tests (2):

Clang :: Analysis/cast-value-notes.cpp
Clang :: Analysis/cast-value-state-dump.cpp

In D66325#1640426, @thakis wrote:

It looks like this renamed DynamicTypeMap.h but didn't update all users, see all the files in Checkers at http://llvm-cs.pcc.me.uk/?q=include.*dynamictypemap.h for example.

Which targets did you try to build locally?

Thanks for the review! The build-bots will fire with that QualType fix (1028 TU on its own). I will look into the exploded-graph-rewriter.py after GSoC to fix every stuff like that patch and also invoke my HTML simplification idea.

Thanks for the reviews! I hope that mentioned error will be visible by the BugReporter revisions.

• Fix printing.

• Fix

• Simplify the template argument obtaining.
• Added a tiny new test case.

Thanks for the reviews so far! I had a contradiction in my mind about regions, but now everything is okay and the notes are not misleading.

• Make the check-clang pass and simplify a test case.

• Remove CastFromTy finally from the getNoteTag() API as it was uninformative.

• Fix more and publish the previously forgotten comments.

• Added a new test case and refactored that test file.

• Fix.

In D66325#1636091, @NoQ wrote:

Yay! I understand the rough idea and it looks perfectly reasonable to start with. Please add FIXMEs/TODOs on how we eventually want to support more complicated inheritance hierarchies.

• Use a set factory to store a dynamic cast information set per memory region.

This patch introduces DynamicCastInfo similar to DynamicTypeInfo which
is stored in DynamicCastSet similar to DynamicTypeMap. It could be used
to store and check the casts and prevent infeasible paths.

• Rebased.

Thanks for the reviews!

• Rebased.
• Added the remaining FIXME.

@xazax.hun It is somehow performance critical code as we have too many casts in the LLVM. I would really appreciate it if you could review it.

@Szelethus pointed out well. My patch is about nullability, and it is perfect. The bug report you made is about deduplication rather than nullability. The fact is we fail to call the deduplication function: eventsDescribeSameCondition() from removeRedundantMsgs(PathPieces &path) because the path does not contain the path pieces. We call removeRedundantMsgs with passing the PD->getMutablePieces(), but this only contains one piece of the top-level function call in the test case you have attached.

• Rainbow Butterfly Unicorn Kitty

I realized that it is meaningless what separator we use to list the checkers, so I have picked ;. The , is limited to the analyzer-config list.

• Pink Fluffy Unicorns Dancing On Rainbows

Any analyzer config flag is equally accessible to anyone as the driver flags as they are both flags. The only difference is the config flags are more code to implement, and a lot more difficult to use. @NoQ, why the hell would we pick another type of flag which makes zero improvement? The goal is to introduce the best possible solution, which is already here.

In D66042#1626460, @NoQ wrote:

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.

In D66042#1625971, @NoQ wrote:

In D66042#1625000, @Szelethus wrote:

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

And this is fine, that's supported. There's a very limited list of such people and we could talk to all of them easily if we wanted to. On the other hand, an end-user running clang --analyze -Xclang -flagflagflag manually on his desktop is not supported.

In D66042#1626122, @NoQ wrote:

use it locally

What do you mean here? If you want to use the patch for evaluating your results, you might as well untick the checker in the scan-build's index.html interface. The point of having this patch landed is to allow users who are worried by false positives of specific core checkers to use the Static Analyzer on their code anyway, without being overwhelmed with false positives. It was never about us, it was always about them^^

In D66042#1625971, @NoQ wrote:

In D66042#1625000, @Szelethus wrote:

Your patch title, and the things things that you said make me believe that there are only a handful of core checkers that cause headaches, how about you add checker options to those instead? If the entirety of the core is problematic, you might as well add a package option to it.

My impression is kinda the opposite: the more we put our hacks into the imperative checker code, the harder it gets. Checkers.td "just works", but whenever we try to mess with it, we always get something wrong. Which is exactly why i greatly appreciated your work to formalize everything in tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Generally, though, i think we're currently only talking about two checkers: the null dereference checker and the "calling a method on a null pointer" checker (which is, suddenly, a separate checker). @Charusso, do i understand correctly that we don't really want to disable uninitialized variable checkers (there are, like, 5 of them) because you've more or less fixed their false positives? Also we might want to disable MallocChecker but it doesn't really do any modeling and doesn't really belong to core either. If it's just two checkers, i definitely don't mind simply splitting them up for the purposes of GSoC, while letting us come to a consensus on this patch.

In D66042#1624684, @NoQ wrote:

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.

Here is a problem with your patch: How would you go about silencing diagnostics for osx.cocoa.RetainCount? I suppose -analyzer-silence-checker=osx.cocoa.RetainCount. The problem however, that the checker tag associated with it refers to osx.cocoa.RetainCountBase under the hood, so you'll need to silence that instead. From that point on, any other checker that suffers from the same issue is also silenced, that was not the intent.

Hmm, sounds like we need to hack up a fix for the checker tag on the bug node, so that it was appropriate to the presumed (as opposed to actual) checker name(?)

In D66042#1624475, @Szelethus wrote:

In D66042#1624469, @Charusso wrote:

In a long-term rewriting the Analyzer from scratch would be ideal. There is no problem with this patch, it will not cause any issues like that. May I would like to disable the apiModeling as well, with my patch it is one command. With your approach it would require to rewrite all of the existing apiModeling checkers after the core ones for no reason as we have better way: this patch.

Why would we need to rewrite apiModeling? Its hidden from users, so the issue of enabling/disabling them is not a big topic.

In D66042#1624468, @Szelethus wrote:

In D66042#1624462, @Charusso wrote:

My solution preserve the checkers as they are and yours definitely would rewrite them. Checker writing has tons of boilerplates, I think adding more should be avoided. Why would you touch the checkers as my solution is already implemented and working perfectly without any overhead?

Speaking about performance impact, note where your patch does the actual silencing: by the time control reaches that point, we created bug report equivalence classes, constructed a trimmed version of the exploded graph, constructed a bug path from that trimmed graph and ran all visitors on and have gathered all diagnostic pieces for it (plenty of shared object creations there). I have a strong suspicion that not even creating the bug report is far faster.

I think that adding yet another way of controlling checkers is confusing, and solves a problem that shouldn't exist in the first place. Now, that being said, the problem does exist, and I see this as an elegant solution for the time being.

In D66042#1624432, @Szelethus wrote:

In D66042#1624365, @Charusso wrote:

In D66042#1624320, @Szelethus wrote:

I think it would make a lot more sense to create a separate (and hidden!) coreModeling package that would do all the modeling, and regard core as a highly recommended, but not mandatory set of checkers. Wouldn't this create a cleaner interface?

Sadly separating a modeling package is impossible. The checkers subscription-based design is made for that purpose to make them both model the analysis, and both emit reports. None of the checkers has separated modeling and separated business logic, because they are so tied together. The reporting part fires on given modeling parts, like that is why they could report errors during modeling and stop the modeling. May if we redesign all the core checkers to separated business/modeling logic it would be helpful.

I didn't mean to do too invasive changes, only something like this, which would be tedious but not difficult:

struct ModelingChecker {
  bool AreDiagnosticsEnabled = false;
  void model() const;
};

void ModelingChecker::model() const {
  // whatever

  if (!AreDiagnosticsEnabled) {
    C.generateSink(State, C.getPredecessor());
    return;
  }

  static CheckerProgramPointTag DiagnosticCheckerTag(this, "DiagnosticChecker");
  const ExplodedNode *ErrorNode = C.generateErrorNode(State, &DiagnosticCheckerTag);
  // etc etc
}

void registerDiagnosticChecker(CheckerManager &Mgr) {
  Mgr.getChecker<ModelingChecker>()->AreDiagnosticsEnabled = true;
}

A solution like this would preserve the current checker structures while neatly hiding the implementation part.

In D66042#1624320, @Szelethus wrote:

I think it would make a lot more sense to create a separate (and hidden!) coreModeling package that would do all the modeling, and regard core as a highly recommended, but not mandatory set of checkers. Wouldn't this create a cleaner interface?