Szelethus added a comment. In D66042#1624469 <https://reviews.llvm.org/D66042#1624469>, @Charusso wrote:
> In D66042#1624468 <https://reviews.llvm.org/D66042#1624468>, @Szelethus wrote: > > > 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. > > > Let us say an average user using scan-build add that flag: `-disable-checker > core.DivideZero`. I am 100% sure the user meant that to disable that > checker's reports. Whatever it takes, the main modeling and main > graph-building is what burns the companies money on electricity bills. We > have tons of ways to suppress reports, because it makes zero overhead in cost. Then I guess overhead isn't a big concern to us after all :) >>>>> Also this patch aims to hide 600 `cast<>` related reports, and try to fix >>>>> that ambiguity to "disable a core checker" as we really mean that to do >>>>> not emit uninteresting reports. Personally I am really against the idea >>>>> to make the core modeling disable-able, this scan-build addition fix >>>>> that, you cannot disable it. If crash occurs with the core package then >>>>> we should give it a 9000 priority level on Bugzilla and `fix-it`. >>>> >>>> My wording may have been poor, apologies for the misunderstanding -- of >>>> course I do not intend to get rid of the modeling, just achieving the same >>>> goal from a different angle. `coreModeling` would be a dependency of all >>>> path sensitive checkers (we did talk about this in one of my patches), and >>>> the user would no longer be exposed to the option of disabling them, only >>>> their diagnostics. This is similar to the way how the checker dependency >>>> system was reimplemented as well, and I like to think that the analyzer's >>>> interface really benefited from it. >>>> >>>> What do you think? >>> >>> At the moment we have a way to disable core modeling because they could >>> break the user's analysis. My patch only touch the scan-build's invocation >>> as an **experimental** feature to make it impossible to disable the core >>> modeling **as a side effect** and **only trough scan-build**. Mainly the >>> idea was to use the fewest possible commands using scan-build therefore now >>> one `-disable-checker` command does two things. I hope that it is useful >>> for other users as well to "disable" the core checkers. >>> >>> However, if you have time to continue your dependencies patch to make the >>> core modeling non-disable-able with making the core checkers bulletproof, >>> that would be neat! But that could be some other patch and out of the scope >>> of that patch. >> >> Most definitely! Would you agree that in the long term such a division would >> be a more ideal approach? Because if we're commiting ourselves to silencing >> checkers, there may be some annoying things to fix too, like, if a checker >> emits a warning with the incorrect name (this is unfortunately not too >> uncommon, for example when the checker tag is associated with the modeling >> checker), the user would be confused why the silence didn't go through, or >> silenced far too many things. If this is just a band aid solution only for >> scan-build, only for core checkers, wouldn't an analyzer-config be better? > > 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. 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. I genuinely mean to cause no unnecessary headaches for you, but adding this is a significant API change that we'll have to support going forward if it gets into a release. If this is a quick fix to reduce the false positives on LLVM, I'm all for it, but would prefer to see a solution without such a commitment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66042/new/ https://reviews.llvm.org/D66042 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
