[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 = 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=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:
[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
