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

[Kristóf Umann via cfe-dev]

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/20190326/64aeb655/attachment.html>