[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
rjmccall added a comment. That's not an unreasonable idea, and we did consider it, and in fact it's still on the table as something we could do. However, it's a significantly more complex change, and we're not committed to doing it, and so we wanted to at least put this warning in place to help people avoid a relatively common bug. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D25844/new/ https://reviews.llvm.org/D25844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
Cy-4AH added a comment. Hello! What compiler do when call doStuff: NSString * __strong fillMe; NSString * __autoreleasing autorelesingFillMe = fillMe; doStuff(&fillMe); fillMe = autoreleasingFillMe; Why it's not making the same thing in doStuff body? Compiler generated code for yours example should be: void doStuff(NSString **fillMeIn) { __block NSString *fillMeInResult; __block BOOL fillMeInResultHasChanged = NO; [@[@"array"] enumerateObjectsUsingBlock: ^(id obj, NSUInteger idx, BOOL* stop) { *stop = YES; fillMeInResult = [@"wow" mutableCopy]; fillMeInResultHasChanged = YES; } ]; if (fillMeInResultHasChanged) { *fillMeIn = fillMeInResult; } } Instead we got this warning and had to do the same stuff in every implementation Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D25844/new/ https://reviews.llvm.org/D25844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
This revision was automatically updated to reflect the committed changes. Closed by commit rL285031: [Sema][ObjC] Warn about implicitly autoreleasing out-parameters captured (authored by ahatanak). Changed prior to commit: https://reviews.llvm.org/D25844?vs=75442&id=75650#toc Repository: rL LLVM https://reviews.llvm.org/D25844 Files: cfe/trunk/include/clang/Basic/DiagnosticGroups.td cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/lib/Sema/SemaExpr.cpp cfe/trunk/test/SemaObjC/arc.m Index: cfe/trunk/include/clang/Basic/DiagnosticGroups.td === --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td @@ -498,6 +498,7 @@ def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">; def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak", [ARCRepeatedUseOfWeakMaybe]>; +def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">; def ObjCBridge : DiagGroup<"bridge-cast">; def DeallocInCategory:DiagGroup<"dealloc-in-category">; Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td === --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -5077,6 +5077,16 @@ } // end "ARC and @properties" category +def warn_block_capture_autoreleasing : Warning< + "block captures an autoreleasing out-parameter, which may result in " + "use-after-free bugs">, + InGroup, DefaultIgnore; +def note_declare_parameter_autoreleasing : Note< + "declare the parameter __autoreleasing explicitly to suppress this warning">; +def note_declare_parameter_strong : Note< + "declare the parameter __strong or capture a __block __strong variable to " + "keep values alive across autorelease pools">; + def err_arc_atomic_ownership : Error< "cannot perform atomic operation on a pointer to type %0: type has " "non-trivial ownership">; Index: cfe/trunk/test/SemaObjC/arc.m === --- cfe/trunk/test/SemaObjC/arc.m +++ cfe/trunk/test/SemaObjC/arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s typedef unsigned long NSUInteger; typedef const void * CFTypeRef; @@ -808,3 +808,10 @@ TKAssertEqual(object, nil); TKAssertEqual(object, (id)nil); } + +void block_capture_autoreleasing(A * __autoreleasing *a, A **b) { // expected-note {{declare the parameter __autoreleasing explicitly to suppress this warning}} expected-note {{declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools}} + ^{ +(void)*a; +(void)*b; // expected-warning {{block captures an autoreleasing out-parameter, which may result in use-after-free bugs}} + }(); +} Index: cfe/trunk/lib/Sema/SemaExpr.cpp === --- cfe/trunk/lib/Sema/SemaExpr.cpp +++ cfe/trunk/lib/Sema/SemaExpr.cpp @@ -13502,6 +13502,23 @@ } return false; } + + // Warn about implicitly autoreleasing indirect parameters captured by blocks. + if (auto *PT = dyn_cast(CaptureType)) { +QualType PointeeTy = PT->getPointeeType(); +if (isa(PointeeTy.getCanonicalType()) && +PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing && +!isa(PointeeTy)) { + if (BuildAndDiagnose) { +SourceLocation VarLoc = Var->getLocation(); +S.Diag(Loc, diag::warn_block_capture_autoreleasing); +S.Diag(VarLoc, diag::note_declare_parameter_autoreleasing) << +FixItHint::CreateInsertion(VarLoc, "__autoreleasing"); +S.Diag(VarLoc, diag::note_declare_parameter_strong); + } +} + } + const bool HasBlocksAttr = Var->hasAttr(); if (HasBlocksAttr || CaptureType->isReferenceType() || (S.getLangOpts().OpenMP && S.IsOpenMPCapturedDecl(Var))) { Index: cfe/trunk/include/clang/Basic/DiagnosticGroups.td === --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td @@ -498,6 +498,7 @@ def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">; def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak", [ARCRepeatedUseOfWeakMaybe]>; +def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">; def ObjCBridge : DiagGroup<"bridge-cast">; def DeallocInCategory:DiagGroup<"dealloc-in-category">; Index: cfe/trunk/include/clang/Basic/Dia
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
rjmccall added a comment. Thanks. LGTM. https://reviews.llvm.org/D25844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
ahatanak updated this revision to Diff 75442. ahatanak marked an inline comment as done. ahatanak added a comment. Improve warning messages. https://reviews.llvm.org/D25844 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/SemaObjC/arc.m Index: test/SemaObjC/arc.m === --- test/SemaObjC/arc.m +++ test/SemaObjC/arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s typedef unsigned long NSUInteger; typedef const void * CFTypeRef; @@ -808,3 +808,10 @@ TKAssertEqual(object, nil); TKAssertEqual(object, (id)nil); } + +void block_capture_autoreleasing(A * __autoreleasing *a, A **b) { // expected-note {{declare the parameter __autoreleasing explicitly to suppress this warning}} expected-note {{declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools}} + ^{ +(void)*a; +(void)*b; // expected-warning {{block captures an autoreleasing out-parameter, which may result in use-after-free bugs}} + }(); +} Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -13473,6 +13473,23 @@ } return false; } + + // Warn about implicitly autoreleasing indirect parameters captured by blocks. + if (auto *PT = dyn_cast(CaptureType)) { +QualType PointeeTy = PT->getPointeeType(); +if (isa(PointeeTy.getCanonicalType()) && +PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing && +!isa(PointeeTy)) { + if (BuildAndDiagnose) { +SourceLocation VarLoc = Var->getLocation(); +S.Diag(Loc, diag::warn_block_capture_autoreleasing); +S.Diag(VarLoc, diag::note_declare_parameter_autoreleasing) << +FixItHint::CreateInsertion(VarLoc, "__autoreleasing");; +S.Diag(VarLoc, diag::note_declare_parameter_strong); + } +} + } + const bool HasBlocksAttr = Var->hasAttr(); if (HasBlocksAttr || CaptureType->isReferenceType() || (S.getLangOpts().OpenMP && S.IsOpenMPCapturedDecl(Var))) { Index: include/clang/Basic/DiagnosticSemaKinds.td === --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -5073,6 +5073,16 @@ } // end "ARC and @properties" category +def warn_block_capture_autoreleasing : Warning< + "block captures an autoreleasing out-parameter, which may result in " + "use-after-free bugs">, + InGroup, DefaultIgnore; +def note_declare_parameter_autoreleasing : Note< + "declare the parameter __autoreleasing explicitly to suppress this warning">; +def note_declare_parameter_strong : Note< + "declare the parameter __strong or capture a __block __strong variable to " + "keep values alive across autorelease pools">; + def err_arc_atomic_ownership : Error< "cannot perform atomic operation on a pointer to type %0: type has " "non-trivial ownership">; Index: include/clang/Basic/DiagnosticGroups.td === --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -498,6 +498,7 @@ def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">; def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak", [ARCRepeatedUseOfWeakMaybe]>; +def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">; def ObjCBridge : DiagGroup<"bridge-cast">; def DeallocInCategory:DiagGroup<"dealloc-in-category">; Index: test/SemaObjC/arc.m === --- test/SemaObjC/arc.m +++ test/SemaObjC/arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s typedef unsigned long NSUInteger; typedef const void * CFTypeRef; @@ -808,3 +808,10 @@ TKAssertEqual(object, nil); TKAssertEqual(object, (id)nil); } + +void block_capture_autoreleasing(A * __autoreleasing *a, A **b) { // expected-note {{declare the parameter __autoreleasing explicitly to suppress this warning}} expected-note {{declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools}} + ^{ +(void)*a; +(void)*b; // expected-warning {{block captures an
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5080 +def note_explicit_ownership_qualification : Note< + "explicitly specify ownership qualification">; + How about: warning: block captures an autoreleasing out-parameter, which may result in use-after-free bugs note: declare the parameter __autoreleasing explicitly to suppress this warning (include a fixit on this one) note: declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools https://reviews.llvm.org/D25844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks
ahatanak created this revision. ahatanak added a reviewer: rjmccall. ahatanak added a subscriber: cfe-commits. ARC implicitly marks indirect parameters passed to a function as autoreleasing and passing a block that captures those parameters to another function sometimes causes problems that are hard to debug. For example, in the code below, a block capturing fillMeIn is passed to enumerateObjectsUsingBlock, in which an autorelease pool is pushed and popped before and after the block is invoked: void doStuff(NSString **fillMeIn) { [@[@"array"] enumerateObjectsUsingBlock: ^(id obj, NSUInteger idx, BOOL* stop) { *stop = YES; *fillMeIn = [@"wow" mutableCopy]; } ]; } The object assigned to *fillMeIn gets autoreleased inside the block, so it gets destructed when the autorelease pool is drained, and the program will crash if it tries to access the NSString returned in fillMeIn after doStuff returns. To help the users figure out why the program is crashing, this patch adds a new warning "-Wblock-capture-autoreleasing" which warns about implicitly autoreleasing indirect parameters captured by blocks and suggests explicitly specifying ownership qualification. https://reviews.llvm.org/D25844 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/SemaObjC/arc.m Index: test/SemaObjC/arc.m === --- test/SemaObjC/arc.m +++ test/SemaObjC/arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s typedef unsigned long NSUInteger; typedef const void * CFTypeRef; @@ -808,3 +808,10 @@ TKAssertEqual(object, nil); TKAssertEqual(object, (id)nil); } + +void block_capture_autoreleasing(A * __autoreleasing *a, A **b) { // expected-note {{explicitly specify ownership qualification}} + ^{ +(void)*a; +(void)*b; // expected-warning {{block captures indirect parameter implicitly qualified with __autoreleasing}} + }(); +} Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -13473,6 +13473,20 @@ } return false; } + + // Warn about implicitly autoreleasing indirect parameters captured by blocks. + if (auto *PT = dyn_cast(CaptureType)) { +QualType PointeeTy = PT->getPointeeType(); +if (isa(PointeeTy.getCanonicalType()) && +PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing && +!isa(PointeeTy)) { + if (BuildAndDiagnose) { +S.Diag(Loc, diag::warn_block_capture_autoreleasing); +S.Diag(Var->getLocation(), diag::note_explicit_ownership_qualification); + } +} + } + const bool HasBlocksAttr = Var->hasAttr(); if (HasBlocksAttr || CaptureType->isReferenceType() || (S.getLangOpts().OpenMP && S.IsOpenMPCapturedDecl(Var))) { Index: include/clang/Basic/DiagnosticSemaKinds.td === --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -5073,6 +5073,12 @@ } // end "ARC and @properties" category +def warn_block_capture_autoreleasing : Warning< + "block captures indirect parameter implicitly qualified with __autoreleasing">, + InGroup, DefaultIgnore; +def note_explicit_ownership_qualification : Note< + "explicitly specify ownership qualification">; + def err_arc_atomic_ownership : Error< "cannot perform atomic operation on a pointer to type %0: type has " "non-trivial ownership">; Index: include/clang/Basic/DiagnosticGroups.td === --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -498,6 +498,7 @@ def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">; def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak", [ARCRepeatedUseOfWeakMaybe]>; +def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">; def ObjCBridge : DiagGroup<"bridge-cast">; def DeallocInCategory:DiagGroup<"dealloc-in-category">; Index: test/SemaObjC/arc.m === --- test/SemaObjC/arc.m +++ test/SemaObjC/arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s typede