[analyzer] Refactoring AnalyzerOptions

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[analyzer] Refactoring AnalyzerOptions

David Blaikie via cfe-dev
Hi!

We did have a thread about this but with a very misleading title, so here's a link to that, and I'll get into this mail:

AnalyzerOptions shouldn't be mutable once fully initialized (which should be achieveble by the time the actual analysis begins), and the greatest enemies of this idea are checker options, because
* we can either forget about collecting all checker options and possibly diagnose them, and let checkers use AnalyzerOptions as a sort of set of user supplied options. This is the current state of things, and should my non-checker option refactoring effort go through, AnalyzerOptions can be made const straight away.
* we could supply a mutable AnalyzerOptions object to the checkers when registering, let them register and evaluate their options, and make it immutable for the rest of the analysis.

I'm highly in favour of the second option, but it's a non-trivial issue, mostly because of external plugins, which is why I'm looking for some feedback on my ideas.

In order to register (and, more importantly, initialize) checkers, one needs to have access to a CheckerManager object, which isn't trivial to create, which makes it impossible to implement a help flag (like -analyzer-checker-help or the proposed -analyzer-config-help). I'm proposing two possible solutions.

1. Extract everything that isn't easily accessible to a new CheckerManagerData class, make CheckerManager only responsible for interacting (but not registering) checkers. I've got a fork on which I managed to get this working, but I disliked this approach, and went on to find a better solution.

2. Force checkers to properly register their options in a new, registerOptionsFor##CHECKERNAME function, which would take AnalyzerOptions as a parameter, alongside register##CHECKERNAME. This would add one more complication to the already very-not-trivial registering process, but could also be autogenerated using tblgen.

It's clear to me that the second option is superior to the second, but going forward with either is a lot of work, so I'm looking for feedback.

Thanks to everyone who already took the time to help me with this effort!

Cheers,
Kristóf

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Refactoring AnalyzerOptions

David Blaikie via cfe-dev
Too bad I cant edit mails.

Where I talked about extracting all isn't easily accessable fields and related methods to CheckerManagerData, I actually meant *easily* accessible (since some checkers actually need to access LangOptions, as well as AnalyerOptions, both avaible when other similar -help options are handled).

On 26 Oct 2018 20:58, "Kristóf Umann" <[hidden email]> wrote:
Hi!

We did have a thread about this but with a very misleading title, so here's a link to that, and I'll get into this mail:

AnalyzerOptions shouldn't be mutable once fully initialized (which should be achieveble by the time the actual analysis begins), and the greatest enemies of this idea are checker options, because
* we can either forget about collecting all checker options and possibly diagnose them, and let checkers use AnalyzerOptions as a sort of set of user supplied options. This is the current state of things, and should my non-checker option refactoring effort go through, AnalyzerOptions can be made const straight away.
* we could supply a mutable AnalyzerOptions object to the checkers when registering, let them register and evaluate their options, and make it immutable for the rest of the analysis.

I'm highly in favour of the second option, but it's a non-trivial issue, mostly because of external plugins, which is why I'm looking for some feedback on my ideas.

In order to register (and, more importantly, initialize) checkers, one needs to have access to a CheckerManager object, which isn't trivial to create, which makes it impossible to implement a help flag (like -analyzer-checker-help or the proposed -analyzer-config-help). I'm proposing two possible solutions.

1. Extract everything that isn't easily accessible to a new CheckerManagerData class, make CheckerManager only responsible for interacting (but not registering) checkers. I've got a fork on which I managed to get this working, but I disliked this approach, and went on to find a better solution.

2. Force checkers to properly register their options in a new, registerOptionsFor##CHECKERNAME function, which would take AnalyzerOptions as a parameter, alongside register##CHECKERNAME. This would add one more complication to the already very-not-trivial registering process, but could also be autogenerated using tblgen.

It's clear to me that the second option is superior to the second, but going forward with either is a lot of work, so I'm looking for feedback.

Thanks to everyone who already took the time to help me with this effort!

Cheers,
Kristóf

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Refactoring AnalyzerOptions

David Blaikie via cfe-dev
What do you think about refactoring Checkers.td into a .def-file and
listing checker options there? Eg.,

     CHECKER(Malloc, core,
         "Check for memory leaks, double free, and use-after-free
problems.")
     OPTION(Malloc, Optimistic, bool, false,
         "A useless option that needs to be removed.")

     CHECKER(PthreadLock, alpha.unix,
         "Simple lock -> unlock checker"

We could also de-duplicate packages (though i don't think that's
necessary, as it's a matter of simple string prefix comparison):

     BEGIN_PACKAGE(unix, alpha)
         CHECKER(PthreadLock, "Simple lock -> unlock checker")
         CHECKER(...)
         OPTION(...)
     END_PACKAGE

=====

As an unrelated note, i've been dreaming for a while now about replacing
packages with hashtags for more flexibility. Eg.,

     CHECKER(PthreadLock, "Simple lock -> unlock checker", "#alpha
#posix #pathsensitive #threading")

Of course, we can always keep packages around for backwards compatibility.


On 10/26/18 3:17 PM, Kristóf Umann wrote:

> Too bad I cant edit mails.
>
> Where I talked about extracting all isn't easily accessable fields and
> related methods to CheckerManagerData, I actually meant *easily*
> accessible (since some checkers actually need to access LangOptions,
> as well as AnalyerOptions, both avaible when other similar -help
> options are handled).
>
> On 26 Oct 2018 20:58, "Kristóf Umann" <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi!
>
>     We did have a thread about this but with a very misleading title,
>     so here's a link to that, and I'll get into this mail:
>     http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html
>     <http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html>
>
>     AnalyzerOptions shouldn't be mutable once fully initialized (which
>     should be achieveble by the time the actual analysis begins), and
>     the greatest enemies of this idea are checker options, because
>     * we can either forget about collecting all checker options and
>     possibly diagnose them, and let checkers use AnalyzerOptions as a
>     sort of set of user supplied options. This is the current state of
>     things, and should my non-checker option refactoring effort go
>     through, AnalyzerOptions can be made const straight away.
>     * we could supply a mutable AnalyzerOptions object to the checkers
>     when registering, let them register and evaluate their options,
>     and make it immutable for the rest of the analysis.
>
>     I'm highly in favour of the second option, but it's a non-trivial
>     issue, mostly because of external plugins, which is why I'm
>     looking for some feedback on my ideas.
>
>     In order to register (and, more importantly, initialize) checkers,
>     one needs to have access to a CheckerManager object, which isn't
>     trivial to create, which makes it impossible to implement a help
>     flag (like -analyzer-checker-help or the proposed
>     -analyzer-config-help). I'm proposing two possible solutions.
>
>     1. Extract everything that isn't easily accessible to a new
>     CheckerManagerData class, make CheckerManager only responsible for
>     interacting (but not registering) checkers. I've got a fork on
>     which I managed to get this working, but I disliked this approach,
>     and went on to find a better solution.
>
>     2. Force checkers to properly register their options in a new,
>     registerOptionsFor##CHECKERNAME function, which would take
>     AnalyzerOptions as a parameter, alongside register##CHECKERNAME.
>     This would add one more complication to the already
>     very-not-trivial registering process, but could also be
>     autogenerated using tblgen.
>
>     It's clear to me that the second option is superior to the second,
>     but going forward with either is a lot of work, so I'm looking for
>     feedback.
>
>     Thanks to everyone who already took the time to help me with this
>     effort!
>
>     Cheers,
>     Kristóf
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Refactoring AnalyzerOptions

David Blaikie via cfe-dev
Actually, I already have a fork on which I managed to get this info into Checkers.td (and have come to the same conclusion about tblgen being difficult to manage).

Hashtags sound awesome, but I'll probably be burnt out with options by the time I finish the already planned changes.

On 29 Oct 2018 19:39, "Artem Dergachev" <[hidden email]> wrote:
What do you think about refactoring Checkers.td into a .def-file and listing checker options there? Eg.,

    CHECKER(Malloc, core,
        "Check for memory leaks, double free, and use-after-free problems.")
    OPTION(Malloc, Optimistic, bool, false,
        "A useless option that needs to be removed.")

    CHECKER(PthreadLock, alpha.unix,
        "Simple lock -> unlock checker"

We could also de-duplicate packages (though i don't think that's necessary, as it's a matter of simple string prefix comparison):

    BEGIN_PACKAGE(unix, alpha)
        CHECKER(PthreadLock, "Simple lock -> unlock checker")
        CHECKER(...)
        OPTION(...)
    END_PACKAGE

=====

As an unrelated note, i've been dreaming for a while now about replacing packages with hashtags for more flexibility. Eg.,

    CHECKER(PthreadLock, "Simple lock -> unlock checker", "#alpha #posix #pathsensitive #threading")

Of course, we can always keep packages around for backwards compatibility.



On 10/26/18 3:17 PM, Kristóf Umann wrote:
Too bad I cant edit mails.

Where I talked about extracting all isn't easily accessable fields and related methods to CheckerManagerData, I actually meant *easily* accessible (since some checkers actually need to access LangOptions, as well as AnalyerOptions, both avaible when other similar -help options are handled).

On 26 Oct 2018 20:58, "Kristóf Umann" <[hidden email] <mailto:[hidden email]>> wrote:

    Hi!

    We did have a thread about this but with a very misleading title,
    so here's a link to that, and I'll get into this mail:
    http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html
    <http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html>

    AnalyzerOptions shouldn't be mutable once fully initialized (which
    should be achieveble by the time the actual analysis begins), and
    the greatest enemies of this idea are checker options, because
    * we can either forget about collecting all checker options and
    possibly diagnose them, and let checkers use AnalyzerOptions as a
    sort of set of user supplied options. This is the current state of
    things, and should my non-checker option refactoring effort go
    through, AnalyzerOptions can be made const straight away.
    * we could supply a mutable AnalyzerOptions object to the checkers
    when registering, let them register and evaluate their options,
    and make it immutable for the rest of the analysis.

    I'm highly in favour of the second option, but it's a non-trivial
    issue, mostly because of external plugins, which is why I'm
    looking for some feedback on my ideas.

    In order to register (and, more importantly, initialize) checkers,
    one needs to have access to a CheckerManager object, which isn't
    trivial to create, which makes it impossible to implement a help
    flag (like -analyzer-checker-help or the proposed
    -analyzer-config-help). I'm proposing two possible solutions.

    1. Extract everything that isn't easily accessible to a new
    CheckerManagerData class, make CheckerManager only responsible for
    interacting (but not registering) checkers. I've got a fork on
    which I managed to get this working, but I disliked this approach,
    and went on to find a better solution.

    2. Force checkers to properly register their options in a new,
    registerOptionsFor##CHECKERNAME function, which would take
    AnalyzerOptions as a parameter, alongside register##CHECKERNAME.
    This would add one more complication to the already
    very-not-trivial registering process, but could also be
    autogenerated using tblgen.

    It's clear to me that the second option is superior to the second,
    but going forward with either is a lot of work, so I'm looking for
    feedback.

    Thanks to everyone who already took the time to help me with this
    effort!

    Cheers,
    Kristóf




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Refactoring AnalyzerOptions

David Blaikie via cfe-dev
The biggest complication is the existence of external plugins. What are your feelings on the second solution I proposed? Whether I use a .def file or a .td file makes little difference to me at the moment, that's something I can always change, if I have a lot of free time and more then 3-4 handfuls of hair.

Kristóf Umann <[hidden email]> ezt írta (időpont: 2018. okt. 29., H, 19:57):
Actually, I already have a fork on which I managed to get this info into Checkers.td (and have come to the same conclusion about tblgen being difficult to manage).

Hashtags sound awesome, but I'll probably be burnt out with options by the time I finish the already planned changes.

On 29 Oct 2018 19:39, "Artem Dergachev" <[hidden email]> wrote:
What do you think about refactoring Checkers.td into a .def-file and listing checker options there? Eg.,

    CHECKER(Malloc, core,
        "Check for memory leaks, double free, and use-after-free problems.")
    OPTION(Malloc, Optimistic, bool, false,
        "A useless option that needs to be removed.")

    CHECKER(PthreadLock, alpha.unix,
        "Simple lock -> unlock checker"

We could also de-duplicate packages (though i don't think that's necessary, as it's a matter of simple string prefix comparison):

    BEGIN_PACKAGE(unix, alpha)
        CHECKER(PthreadLock, "Simple lock -> unlock checker")
        CHECKER(...)
        OPTION(...)
    END_PACKAGE

=====

As an unrelated note, i've been dreaming for a while now about replacing packages with hashtags for more flexibility. Eg.,

    CHECKER(PthreadLock, "Simple lock -> unlock checker", "#alpha #posix #pathsensitive #threading")

Of course, we can always keep packages around for backwards compatibility.



On 10/26/18 3:17 PM, Kristóf Umann wrote:
Too bad I cant edit mails.

Where I talked about extracting all isn't easily accessable fields and related methods to CheckerManagerData, I actually meant *easily* accessible (since some checkers actually need to access LangOptions, as well as AnalyerOptions, both avaible when other similar -help options are handled).

On 26 Oct 2018 20:58, "Kristóf Umann" <[hidden email] <mailto:[hidden email]>> wrote:

    Hi!

    We did have a thread about this but with a very misleading title,
    so here's a link to that, and I'll get into this mail:
    http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html
    <http://lists.llvm.org/pipermail/cfe-dev/2018-October/059664.html>

    AnalyzerOptions shouldn't be mutable once fully initialized (which
    should be achieveble by the time the actual analysis begins), and
    the greatest enemies of this idea are checker options, because
    * we can either forget about collecting all checker options and
    possibly diagnose them, and let checkers use AnalyzerOptions as a
    sort of set of user supplied options. This is the current state of
    things, and should my non-checker option refactoring effort go
    through, AnalyzerOptions can be made const straight away.
    * we could supply a mutable AnalyzerOptions object to the checkers
    when registering, let them register and evaluate their options,
    and make it immutable for the rest of the analysis.

    I'm highly in favour of the second option, but it's a non-trivial
    issue, mostly because of external plugins, which is why I'm
    looking for some feedback on my ideas.

    In order to register (and, more importantly, initialize) checkers,
    one needs to have access to a CheckerManager object, which isn't
    trivial to create, which makes it impossible to implement a help
    flag (like -analyzer-checker-help or the proposed
    -analyzer-config-help). I'm proposing two possible solutions.

    1. Extract everything that isn't easily accessible to a new
    CheckerManagerData class, make CheckerManager only responsible for
    interacting (but not registering) checkers. I've got a fork on
    which I managed to get this working, but I disliked this approach,
    and went on to find a better solution.

    2. Force checkers to properly register their options in a new,
    registerOptionsFor##CHECKERNAME function, which would take
    AnalyzerOptions as a parameter, alongside register##CHECKERNAME.
    This would add one more complication to the already
    very-not-trivial registering process, but could also be
    autogenerated using tblgen.

    It's clear to me that the second option is superior to the second,
    but going forward with either is a lot of work, so I'm looking for
    feedback.

    Thanks to everyone who already took the time to help me with this
    effort!

    Cheers,
    Kristóf




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev