[cfe-dev] [analyzer] Reimplementing checker options -- looking for reviews!

[Kristóf Umann via cfe-dev]

A polite ping :).

On Thu, 11 Apr 2019 at 14:03, Kristóf Umann <dkszelethus at gmail.com> wrote:

> Ping. We have had some discussions about this at the EuroLLVM conference,
> but it would be great to have this on cfe-dev as well.
>
> On Tue, 26 Mar 2019 at 19:11, Kristóf Umann <dkszelethus at gmail.com> wrote:
>
>> Just a polite ping, Devin, could you please weight in on the responses?
>>
>> On Wed, 20 Mar 2019 at 12:23, Kristóf Umann <dkszelethus at gmail.com>
>> wrote:
>>
>>> Hi Devin!
>>>
>>> Gábor Horváth <xazax.hun at gmail.com> ezt írta (időpont: 2019. márc. 18.,
>>> H, 11:08):
>>>
>>>> Hi Devin!
>>>>
>>>> On Mon, 18 Mar 2019 at 05:04, Devin Coughlin <dcoughlin at apple.com>
>>>> wrote:
>>>>
>>>>> Hi Kristóf,
>>>>>
>>>>> It’s great to see this area getting cleaned up!
>>>>>
>>>>> On Mar 16, 2019, at 1:32 PM, Kristóf Umann <dkszelethus at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Hi!
>>>>>
>>>>> TL;DR: The API for registering checker options changes once several
>>>>> changes that are up for review now land. This affects out-of-tree and
>>>>> checker plugin developers. Now's the time to participate in the discussion!
>>>>>
>>>>>
>>>>> As a reminder, LLVM encourages an incremental development process
>>>>> where for significant changes it is important to discuss the change and
>>>>> gather consensus. This can avoiding wasted effort implement an approach
>>>>> that doesn’t have consensus. See <
>>>>> https://www.llvm.org/docs/DeveloperPolicy.html#incremental-changes>.
>>>>> It also makes the patches easier to review.
>>>>>
>>>>
>>> I realized only later that I've forgotten to add you as reviewer to my
>>> patches. Sorry about that! I probably should've put more effort into
>>> reaching out to a wider audience, although discussions in between Artem,
>>> George, Gábor and me have been going on for months here on cfe-dev and
>>> phabricator. I did not, however, emphasise enough the improtance of some of
>>> these patches. For the sake of completeness, I've included a couple links
>>> to the earlier discussions we had:
>>>
>>> http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html (start
>>> of a thread on changes to AnalyzerOptions)
>>> http://lists.llvm.org/pipermail/cfe-dev/2018-October/059962.html (discussion
>>> on whether, and if so, how plugins should be supported regarding checker
>>> options)
>>> http://lists.llvm.org/pipermail/cfe-dev/2018-December/060519.html (notice
>>> on landing API changes to non-checker config options and checker dependency
>>> handling)
>>> https://reviews.llvm.org/D54438 (checker dependency patch with * a lot
>>> * discussion oh things should go, also discussing in detail the issue of
>>> plugins)
>>>
>>> Going forward, I'll be sure to add you as reviewer on my patches, and
>>> send out [RFC] tagged mails before working on API touching changes.
>>>
>>>
>>>>
>>>>> In (not overwhelmingly) more detail:
>>>>>
>>>>> After many-many months of hard work, I'm very confident in the current
>>>>> state of the project. Allow me to elaborate.
>>>>>
>>>>> In the recent months, the frontend of the analyzer, specifically how
>>>>> command line options are handled, have changed a lot. Right now, compared
>>>>> to how things used to be,
>>>>>
>>>>> * We can list non-checker analyzer configurations
>>>>> * We can verify user input for non-checker analyzer configurations
>>>>> * The interface of AnalyzerOptions changed dramatically in order not
>>>>> to allow this problem to arise again
>>>>> * debug.ConfigDumper contains all non-checker analyzer configurations,
>>>>> making it an actually usable debug tool
>>>>>  * An almost decade-old issue, the checker naming bug was resolved by
>>>>> reimplementing checker dependencies, and the related interface was also
>>>>> changed to guard against this happening again
>>>>>
>>>>> You probably noticed that I put a very strong emphasis on *non-checker
>>>>> analyzer configurations* -- since checkers can be loaded run-time via
>>>>> plugins, doing the same for them is a far more difficult task.
>>>>>
>>>>>
>>>>> I’m not sure how much emphasis we should put on checkers loaded via
>>>>> plugins. The analyzer really doesn’t support a plugin model and probably
>>>>> never will, given the difficulty of maintaining a stable C++ ABI when the
>>>>> components we depend on (such as the AST) don’t expose one in C++.
>>>>>
>>>>
>>>> I am kind of surprised that C++ ABI is a concern. While I do admint
>>>> that it is kind of a fragile solution LLVM does support loading passes
>>>> dynamically that are using the C++ API. What is the difference between LLVM
>>>> and CSA in this regard?
>>>>
>>>
>>> We use the plugin feature extensively in Ericsson to register internal
>>> checkers that are verifying internal product specific design rules which
>>> are of no interest of the community. They are maintained by developers of
>>> the analyzer who are most of the time very aware of the changes in between
>>> versions, so it's of utmost importance to us that the changes related to
>>> dependency and command line option handling is working with plugins as well.
>>>
>>> Would you be more comfortable with these changes if they didn't touch
>>> the examples/ folder? Since some extra code is required for plugins to
>>> work, I would argue on the side of talking about them in *developer* docs,
>>> even if we add scary notes around it.
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> I've uploaded several patches that finally fixes this for good.
>>>>> Although these have up for more than a month now, the code changed quite a
>>>>> bit, and after several in-office discussions, vigorous testing and
>>>>> refactoring, I'm very confident that everything is in it's final place.
>>>>> Please take a look if these changes affect you!
>>>>>
>>>>> The most important of these patches is https://reviews.llvm.org/D57855.
>>>>> Please visit the "Stack" as well, the list of patches that depend on this
>>>>> and those this depends on. While 11 patches might seem a little scary at
>>>>> first, I've put a lot of effort into making as small as possible, in order
>>>>> to easy on reviewing. Note that the one patch I highlighted here is quite
>>>>> large however.
>>>>>
>>>>>
>>>>> I commented on a bunch of these.
>>>>>
>>>>
>>>>> It is really, really great to see the improvements in testability and
>>>>> specification here. As I noted in some of the patches, I do have some
>>>>> serious concerns about the changes to the user model and command-line flags
>>>>> — but I don’t want those to get in the way of the general goodness of many
>>>>> of the improvements here.
>>>>>
>>>>
>>>> Clang does have a history of having flags that are exclusively for
>>>> developers -Weverything is being one example. I think if those flags make
>>>> developing clang more convenient it might be worth to have them while
>>>> making it clear in the documentation and the output that this is not
>>>> intended to be used by end user. What do you think, would such notices help?
>>>>
>>>
>>> Shooting in the dark here, so please feel free to correct me, but isn't
>>> everything behind -cc1 a "power user only" thing? Since we already list
>>> debug and alpha checkers for the flag -analyzer-checker-help (and recently,
>>> -analyzer-config-help),
>>>
>>> I have had thoughts about exposing some of these flags however through
>>> driver flags, and narrowing these options down to the few that aren't
>>> developer-only features (like expand-macros, or
>>> alpha.cplusplus.UninitializedObject:Pedantic). Also, explicitly noting each
>>> flag whether they are developer-only or not sounds great too.
>>>
>>>
>>>>
>>>> Regards,
>>>> Gábor
>>>>
>>>>
>>>>> It would probably be a good idea to tease apart the patches that
>>>>> change the user model from those that improve analyzer infrastructure.
>>>>> Let's get the infrastructure ones landed!
>>>>>
>>>>
>>> I'm a little unsure about this one, could you specify? I would believe
>>> that the patches are already structured that way.
>>>
>>>
>>>> Devin
>>>>>
>>>>
>>> Thank you so much for the encouraging words on the effort, as well as
>>> the feedback on my patches! I'm very excited to see this project finally
>>> landing.
>>>
>>> Cheers,
>>> Kristóf
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190507/f478d7fb/attachment.html>