NagyDonat wrote: > First, let's align on why we currently have the concept of "modeling" and > "reporting" checkers. I think you are probably already aware of this, but > let's clarify this. > > It's a great property if the exploded graph remains sort of the same no > matter what checkers are enabled. It makes it easier to reason about it and > when and why we have sink nodes, affecting what paths explored etc. > In the past (and also still today AFAIK in some cases) checkers checked some > property and reported a fatal error node. So there was a sink node if the > checker was enabled, but as soon as it was disabled, the sink node was > missing, discovering different issues in other checkers down the line in the > exploded graph. > To solve this, was the effort of splitting the checkers into modeling and > reporting and emit the sink node regardless. This way only the user-facing > diagnostics would be different, but the underlying egraph would remain the > same regardless if we enable or disable a checker. (This statement isn't > quite true, but you get the principle).
I'm aware of this goal that a checker should emit sinks even if the reports are suppressed, and I agree that this would be useful as the effect of disabling a "well-behaving", trustworthy checker. However, I would guess that checkers are usually disabled when they mismodel the analyzed code and produce crazy garbage (or e.g. it's an `optin` checker checking that enforces a design rule which is intentionally not followed), and in those situations I think the user needs a tool that is more drastic than just replacing the reports with sinks. > So the problem was that we had to hack together multiple "checker frontends" > and conditionally report a fatal bug or a sink node on an error. >In theory, a "checker frontend" should only ever depend on a set of "checker >backends" aka. "modeling checkers". I wish we had some better way of doing this. I agree and I hope that the checker family framework will provide a way towards implementing this. > I think we are now better than ever to achieve this that we tie together the > BugType and a checker frontend that we can query. Actually I introduced the class `CheckerFrontendWithBugType` only as an ad-hoc, invisible helper that I intend to use to write slightly shorter code in situations where it applies. In more complex checker families where a single frontend can have multiple bug types, I intend to define multiple `BugType`s that each take the same single `CheckerFrontend` as their constructor argument. (But if the transition to the new framework is complete, we can eliminate the `BugType` constructor that takes a `CheckerName`, so each `BugType` will hold a pointer to a `CheckerFrontend`.) --------- > This makes me question why we want to improve the ergonomics of defining and > organizing checkers? Is this the most important pain point we want to focus > on right now? No, this is not the most important pain point -- this was just annoying me personally, so I tried to fix it quickly and got carried away... I wouldn't say that this is completely useless (we will be able to build new functionality onto the new framework in a way that wouldn't be possible with the old swamp of hacks), but this isn't an urgent project. > To me btw, this checker families and what we had before with raw bool flags > are the same thing. It's just now a lot less ugly. I agree, this is after all a non-functional change -- I'm proposing it because I got fed up with ugly code. > I'll be honest with you, that right now I can't see if the currently more > appealing direction with option 2 would be really the better path to take (or > something along that line). So I think it's a risky direction to have so many > changes on the flight in the subject before consolidating a statusquo first > with checker families. After that we may have a better view what's next, if > we still believe that the ergonomics is the best place to invest. I agree here as well. When I started to describe the "Default" and "Alternative" plans I initially felt that we need to pick between them right now; but as I thought about it more, it seems that they are not opposed and in fact the best path for eventually implementing the "Alternative" plan (which is indeed more appealing intuitively) would be first implementing the "Default" plan (which cleans up lots of hacks, but leaves the modeling checkers unchanged) and then later introducing the "Alternative" plan if we feel that we need it. Based on this I'll quickly incorporate your patches and the "use the internal classname-like checker name" logic onto the branch, and then I think we can merge it as a good enough implementation of the Default plan framework. https://github.com/llvm/llvm-project/pull/139256 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
