[PATCH] D20113: Fix mangled name of method with ns_consumed parameters.
sdefresne created this revision.
sdefresne added a reviewer: rjmccall.
sdefresne added a subscriber: cfe-commits.
When a function/method use a parameter with "ns_consumed" attribute,
ensure that the mangled name is the same whether -fobjc-arc is used
or not.
Since "ns_consumed" attribute is generally used to inform ARC that
a function/method does sink the reference, it mean it is usually
implemented in a compilation unit compiled without -fobjc-arc but
used form a compilation unit compiled with it.
Originally found while trying to use "ns_consumed" attribute in an
Objective-C++ file in Chromium (http://crbug.com/599980) where it
caused a linker error.
Regression introduced by revision 262278 (previously the attribute
was incorrectly not part of the mangled name).
http://reviews.llvm.org/D20113
Files:
lib/Sema/SemaType.cpp
test/CodeGenObjCXX/arc-mangle.mm
test/CodeGenObjCXX/mangle.mm
Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -113,3 +113,10 @@
// CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
+void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
+void f(__strong id (*fn)(id, __attribute__((ns_consumed)) id)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -18,6 +18,8 @@
void f(const __unsafe_unretained id *) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFU19ns_returns_retainedP11objc_objectvE
void f(__attribute__((ns_returns_retained)) id (*fn)()) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4229,7 +4229,7 @@
}
}
- if (LangOpts.ObjCAutoRefCount && Param->hasAttr()) {
+ if (Param->hasAttr()) {
ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true);
HasAnyInterestingExtParameterInfos = true;
}
Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -113,3 +113,10 @@
// CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
+void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
+void f(__strong id (*fn)(id, __attribute__((ns_consumed)) id)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -18,6 +18,8 @@
void f(const __unsafe_unretained id *) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFU19ns_returns_retainedP11objc_objectvE
void f(__attribute__((ns_returns_retained)) id (*fn)()) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4229,7 +4229,7 @@
}
}
- if (LangOpts.ObjCAutoRefCount && Param->hasAttr()) {
+ if (Param->hasAttr()) {
ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true);
HasAnyInterestingExtParameterInfos = true;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20113: Fix mangled name of method with ns_consumed parameters.
sdefresne added a comment.
In http://reviews.llvm.org/D20113#425984, @rjmccall wrote:
> This is a good catch, thanks!
Thank you for the quick reply.
Please excuse me if I misunderstood you or if my remark appear off the mark,
this is my first time sending patches to clang. I'm not yet completely familiar
with all clang concepts :-)
> As a slight adjustment, It's probably better to just ignore this attribute
> when mangling the function type of an entity, the same way that we generally
> don't mangle return types because they don't affect overloading. That will
> require an extra flag to be passed down in a few places, but that's pretty
> reasonable. This will generally allow NS_CONSUMED and NS_RETURNS_RETAINED to
> be placed on existing APIs without changing their mangling unless the
> attribute is used in a secondary position (such as the type of an argument).
>
> Finally, you should give ns_returns_retained the same treatment, as well as
> the parameter-ABI attributes.
I'm not really sure what you mean by "ignore this attribute when mangling the
function type of an entity". I read this as "we should ensure that the
ns_consumed attribute is only mangled if it is applied to a parameter". Is this
correct?
If so, then there is nothing to do, as "ns_consumed" attibute is ignored if
applied to a non-parameter type as far as I can tell (see below for compilation
warning clang already emits regarding ignored attributes). So, my understanding
is that the ns_consumed attribute is only used if applied to a parameter, and
thus only when it is relevant to include it in the mangled name.
Regarding ns_returns_retained, it is ignored if not applied to the function
itself.
$ clang++ -fobjc-arc -o x.o -c x.mm
x.mm:1:19: warning: 'ns_consumed' attribute only applies to parameters
[-Wignored-attributes]
id __attribute__((ns_consumed)) f() { return 0; }
^
x.mm:2:23: warning: 'ns_consumed' attribute only applies to parameters
[-Wignored-attributes]
id g() __attribute__((ns_consumed)) { return 0; }
^
x.mm:7:26: warning: 'ns_returns_retained' only applies to function types; type
here is 'id' [-Wignored-attributes]
void k(id __attribute__((ns_returns_retained))) {}
So, could you elaborate a bit more on what additional changes I need to make to
my patch?
http://reviews.llvm.org/D20113
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20113: Fix mangled name of method with ns_consumed parameters.
sdefresne updated this revision to Diff 57924.
sdefresne added a comment.
Ok, this make sense. I've updated my change to follow your recommendation. Can
you take another look?
Using 'extern "C" { ... }" would probably not be an option in my case as I want
to use "ns_consumed" for the parameter of a templated class (i.e. this probably
won't work if using "C" mangling, and I'm not even sure it would compile).
http://reviews.llvm.org/D20113
Files:
lib/AST/ItaniumMangle.cpp
test/CodeGenObjCXX/arc-attrs.mm
test/CodeGenObjCXX/arc-mangle.mm
test/CodeGenObjCXX/mangle.mm
Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -std=c++11 -emit-llvm -fblocks -o - | FileCheck %s
// CHECK: @"_ZZ11+[A shared]E1a" = internal global
// CHECK: @"_ZZ11-[A(Foo) f]E1a" = internal global
@@ -113,3 +113,10 @@
// CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fP11objc_object
+void f(__attribute__((ns_consumed)) id) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_S0_E
+void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU13block_pointerFvP11objc_objectE
+void f(void (^)(__attribute__((ns_consumed)) id)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fobjc-runtime-has-weak -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fobjc-arc -fobjc-runtime-has-weak -triple %itanium_abi_triple -emit-llvm -fblocks -o - %s | FileCheck %s
// CHECK-LABEL: define {{.*}}void @_Z1fPU8__strongP11objc_object(i8**)
void f(__strong id *) {}
@@ -18,10 +18,14 @@
void f(const __unsafe_unretained id *) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFU19ns_returns_retainedP11objc_objectvE
void f(__attribute__((ns_returns_retained)) id (*fn)()) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fP11objc_object
+void f(__attribute__((ns_consumed)) id) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
void f(__strong id (*fn)(id, __attribute__((ns_consumed)) id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU13block_pointerFvU11ns_consumedP11objc_objectE
+void f(void (^)(__attribute__((ns_consumed)) id)) {}
template struct unsigned_c { };
Index: test/CodeGenObjCXX/arc-attrs.mm
===
--- test/CodeGenObjCXX/arc-attrs.mm
+++ test/CodeGenObjCXX/arc-attrs.mm
@@ -12,7 +12,7 @@
id x = makeObject1();
// CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z11makeObject2v()
- // CHECK-NEXT: call void @_Z13releaseObjectU11ns_consumedP11objc_object(i8* [[OBJ2]])
+ // CHECK-NEXT: call void @_Z13releaseObjectP11objc_object(i8* [[OBJ2]])
releaseObject(makeObject2());
// CHECK-NEXT: call void @objc_storeStrong(i8** [[X]], i8* null)
@@ -31,16 +31,16 @@
// CHECK-LABEL: define void @_Z12templateTestv
void templateTest() {
// CHECK: [[X:%.*]] = alloca i8*, align 8
- // CHECK-NEXT: [[OBJ1:%.*]] = call i8* @_Z12makeObjectT1IU8__strongP11objc_objectEU19ns_returns_retainedT_v()
+ // CHECK-NEXT: [[OBJ1:%.*]] = call i8* @_Z12makeObjectT1IU8__strongP11objc_objectET_v()
// CHECK-NEXT: store i8* [[OBJ1]], i8** [[X]], align 8
id x = makeObjectT1();
- // CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z12makeObjectT2IU8__strongP11objc_objectEU19ns_returns_retainedT_v()
- // CHECK-NEXT: call void @_Z13releaseObjectU11ns_consumedP11objc_object(i8* [[OBJ2]])
+ // CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z12makeObjectT2IU8__strongP11objc_objectET_v()
+ // CHECK-NEXT: call void @_Z13releaseObjectP11objc_object(i8* [[OBJ2]])
releaseObject(makeObjectT2());
// CHECK-NEXT: [[OBJ3:%.*]] = call i8* @_Z11makeObject1v()
- // CHECK-NEXT: call void @_Z14releaseObjectTIU8__strongP11objc_objectEvU11ns_consumedT_(i8* [[OBJ3]])
+ // CHECK-NEXT: call void @_Z14releaseObjectTIU8__strongP11objc_objectEvT_(i8* [[OBJ3]])
releaseObjectT(makeObject1());
// CHECK-NEXT: call void @objc_storeStrong(i8** [[X]], i8* null)
Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -2243,7 +2243,7 @@
FunctionTypeDepth.enterResultType();
// Mangle ns_returns_retained as an order-sensitive qualifier here.
-if (Proto->getExtInfo().getProducesResult())
+if (Proto->getExtInfo().get
Re: [PATCH] D20113: Fix mangled name of method with ns_consumed parameters.
sdefresne added a comment. Thank you for the comment. I think my change still needs to be reviewed and approved by someone (at least in the Phabricator interface it still appears as "Need review"). Can you do the review and give approval if it looks good to you? Once this is approved, should I just follow instructions at http://llvm.org/docs/Phabricator.html#subversion-and-arcanist? Do I need to request any kind of access rights? Sorry for the questions, as I said, this is my first change against clang/llvm. Cheers, http://reviews.llvm.org/D20113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add warning for blocks capturing {'this', raw pointers, references} (PR #144388)
https://github.com/sdefresne created
https://github.com/llvm/llvm-project/pull/144388
Add three new warning that reports when blocks captures 'this', a raw pointer
(i.e. not a pointer to a reference counted object) or a reference.
Those warnings can be used in Objective-C++ to detect blocks that create
potentially unsafe capture (as the captured pointer can be dangling by the time
the block is captured).
To reduce the false positive, no warning is emitted if the block is detected as
not escaping.
The three warnings are:
- -Wblocks-capturing-this
- -Wblocks-capturing-reference
- -Wblocks-capturing-raw-pointer
Fixes issue #143924.
>From f41dc700d51726563a19aa8fa87b81075ba1859e Mon Sep 17 00:00:00 2001
From: Sylvain Defresne
Date: Mon, 16 Jun 2025 17:09:24 +0200
Subject: [PATCH] Add warning for blocks capturing {'this', raw pointers,
references}
Add three new warning that reports when blocks captures 'this', a
raw pointer (i.e. not a pointer to a reference counted object) or
a reference.
Those warnings can be used in Objective-C++ to detect blocks that
create potentially unsafe capture (as the captured pointer can be
dangling by the time the block is captured).
To reduce the false positive, no warning is emitted if the block
is detected as not escaping.
The three warnings are:
- -Wblocks-capturing-this
- -Wblocks-capturing-reference
- -Wblocks-capturing-raw-pointer
Fixes issue #143924.
---
.../clang/Basic/DiagnosticSemaKinds.td| 11 ++
clang/include/clang/Sema/Sema.h | 19 +++-
clang/lib/Sema/SemaDecl.cpp | 44 ++--
clang/lib/Sema/SemaDeclObjC.cpp | 2 +-
clang/lib/Sema/SemaExpr.cpp | 13 +++
clang/lib/Sema/SemaExprObjC.cpp | 6 +-
.../warn-blocks-capturing-pointers.mm | 103 ++
7 files changed, 184 insertions(+), 14 deletions(-)
create mode 100644 clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 979ff60b73b75..4b33c40658028 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning <
"block implicitly retains 'self'; explicitly mention 'self' to indicate "
"this is intended behavior">,
InGroup>, DefaultIgnore;
+def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">,
+ InGroup>,
+ DefaultIgnore;
+def warn_blocks_capturing_reference
+: Warning<"block implicitly captures a C++ reference">,
+ InGroup>,
+ DefaultIgnore;
+def warn_blocks_capturing_raw_pointer
+: Warning<"block implicitly captures a raw pointer">,
+ InGroup>,
+ DefaultIgnore;
def warn_arc_possible_repeated_use_of_weak : Warning <
"weak %select{variable|property|implicit property|instance variable}0 %1 may
"
"be accessed multiple times in this %select{function|method|block|lambda}2 "
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 29452bb37260d..7a6ec7c7a3f2c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8227,6 +8227,22 @@ class Sema final : public SemaBase {
}
};
+ /// Store information about a diagnosable block catpure.
+ struct BlockCapture {
+/// Enumeration representing types of block captures that may be
+/// diagnosable because they could be problematic.
+enum CaptureType {
+ Self,
+ This,
+ Reference,
+ RawPointer,
+};
+
+SourceLocation Loc;
+const BlockDecl *BD;
+CaptureType Type;
+ };
+
/// Check an argument list for placeholders that we won't try to
/// handle later.
bool CheckArgsForPlaceholders(MultiExprArg args);
@@ -8243,8 +8259,7 @@ class Sema final : public SemaBase {
/// List of SourceLocations where 'self' is implicitly retained inside a
/// block.
- llvm::SmallVector, 1>
- ImplicitlyRetainedSelfLocs;
+ llvm::SmallVector DiagnosableBlockCaptures;
/// Do an explicit extend of the given block pointer if we're in ARC.
void maybeExtendBlockObject(ExprResult &E);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5cffd82e3372e..713cbf9e3ef2c 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16149,7 +16149,7 @@ class ExitFunctionBodyRAII {
bool IsLambda = false;
};
-static void diagnoseImplicitlyRetainedSelf(Sema &S) {
+static void diagnoseBlockCaptures(Sema &S) {
llvm::DenseMap EscapeInfo;
auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
@@ -16170,13 +16170,35 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) {
return It->second = R;
};
- // If the location where 'self' is implicitly retained is inside a escaping
- // block, emit a diagnostic.
- for (const
[clang] Add warning for blocks capturing {'this', raw pointers, references} (PR #144388)
@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning < "block implicitly retains 'self'; explicitly mention 'self' to indicate " "this is intended behavior">, InGroup>, DefaultIgnore; +def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">, + InGroup>, + DefaultIgnore; +def warn_blocks_capturing_reference +: Warning<"block implicitly captures a C++ reference">, + InGroup>, + DefaultIgnore; +def warn_blocks_capturing_raw_pointer +: Warning<"block implicitly captures a raw pointer">, sdefresne wrote: I initially developed this change because I was concerned about the number of occurrences of blocks capturing C pointers (or `this` or references) I was seeing during code reviews (or when investigating crashes). At this point this was just a feeling that there was a problem in our code base, but I had no hard data. Having developed this change, I was able to build the code base I'm familiar with (Chrome on iOS) with the three warnings enabled, and spent the last few days looking at all the warnings reported by the current change, trying to get an idea of the volume of true and false positive they would find. I'm still doing the analysis, but I can share some intermediate results. I saved all the build output, then collected all unique warnings triplet "warning-name, filename, line" and looks at each of them. This gives me a lower bound of the number of errors (as the same warning can happen multiple times per line if multiple pointers are captured). Numbers of occurrences per warning in production code (additionally in test code): - `-Wblocks-capturing-this`: 28 (571) - `-Wblocks-capturing-reference`: 7 (37) - `-Wblocks-capturing-raw-pointer`: 53 (174) Trying to categorize the 88 warnings in production - 53 are totally true positive, and would need to be fixed in our code base (60%), - 13 are captured blocks that are non-escaping blocks not marked as such (15%), - 7 are capturing pointers to static variables (8%), - 2 are capturing function pointers (2%) -- likely due to a bug in my implementation, - 2 are pointers to pointers and should have used `__block` variables anyway (2%). The last 11 warnings are in third-party code I'm not really familiar with. If we ignore those, this gives 55 true positive (I'm counting the 53 above and the 2 pointers to pointers here) out of 77 warnings, or 73% of true positive. Among the warnings I left in the false positive, it could be argued that some of them are not false positive but could be counted as true positive. For exemple, among the 7 pointers to static variables, only 4 of them could be deduced statically. The other 3 are blocks capturing `const char*` parameters where the function are only ever called with pointer to static string (this is something I know because I know the code base, but that the compiler cannot prove). I didn't have time to look at all the warnings in test code as there are so many warnings reported there (but I think many could be fixed by marking a few helper function as taking a non-escaping block). Given those numbers, I think those warning are valuable for the Chrome on iOS codebase. I am less familiar with other codebase, though the Chromium codebase is considered of a relatively good quality, so maybe the warnings could also be interesting in other code bases. If you think that despite those numbers the warnings are not interesting in other code bases, then I will consider implementing them as Chrome specific clang plugins (the [Chromium documentation](https://chromium.googlesource.com/chromium/src/+/main/docs/writing_clang_plugins.md) recommends first trying to get warning adopted by clang, and this is why I started this effort). https://github.com/llvm/llvm-project/pull/144388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add warning for blocks capturing {'this', raw pointers, references} (PR #144388)
@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning <
"block implicitly retains 'self'; explicitly mention 'self' to indicate "
"this is intended behavior">,
InGroup>, DefaultIgnore;
+def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">,
+ InGroup>,
+ DefaultIgnore;
+def warn_blocks_capturing_reference
+: Warning<"block implicitly captures a C++ reference">,
+ InGroup>,
+ DefaultIgnore;
+def warn_blocks_capturing_raw_pointer
+: Warning<"block implicitly captures a raw pointer">,
sdefresne wrote:
The table with the data with true/false positive vs kind of capture.
kind | count | true positive | no escape block | __block | static pointer |
function pointer | other | ratio
-- | -- | -- | -- | -- | -- | -- | -- | --
this | 28 | 20 | 0 | 0 | 0 | 0 | 8 | 71.43%
reference | 7 | 3 | 3 | 0 | 1 | 0 | 0 | 42.86%
pointer | 53 | 30 | 10 | 2 | 6 | 2 | 3 | 56.60%
total | 88 | 53 | 13 | 2 | 7 | 2 | 11 | 60.23%
Regarding the suppression mechanism, besides `#pragma clang diagnostic`, the
developer can be explicit about the capture either by declaring a local struct
to store the captured values, or using variables marked with `__block`.
So for example, one of the "false positive" is this [code in
dawn](https://dawn.googlesource.com/dawn/+/refs/heads/main/src/dawn/native/metal/BufferMTL.mm#156)
```
Ref deviceRef = GetDevice();
wgpu::Callback callback = hostMappedDesc->disposeCallback;
void* userdata = hostMappedDesc->userdata;
auto dispose = ^(void*, NSUInteger) {
deviceRef->GetCallbackTaskManager()->AddCallbackTask(
[callback, userdata] { callback(userdata); });
};
```
I think this can be fixed by writing either
```
Ref deviceRef = GetDevice();
__block wgpu::Callback callback = hostMappedDesc->disposeCallback;
__block void* userdata = hostMappedDesc->userdata;
auto dispose = ^(void*, NSUInteger) {
deviceRef->GetCallbackTaskManager()->AddCallbackTask(
[callback, userdata] { callback(userdata); });
};
```
or
```
Ref deviceRef = GetDevice();
struct { wgpu::Callback callback; void* userdata; } captured = {
hostMappedDesc->disposeCallback,
hostMappedDesc->userdata,
};
auto dispose = ^(void*, NSUInteger) {
deviceRef->GetCallbackTaskManager()->AddCallbackTask(
[captured] { captured.callback(captured.userdata); });
};
```
Since the warning does not inspect the captured objects, and only warn about
pointers and reference, using a structure hides the capture from the warning.
In the same way by using `__block` the pointer are copied in the block
explicitly, and thus the warning should not fire (I have not tested, if it
does, I would consider this a bug in my implementation and will fix it). The
same pattern can also work for references.
Basically, to disable the warning, you would have to be explicit that the
capture is intended.
https://github.com/llvm/llvm-project/pull/144388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
