XCode 3.2 Static Analysis

Discussion in 'Mac Programming' started by Soulstorm, Sep 14, 2009.

  1. Soulstorm macrumors 68000

    Soulstorm

    Joined:
    Feb 1, 2005
    #1
    I have been experimenting with static analysis on my machine, and I have the impression that although it is a great piece of work, it doesn't always find the correct errors.

    For example, I have this code:

    Code:
    - (NSString *)generateSoundID
    {
    	NSString *result = nil;
    	NSFileManager *fm = [NSFileManager defaultManager];
    	
    	NSAutoreleasePool *pool = [[NSAutoreleasePool alloc]init];
    	
    	NSString *soundsDirectory = [[SFGlobals sharedSFGlobals]applicationSoundsFolder];
    	unsigned i = 0;
    	BOOL shouldContinueLooping = YES;
    	do {
    		NSString *nameToTest = [NSString stringWithFormat:@"%@%u.%@", SOUND_FILE_PREFIX, i, SOUND_FILE_SUFFIX];
    		NSString *locationToTest = [soundsDirectory stringByAppendingPathComponent:nameToTest];
    		NSLog(@"now checking: %@", locationToTest);
    		if (![fm fileExistsAtPath:locationToTest]){
    			result = [nameToTest retain];
    			shouldContinueLooping = NO;
    		}
    		i++;
    	} while (shouldContinueLooping == YES);
    	
    	[pool release];
    	return result;
    }
    
    The code searches for an available name of type "soundXX.wav" (where XX is a 2 digit number) on the file system, and when it finds it, it returns it. Static analyzer rings the bell on this one, and complains that there is a possible memory leak on "nameToTest".

    Maybe this is my own fault, but I fail to see the memory leak here. I allocated nameToTest, retained it, released it (since I release the Autorelease pool) and returned it from my function. Shouldn't it be autoreleased afterwards by cocoa since it is an autoreleased object?

    I would really like someone to explain this to me, as this may save me a lot of trouble in the future.
     
  2. Catfish_Man macrumors 68030

    Catfish_Man

    Joined:
    Sep 13, 2001
    Location:
    Portland, OR
    #2
    So, nameToTest initially has a retain count of 0 (1 + -1 from the implicit autorelease). Then you retain it, leaving it at a net retain count of +1. Then you return it, still at +1. That seems like a memory leak to me.
     
  3. Ritsuka macrumors 6502a

    Joined:
    Sep 3, 2006
    #3
    Yes it's autoreleased, but you retained it, so the ref count won't be 0 and nameToTest won't be dealloced. Autorelease just decrease the ref count by 1. If the ref count is more then 1 you need another release.
     
  4. whooleytoo macrumors 603

    whooleytoo

    Joined:
    Aug 2, 2002
    Location:
    Cork, Ireland.
    #4
    Yup, I think "return [result autorelease];" should do it.
     
  5. Soulstorm thread starter macrumors 68000

    Soulstorm

    Joined:
    Feb 1, 2005
    #5
    Oh, so when an object is autoreleased, it must still get an -autorelease message in order to be released in the next autorelease pool release?
     
  6. Catfish_Man macrumors 68030

    Catfish_Man

    Joined:
    Sep 13, 2001
    Location:
    Portland, OR
    #6
    No, that's not what I said. The problem is that you have +2 (allocation, and retain) and only -1 (autorelease). You need another release or autorelease to balance.
     
  7. gnasher729 macrumors P6

    gnasher729

    Joined:
    Nov 25, 2005
    #7
    I suppose that is the intent of the function, to return an NSString* with a retain count of one (the result of the retain method is stored in result and returned). The method should probably have a different name, like "createSoundID" or "copySoundID", because it behaves like a "create" or "copy" method. Would be interesting to see if changing the name of the method would make a difference.
     
  8. Catfish_Man macrumors 68030

    Catfish_Man

    Joined:
    Sep 13, 2001
    Location:
    Portland, OR
    #8
    It would; clang's static analyzer understands Cocoa naming conventions.
     
  9. whooleytoo macrumors 603

    whooleytoo

    Joined:
    Aug 2, 2002
    Location:
    Cork, Ireland.
    #9
    Interesting, sounds like it's quite powerful then.

    I assume so that it's clever enough to recognise that having the line retaining nameToTest within a loop is fine (since that's only executed once)?
     
  10. Catfish_Man macrumors 68030

    Catfish_Man

    Joined:
    Sep 13, 2001
    Location:
    Portland, OR
    #10
    Yup. It can't do cross-function analysis (the naming convention awareness is sort of a workaround for this... sort of) at this point, but within a function it does do data-dependent analysis, so it can tell that that path will exit the loop.
     
  11. Soulstorm thread starter macrumors 68000

    Soulstorm

    Joined:
    Feb 1, 2005
    #11
    Thanks a lot, guys, I placed a -autorelease message in the end of the function, and static analysis stopped complaining.

    Actually, static analysis seems like a very powerful tool. It's one of the reasons I made the switch to Leopard almost right away, because I had the intention of waiting for Leopard 10.6.2 to make the upgrade.
     
  12. gnasher729 macrumors P6

    gnasher729

    Joined:
    Nov 25, 2005
    #12
    I did some experiments: It assumes that functions starting with Create or Copy return an object that is retained and should be released, functions starting with Get return an object that isn't yours and shouldn't be released, and anything else is assumed to have unknown behaviour, so the analyser can't warn you. This means you should rename your functions accordingly.

    I also checked that the analyser doesn't look into static inline functions, even if the compiler does. So you can't write a function that will release something. For example a replacement for CFRelease that doesn't crash when the object released is NULL must be written as a macro.

    As usual, the idea is not to fix compiler warnings, but to make the code correct. Did you want that function to return an autoreleased string or a retained string? If you wanted it to create a retained string then change the function name, not the code.
     
  13. kainjow Moderator emeritus

    kainjow

    Joined:
    Jun 15, 2000
    #13
    This does work nicely for functions, but not so much for methods. If you have a method that returns a retained object, Clang will only ignore it if it starts with copy, but not create, which as a result causes a warning with the createViewController method in Apple's QCPlugIn class. You *could* use the NS_RETURNS_RETAINED macro but I wish the "create" suffix was supported in methods.

    I also found that if you use underscores instead of camel case for init methods, it doesn't understand that. For example init_with_something vs initWithSomething.
     

Share This Page