NSAutoreleasePool checker

Clang Frontend
1

Hi Everyone,

Currently the NSAutoreleasePool checker in the static analyser is only enabled in GC mode. Does anyone object if I enable it unconditionally? Sending -drain instead of -release to autorelease pools works in both GC and non-GC mode, while sending -release is a no-op in GC mode, so it's good style to use -drain always (recent Apple examples use -drain even in non-GC mode).

David

2

Hi David,

The point of this checker is to find bugs. Enabling all the time is likely to produce a significant amount of noise on real world code while providing no real value.

That said, I think there should be a way to opt into the check even if you are in non-GC, but I don't think that turning it on all the time is the right answer. I'm very skeptical that's what most people would want.

Ted

3

Hi Ted,

The problem is that the GC setting is dependent on the current compilation mode, not on the deployment target. If you are writing a framework that may be compiled in GC or non-GC mode, then you may well be building it and running the analyser in non-GC mode (e.g. for targeting the iPhone), but it would be helpful for the analyser to warn you that you're using the autorelease pool in a non-recommended way. Someone who grabs your framework for use on another platform may compile it with -fobjc-gc, but not run the analyser, so no one will ever see the message.

I've just fixed 74 instances of this in the GNUstep code. Most of the people working on the project are building in non-GC mode, so if they run the analyser, they won't see this warning, but it is something that should be fixed for people who do then build the code with GC enabled.

David

4

Currently the NSAutoreleasePool checker in the static analyser is only
enabled in GC mode.

Which is kinda weird, since, as Apple's docs say: "In a garbage-
collected environment, there is no need for autorelease pools."

Maybe the checker should warn that using pools is useless? :slight_smile:
Presumably, the assumption is that the code in question is in fact dual mode.

Does anyone object if I enable it unconditionally?
Sending -drain instead of -release to autorelease pools works in both GC
and non-GC mode, while sending -release is a no-op in GC mode, so it's
good style to use -drain always (recent Apple examples use -drain even
in non-GC mode).

And drain works back to 10.4 even. Personally, I'd like to see this change.

5

Hi Ted,

The problem is that the GC setting is dependent on the current compilation mode, not on the deployment target. If you are writing a framework that may be compiled in GC or non-GC mode, then you may well be building it and running the analyser in non-GC mode (e.g. for targeting the iPhone), but it would be helpful for the analyser to warn you that you're using the autorelease pool in a non-recommended way. Someone who grabs your framework for use on another platform may compile it with -fobjc-gc, but not run the analyser, so no one will ever see the message.

Hi David,

In my opinion, the analysis results should depend on the current compilation mode. We have a variety of checkers that take into account the current target triple (and other settings) when making decisions.

For example, consider the ObjC memory management checker. Currently it does a dual analysis if one passes -fobjc-gc: one for manual reference counting, the other for GC. Whether or not the code runs in GC strongly impacts the kind of analysis results we get from that checker. For those users not using GC, they would be extremely annoyed if we reported GC-specific bugs (or vis versa) for a mode that there code never runs in. Also consider the impact on the target architecture on the sizes of primitive types; these can impact static analysis results in subtle ways, and also cause some checkers to fire in some cases but not others (e.g., 64 versus 32 bit).

I've just fixed 74 instances of this in the GNUstep code. Most of the people working on the project are building in non-GC mode, so if they run the analyser, they won't see this warning, but it is something that should be fixed for people who do then build the code with GC enabled.

Honestly, a framework developer who cares about deploying in multiple settings should be running the analyzer for each of those cases. The argument is really no different than for testing a framework (one should be testing for each of the deployment settings). While this ostensibly looks like a case where we can just check all the time, I just don't think (a) this would have value for people who don't care about GC and (b) really sidesteps the general picture that some checks will inherently have different behavior depending on the compilation target and mode.

In the case of GNUstep, if the majority of the developers aren't compiling for GC, then this sounds like something that could be addressed by integration testing. For example, a buildbot could be running the analyzer in GC mode, and reporting new results. As callous as that sounds, I don't think this is unreasonable. For the LLVM project, it is not possible for developers to test on every host platform (e.g, Linux, Mac, FreeBSD, etc) that we care about. For GNUStep, deployment to different memory models sounds like a worthy thing to test with automatic testing, both with and without static analysis.

Of course, you are free to make whatever changes you want to Clang for your own setup (which I imagine you have), but I don't think this is the right decision for mainline Clang.

Cheers,
Ted

6

I think it is fine to worth considering this change because it “makes sense for all users.” If this is in the recommended Cocoa API practices, for example, I’d be more than happy to make this change. If this makes sense for (say) all GNUStep users, and we can determine that we are using GNUStep, then I’d be fine with turning it on in that case as well because that would make sense for that platform (as decided by those project owners).

7

This has been the recommended practice for Cocoa since 10.5. -drain will trigger a collection in GC mode and is equivalent to -release in non-GC mode. Calling -release on an autorelease pool is, as I said in my first mail, bad style in any mode.

David

8

Is there a

Does anyone object if I enable it unconditionally?

Sending -drain instead of -release to autorelease pools works in both GC

and non-GC mode, while sending -release is a no-op in GC mode, so it’s

good style to use -drain always (recent Apple examples use -drain even

in non-GC mode).

And drain works back to 10.4 even. Personally, I’d like to see this change.

I think it is fine to worth considering this change because it “makes sense for all users.” If this is in the recommended Cocoa API practices, for example, I’d be more than happy to make this change. If this makes sense for (say) all GNUStep users, and we can determine that we are using GNUStep, then I’d be fine with turning it on in that case as well because that would make sense for that platform (as decided by those project owners).

This has been the recommended practice for Cocoa since 10.5.

If you can find a documentation reference that clearly states that, then I’m happy to make the switch. All I see is:

Garbage CollectionIn a garbage-collected environment, there is no need for autorelease pools. You may, however, write a framework that is designed to work in both a garbage-collected and reference-counted environment. In this case, you can use autorelease pools to hint to the collector that collection may be appropriate. In a garbage-collected environment, sending a [drain](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/drain) message to a pool triggers garbage collection if necessary; [release](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/release), however, is a no-op. In a reference-counted environment, [drain](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/drain) has the same effect as [release](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/release). Typically, therefore, you should use [drain](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/drain)instead of [release](http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/occ/instm/NSAutoreleasePool/release).

which is not the same thing.