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

Tue May 7 15:26:23 PDT 2019

I just realized that my earlier mail wasnt discarded as a draft, but rather
sent, didnt mean to ping this that often, sorry :).

On Wed, 8 May 2019, 00:25 Kristóf Umann, <dkszelethus at gmail.com> wrote:

> 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/421f58aa/attachment.html>