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

[Kristóf Umann via cfe-dev]

A polite ping, did you have time to take a look at this?

On Tue, 7 May 2019, 01:11 Kristóf Umann, <dkszelethus at gmail.com> wrote:

> 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/20190508/68df30aa/attachment.html>