Page MenuHomePhabricator

Charusso (Csaba Dabis)
Research

Projects

User does not belong to any projects.

User Details

User Since
Dec 25 2017, 5:51 AM (90 w, 6 d)

Recent Activity

Fri, Sep 20

Charusso added a comment to D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length.

Thanks!

Fri, Sep 20, 12:59 PM · Restricted Project, Restricted Project
Charusso updated the diff for D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length.
  • Fix.
Fri, Sep 20, 12:59 PM · Restricted Project, Restricted Project
Charusso added inline comments to D67079: [analyzer] CastValueChecker: Model inheritance.
Fri, Sep 20, 11:11 AM · Restricted Project
Charusso updated the diff for D67079: [analyzer] CastValueChecker: Model inheritance.
  • Try to do the math.
  • Create a consistent dynamic type API.
Fri, Sep 20, 11:11 AM · Restricted Project

Sat, Sep 14

Charusso added a comment to D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length.

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

Sat, Sep 14, 8:35 AM · Restricted Project, Restricted Project
Charusso updated the diff for D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length.

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.

Sat, Sep 14, 8:26 AM · Restricted Project, Restricted Project

Mon, Sep 9

Charusso updated the diff for D67079: [analyzer] CastValueChecker: Model inheritance.
  • Fix.
Mon, Sep 9, 10:34 AM · Restricted Project
Charusso added inline comments to D67079: [analyzer] CastValueChecker: Model inheritance.
Mon, Sep 9, 10:34 AM · Restricted Project

Mon, Sep 2

Charusso added inline comments to D67079: [analyzer] CastValueChecker: Model inheritance.
Mon, Sep 2, 8:15 AM · Restricted Project
Charusso created D67079: [analyzer] CastValueChecker: Model inheritance.
Mon, Sep 2, 8:10 AM · Restricted Project

Fri, Aug 30

Charusso accepted D67019: [analyzer] pr43179: CallDescription: Check number of parameters as well as number of arguments..

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

Fri, Aug 30, 1:45 PM · Restricted Project, Restricted Project

Sun, Aug 25

Charusso added a comment to D66721: [analyzer] Analysis: Prevent bitwise operation false positives.
In D66721#1644531, @NoQ wrote:

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

Sun, Aug 25, 2:35 PM · Restricted Project
Charusso added inline comments to D66721: [analyzer] Analysis: Prevent bitwise operation false positives.
Sun, Aug 25, 1:43 PM · Restricted Project
Charusso updated the diff for D66721: [analyzer] Analysis: Prevent bitwise operation false positives.
  • Fix.
Sun, Aug 25, 1:43 PM · Restricted Project
Charusso created D66721: [analyzer] Analysis: Prevent bitwise operation false positives.
Sun, Aug 25, 11:50 AM · Restricted Project

Sat, Aug 24

Charusso committed rG0d7252b78369: [analyzer] Analysis: Fix checker silencing (authored by Charusso).
[analyzer] Analysis: Fix checker silencing
Sat, Aug 24, 5:19 AM
Charusso committed rL369845: [analyzer] Analysis: Fix checker silencing.
[analyzer] Analysis: Fix checker silencing
Sat, Aug 24, 5:16 AM

Aug 23 2019

Charusso added a comment to D66593: [analyzer] CastValueChecker: Fix some assertions.

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!

Aug 23 2019, 1:00 AM · Restricted Project

Aug 22 2019

Charusso abandoned D66593: [analyzer] CastValueChecker: Fix some assertions.

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!

Aug 22 2019, 8:50 PM · Restricted Project
Charusso added inline comments to D66593: [analyzer] CastValueChecker: Fix some assertions.
Aug 22 2019, 4:34 PM · Restricted Project
Charusso updated the diff for D66593: [analyzer] CastValueChecker: Fix some assertions.
  • The null MemRegion is a 0 (Loc), try to avoid to store it.
Aug 22 2019, 4:34 PM · Restricted Project
Charusso added a comment to D65239: [analyzer] RangeConstraintManager: Apply constraint ranges of bitwise operations.

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

Aug 22 2019, 4:16 PM · Restricted Project
Charusso updated the diff for D65239: [analyzer] RangeConstraintManager: Apply constraint ranges of bitwise operations.
  • Fix.
Aug 22 2019, 4:15 PM · Restricted Project
Charusso added a comment to D66593: [analyzer] CastValueChecker: Fix some assertions.

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

Aug 22 2019, 7:29 AM · Restricted Project
Charusso created D66593: [analyzer] CastValueChecker: Fix some assertions.
Aug 22 2019, 7:27 AM · Restricted Project

Aug 21 2019

Charusso committed rG4d71600c1132: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull() (authored by Charusso).
[analyzer] CastValueChecker: Model isa(), isa_and_nonnull()
Aug 21 2019, 8:00 PM
Charusso committed rL369615: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
[analyzer] CastValueChecker: Model isa(), isa_and_nonnull()
Aug 21 2019, 7:59 PM
Charusso closed D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
Aug 21 2019, 7:59 PM · Restricted Project, Restricted Project
Charusso added a comment to D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().

Thanks for the review!

Aug 21 2019, 7:55 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
  • Fix.
Aug 21 2019, 7:55 PM · Restricted Project, Restricted Project
Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
   return C.getNoteTag(
-      [=] {
+      [=]() -> std::string {
         SmallString<128> Msg;
Aug 21 2019, 7:29 PM · Restricted Project, Restricted Project
Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.

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
Aug 21 2019, 7:06 PM · Restricted Project, Restricted Project
Charusso committed rG22dc44ff896a: [analyzer] CastValueChecker: Try to fix the buildbots (authored by Charusso).
[analyzer] CastValueChecker: Try to fix the buildbots
Aug 21 2019, 6:43 PM
Charusso committed rL369609: [analyzer] CastValueChecker: Try to fix the buildbots.
[analyzer] CastValueChecker: Try to fix the buildbots
Aug 21 2019, 6:43 PM
Charusso committed rGe4bf456fcef2: [analyzer] CastValueChecker: Rewrite dead header hotfix (authored by Charusso).
[analyzer] CastValueChecker: Rewrite dead header hotfix
Aug 21 2019, 5:40 PM
Charusso committed rL369607: [analyzer] CastValueChecker: Rewrite dead header hotfix.
[analyzer] CastValueChecker: Rewrite dead header hotfix
Aug 21 2019, 5:40 PM
Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.

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?

Aug 21 2019, 5:39 PM · Restricted Project, Restricted Project
Charusso committed rG0202c3596c52: [analyzer] CastValueChecker: Store the dynamic types and casts (authored by Charusso).
[analyzer] CastValueChecker: Store the dynamic types and casts
Aug 21 2019, 5:21 PM
Charusso committed rL369605: [analyzer] CastValueChecker: Store the dynamic types and casts.
[analyzer] CastValueChecker: Store the dynamic types and casts
Aug 21 2019, 5:20 PM
Charusso closed D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 21 2019, 5:20 PM · Restricted Project, Restricted Project
Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.

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.

Aug 21 2019, 5:19 PM · Restricted Project, Restricted Project
Charusso committed rGb73a5711f634: [analyzer] TrackConstraintBRVisitor: Do not track unknown values (authored by Charusso).
[analyzer] TrackConstraintBRVisitor: Do not track unknown values
Aug 21 2019, 5:13 PM
Charusso committed rL369604: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
[analyzer] TrackConstraintBRVisitor: Do not track unknown values
Aug 21 2019, 5:10 PM
Charusso closed D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
Aug 21 2019, 5:09 PM · Restricted Project, Restricted Project
Charusso added a comment to D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.

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

Aug 21 2019, 5:05 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 21 2019, 3:58 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Fix printing.
Aug 21 2019, 3:58 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Fix
Aug 21 2019, 3:46 PM · Restricted Project, Restricted Project

Aug 20 2019

Charusso updated the diff for D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
  • Simplify the template argument obtaining.
  • Added a tiny new test case.
Aug 20 2019, 7:52 PM · Restricted Project, Restricted Project
Charusso added a comment to D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().

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.

Aug 20 2019, 7:52 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Make the check-clang pass and simplify a test case.
Aug 20 2019, 7:15 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Remove CastFromTy finally from the getNoteTag() API as it was uninformative.
Aug 20 2019, 7:03 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 20 2019, 6:51 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 20 2019, 6:51 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Fix more and publish the previously forgotten comments.
Aug 20 2019, 6:51 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Added a new test case and refactored that test file.
Aug 20 2019, 2:18 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
Aug 20 2019, 2:04 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Fix.
Aug 20 2019, 2:04 PM · Restricted Project, Restricted Project

Aug 19 2019

Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
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.

Aug 19 2019, 4:57 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
  • Use a set factory to store a dynamic cast information set per memory region.
Aug 19 2019, 4:57 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 19 2019, 3:37 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 19 2019, 3:20 PM · Restricted Project, Restricted Project
Charusso added a parent revision for D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull(): D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 19 2019, 9:05 AM · Restricted Project, Restricted Project
Charusso created D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
Aug 19 2019, 9:04 AM · Restricted Project, Restricted Project
Charusso added a child revision for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts: D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull().
Aug 19 2019, 9:04 AM · Restricted Project, Restricted Project
Charusso updated the summary of D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 19 2019, 7:36 AM · Restricted Project, Restricted Project
Charusso updated the diff for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.

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.

Aug 19 2019, 7:34 AM · Restricted Project, Restricted Project

Aug 15 2019

Charusso updated the diff for D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
  • Rebased.
Aug 15 2019, 7:14 PM · Restricted Project, Restricted Project
Charusso committed rGa079a4270851: [analyzer] Analysis: Silence checkers (authored by Charusso).
[analyzer] Analysis: Silence checkers
Aug 15 2019, 6:55 PM
Charusso committed rL369078: [analyzer] Analysis: Silence checkers.
[analyzer] Analysis: Silence checkers
Aug 15 2019, 6:52 PM
Charusso closed D66042: [analyzer] Analysis: Silence checkers.
Aug 15 2019, 6:52 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

Thanks for the reviews!

Aug 15 2019, 6:50 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66042: [analyzer] Analysis: Silence checkers.
  • Rebased.
  • Added the remaining FIXME.
Aug 15 2019, 6:48 PM · Restricted Project, Restricted Project
Charusso added a parent revision for D66325: [analyzer] CastValueChecker: Store the dynamic types and casts: D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
Aug 15 2019, 5:27 PM · Restricted Project, Restricted Project
Charusso added a comment to D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.

@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.

Aug 15 2019, 5:27 PM · Restricted Project, Restricted Project
Charusso added a child revision for D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values: D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 15 2019, 5:27 PM · Restricted Project, Restricted Project
Charusso created D66325: [analyzer] CastValueChecker: Store the dynamic types and casts.
Aug 15 2019, 5:27 PM · Restricted Project, Restricted Project
Charusso added a comment to D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.

@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.

Aug 15 2019, 4:09 PM · Restricted Project, Restricted Project

Aug 14 2019

Charusso added a parent revision for D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values: D65889: [analyzer] CastValueChecker: Model castAs(), getAs().
Aug 14 2019, 5:51 PM · Restricted Project, Restricted Project
Charusso added a child revision for D65889: [analyzer] CastValueChecker: Model castAs(), getAs(): D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
Aug 14 2019, 5:51 PM · Restricted Project, Restricted Project
Charusso created D66267: [analyzer] TrackConstraintBRVisitor: Do not track unknown values.
Aug 14 2019, 5:51 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 1:47 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 1:09 PM · Restricted Project, Restricted Project
Charusso added inline comments to D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 12:00 PM · Restricted Project, Restricted Project
Charusso updated the diff for D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 10:13 AM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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.

Aug 14 2019, 6:57 AM · Restricted Project, Restricted Project
Charusso updated the summary of D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 6:56 AM · Restricted Project, Restricted Project
Charusso updated the diff for D66042: [analyzer] Analysis: Silence checkers.
Aug 14 2019, 6:56 AM · Restricted Project, Restricted Project

Aug 13 2019

Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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.

Aug 13 2019, 1:58 PM · Restricted Project, Restricted Project

Aug 12 2019

Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.
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.

Aug 12 2019, 10:35 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.
In D66042#1625971, @NoQ 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.

Aug 12 2019, 4:40 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.
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^^

Aug 12 2019, 4:03 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.
In D66042#1625971, @NoQ 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.

Aug 12 2019, 3:39 PM · Restricted Project, Restricted Project

Aug 11 2019

Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.
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(?)

Aug 11 2019, 8:30 PM · Restricted Project, Restricted Project

Aug 10 2019

Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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.

Aug 10 2019, 3:44 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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.

Aug 10 2019, 2:45 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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.

Aug 10 2019, 1:32 PM · Restricted Project, Restricted Project
Charusso added a comment to D66042: [analyzer] Analysis: Silence checkers.

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?

Aug 10 2019, 8:41 AM · Restricted Project, Restricted Project

Aug 9 2019

Charusso added inline comments to D66042: [analyzer] Analysis: Silence checkers.
Aug 9 2019, 8:19 PM · Restricted Project, Restricted Project
Charusso updated the summary of D66042: [analyzer] Analysis: Silence checkers.
Aug 9 2019, 7:57 PM · Restricted Project, Restricted Project