This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] ObjCAutoreleaseWriteChecker: Support explicit autoreleasepools.
ClosedPublic

Authored by NoQ on Jun 3 2020, 2:54 AM.

Details

Summary

This ASTMatchers-based checker currently supports only a whitelist of block-enumeration methods which are known to internally clear an autorelease pool. Extend this checker to detect writes within the scope of explicit @autoreleasepool statements.

Patch by Paul Pelzl!
(who can't participate in the discussion for IRL reasons; i'll address comments)

Diff Detail

Event Timeline

NoQ created this revision.Jun 3 2020, 2:54 AM

Otherwise, LTGM

clang/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
134–165

This looks a bit messy to me, but not critical.

NoQ updated this revision to Diff 268133.Jun 3 2020, 4:17 AM

Break up the huge twine into an old-school stream.

Lowkey change bug category to a standard category. Technically this is indeed a retain count error so the category is appropriate and it shouldn't matter to the user that it is coming from a different checker. But i'm still a bit worried because RetainCountChecker is so iconic to its users that they may be surprised if a new syntactic checker suddenly shows up under this category. Maybe we should make a new category, something like "Memory error (ARC)".

NoQ marked an inline comment as done.Jun 3 2020, 4:17 AM
vsavchenko accepted this revision.Jun 3 2020, 5:05 AM

Awesome, thanks!

This revision is now accepted and ready to land.Jun 3 2020, 5:05 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 9:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for implementing this! For posterity, I wanted to note a couple cases that this checker doesn't catch.

  1. Under ARC, a more general case of assigning to an __autoreleasing variable. (Not sure why anyone would do this, but it's possible to write.)
@implementation MyClass
- (BOOL)myError:(NSError * __strong *)error
{
    NSError __autoreleasing *localError;

    @autoreleasepool {
        localError = [[NSError alloc] init];
    }

    if (error) {
        *error = localError;
        return YES;
    }
    return NO;
}
@end
  1. Under MRR, writing to an autoreleasing out parameter that outlives the autoreleasePool (similar to the issue that is now found under ARC):
@implementation MyClass
- (BOOL)myError:(NSError **)error
{
    @autoreleasepool {
        if (error) {
            *error = [[[NSError alloc] init] autorelease];
            return YES;
        }
    }
    return NO;
}
@end