[analyzer] Restructuring the interface of the RetainCountChecker family

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

[analyzer] Restructuring the interface of the RetainCountChecker family

Kristóf Umann via cfe-dev
Hi!

This family of checkers is not under my authority, nor am I that knowledgeable about them, but even as a non-user, I find its interface confusing. While this doesn't affect me much, it is also the greatest sinner of how modeling/diagnostic checkers should be structured according to our previous discussions.

I'd be happy to fix it (in fact, I'd prefer to, just to gain a better understanding of how the new checker system should look like), but I obviously can't make decisions on how it should look like -- could you help me please?

Here is the problem:
* OSObjectRetainCountChecker and RetainCountChecker are subcheckers of RetainCountBase, yet they seem to fine-tune how the modeling should be done, rather then what diagnostics should be emitted. Shouldn't we turn them into checker options of RetainCountBase instead?

* OSObjectRetainCountChecker and the checker option osx.cocoa.RetainCount:CheckOSObject have a super weird interaction -- they are supposed to do the same thing (optionally enable some modeling RetainCount does), and exist purely for backward compatibility reasons, but in a way that I personally find impossible to understand.

The option was added by George, and was tied to osx.cocoa.RetainCount rather then osx.OSObjectRetainCountChecker, yet the option is unused unless osx.OSObjectRetainCountChecker itself is enabled, making it the only checker option ever to have 3 stances: enabled, disabled, and unspecified, the only remaining option not retrievable with debug.ConfigDumper, and is also the single reason why we can't make AnalyzerOptions' config table private.


Would it be possible to just simply make this a *regular* option that belongs to osx.RetainCountBase? Are there any users relying on this behavior?

* RefCountBug::RefCountBugType is an enum for various types of retain count related errors. Shouldn't we turn these into subcheckers?

Mind that we can always do this in a backwards-compatible manner, and aside from the Schrödinger option, we can also preserve the original behavior.

Cheers!
Husi

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

Re: [analyzer] Restructuring the interface of the RetainCountChecker family

Kristóf Umann via cfe-dev


On Wed, 28 Aug 2019 at 05:12, Kristóf Umann <[hidden email]> wrote:
Hi!

This family of checkers is not under my authority, nor am I that knowledgeable about them, but even as a non-user, I find its interface confusing. While this doesn't affect me much, it is also the greatest sinner of how modeling/diagnostic checkers should be structured according to our previous discussions.

I'd be happy to fix it (in fact, I'd prefer to, just to gain a better understanding of how the new checker system should look like), but I obviously can't make decisions on how it should look like -- could you help me please?

Here is the problem:
* OSObjectRetainCountChecker and RetainCountChecker are subcheckers of RetainCountBase, yet they seem to fine-tune how the modeling should be done, rather then what diagnostics should be emitted. Shouldn't we turn them into checker options of RetainCountBase instead?

* OSObjectRetainCountChecker and the checker option osx.cocoa.RetainCount:CheckOSObject have a super weird interaction -- they are supposed to do the same thing (optionally enable some modeling RetainCount does), and exist purely for backward compatibility reasons, but in a way that I personally find impossible to understand.

The option was added by George, and was tied to osx.cocoa.RetainCount rather then osx.OSObjectRetainCountChecker, yet the option is unused unless osx.OSObjectRetainCountChecker itself is enabled, making it the only checker option ever to have 3 stances: enabled, disabled, and unspecified, the only remaining option not retrievable with debug.ConfigDumper, and is also the single reason why we can't make AnalyzerOptions' config table private.


Would it be possible to just simply make this a *regular* option that belongs to osx.RetainCountBase? Are there any users relying on this behavior?

Another possible solutions:
* Supply the shouldRegister* functions with more data (AnalyzerOptions in particular), don't even enable the checker if the option is false.
* When we're parsing the checker options in CompilerInvocation.cpp, manually turn this into an -analyzer-disable-checker.
 
* RefCountBug::RefCountBugType is an enum for various types of retain count related errors. Shouldn't we turn these into subcheckers?

Mind that we can always do this in a backwards-compatible manner, and aside from the Schrödinger option, we can also preserve the original behavior.

Cheers!
Husi

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

Re: [analyzer] Restructuring the interface of the RetainCountChecker family

Kristóf Umann via cfe-dev
Do i understand correctly that the only difference between this checker and VirtualCallChecker is that the option is on the wrong checker?

I think we should be able to eventually remove the option. Probably we can even do it right now, but i'll double check.

I also believe that we don't have a strong opinion on what exactly does disabling the checker do. We only care about being able to silence both parts of the checker independently. George originally implemented it by disabling modeling (which automatically guarantees that reports aren't emitted), but there should be no harm in rewriting it to silence the reports instead.


On 8/27/19 8:19 PM, Kristóf Umann wrote:

On Wed, 28 Aug 2019 at 05:12, Kristóf Umann <[hidden email]> wrote:
Hi!

This family of checkers is not under my authority, nor am I that knowledgeable about them, but even as a non-user, I find its interface confusing. While this doesn't affect me much, it is also the greatest sinner of how modeling/diagnostic checkers should be structured according to our previous discussions.

I'd be happy to fix it (in fact, I'd prefer to, just to gain a better understanding of how the new checker system should look like), but I obviously can't make decisions on how it should look like -- could you help me please?

Here is the problem:
* OSObjectRetainCountChecker and RetainCountChecker are subcheckers of RetainCountBase, yet they seem to fine-tune how the modeling should be done, rather then what diagnostics should be emitted. Shouldn't we turn them into checker options of RetainCountBase instead?

* OSObjectRetainCountChecker and the checker option osx.cocoa.RetainCount:CheckOSObject have a super weird interaction -- they are supposed to do the same thing (optionally enable some modeling RetainCount does), and exist purely for backward compatibility reasons, but in a way that I personally find impossible to understand.

The option was added by George, and was tied to osx.cocoa.RetainCount rather then osx.OSObjectRetainCountChecker, yet the option is unused unless osx.OSObjectRetainCountChecker itself is enabled, making it the only checker option ever to have 3 stances: enabled, disabled, and unspecified, the only remaining option not retrievable with debug.ConfigDumper, and is also the single reason why we can't make AnalyzerOptions' config table private.


Would it be possible to just simply make this a *regular* option that belongs to osx.RetainCountBase? Are there any users relying on this behavior?

Another possible solutions:
* Supply the shouldRegister* functions with more data (AnalyzerOptions in particular), don't even enable the checker if the option is false.
* When we're parsing the checker options in CompilerInvocation.cpp, manually turn this into an -analyzer-disable-checker.
 
* RefCountBug::RefCountBugType is an enum for various types of retain count related errors. Shouldn't we turn these into subcheckers?

Mind that we can always do this in a backwards-compatible manner, and aside from the Schrödinger option, we can also preserve the original behavior.

Cheers!
Husi


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