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

[Kristóf Umann via cfe-dev]

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/20190411/66faadb0/attachment.html>