Skip to content

Fixes for __attribute__((ns_consumed)) block parameters #28527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Fixes for __attribute__((ns_consumed)) block parameters #28527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d5ff20
IRGen: Implement support for __attribute__((ns_consumed)) block param…
slavapestov Dec 2, 2019
eef1428
PrintAsObjC: Support for blocks with __owned parameters
slavapestov Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions lib/IRGen/GenClangType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,10 @@ clang::CanQualType GenClangType::visitSILFunctionType(CanSILFunctionType type) {
}

SmallVector<clang::QualType, 4> paramTypes;
SmallVector<clang::FunctionProtoType::ExtParameterInfo, 4> extParamInfos;
for (auto paramTy : type->getParameters()) {
clang::FunctionProtoType::ExtParameterInfo extParamInfo;

// Blocks should only take direct +0 parameters.
switch (paramTy.getConvention()) {
case ParameterConvention::Direct_Guaranteed:
Expand All @@ -594,7 +597,9 @@ clang::CanQualType GenClangType::visitSILFunctionType(CanSILFunctionType type) {
break;

case ParameterConvention::Direct_Owned:
llvm_unreachable("block takes owned parameter");
extParamInfo = extParamInfo.withIsConsumed(true);
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@varungandhi-apple You'll need to make an analogous change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Comment for my own reference): #27479 needs to have an analogous change in ClangTypeConverter.cpp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need to make any changes since visitSILFunctionType in ClangTypeConverter is unreachable -- it should receive only AST types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owned can appear in AST function parameters as well; it's in the ParameterTypeFlags.


case ParameterConvention::Indirect_In:
case ParameterConvention::Indirect_In_Constant:
case ParameterConvention::Indirect_Inout:
Expand All @@ -606,12 +611,16 @@ clang::CanQualType GenClangType::visitSILFunctionType(CanSILFunctionType type) {
paramTy.getArgumentType(IGM.getSILModule(), type));
if (param.isNull())
return clang::CanQualType();

paramTypes.push_back(param);
extParamInfos.push_back(extParamInfo);
}

// Build the Clang function type.
clang::FunctionProtoType::ExtProtoInfo defaultEPI;
auto fnTy = clangCtx.getFunctionType(resultType, paramTypes, defaultEPI);
clang::FunctionProtoType::ExtProtoInfo extProtoInfo;
extProtoInfo.ExtParameterInfos = extParamInfos.begin();
Copy link
Contributor

@varungandhi-apple varungandhi-apple Dec 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slavapestov -- is SmallVector guaranteed to have contiguous storage in the resizing case? (If not, then this wouldn't work because there's no way to recover the split storage since ExtParameterInfos is a pointer). And is clangCtx.getFunctionType guaranteed to create a copy of this array? Otherwise, there's a lifetime problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to both of these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if these points are documented anywhere? I looked at the Doxygen and I couldn't find this. Also, is there some general guidelines at play here (e.g. "getFooType" always deep-copies everything in Clang) or is this a "yes, I know these particular points are true"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All AST nodes are supposed to be self-contained in both Clang and Swift (except inasmuch as they reference other AST nodes, which should themselves be self-contained). Creating an AST node should always be deep-copying any necessary arrays into trailing storage or some other associated storage.

There might be exceptions for TypeVariableTypes in Swift, they're a weird case.


auto fnTy = clangCtx.getFunctionType(resultType, paramTypes, extProtoInfo);
clang::QualType ptrTy;

switch (kind) {
Expand Down
13 changes: 12 additions & 1 deletion lib/PrintAsObjC/DeclAndTypePrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,7 +1934,18 @@ class DeclAndTypePrinter::Implementation
if (!FT->getParams().empty()) {
interleave(FT->getParams(),
[this](const AnyFunctionType::Param &param) {
print(param.getOldType(), OTK_None, param.getLabel(),
switch (param.getValueOwnership()) {
case ValueOwnership::Default:
case ValueOwnership::Shared:
break;
case ValueOwnership::Owned:
os << "SWIFT_RELEASES_ARGUMENT ";
break;
case ValueOwnership::InOut:
llvm_unreachable("bad specifier");
}

print(param.getParameterType(), OTK_None, param.getLabel(),
IsFunctionParam);
},
[this] { os << ", "; });
Expand Down
5 changes: 5 additions & 0 deletions lib/PrintAsObjC/PrintAsObjC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ static void writePrologue(raw_ostream &out, ASTContext &ctx) {
"#else\n"
"# define SWIFT_NOESCAPE\n"
"#endif\n"
"#if __has_attribute(ns_consumed)\n"
"# define SWIFT_RELEASES_ARGUMENT __attribute__((ns_consumed))\n"
"#else\n"
"# define SWIFT_RELEASES_ARGUMENT\n"
"#endif\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it it's general policy to use Swift-specific macros here (which we can just add whenever we need them) rather than the NS ones?

"#if __has_attribute(warn_unused_result)\n"
"# define SWIFT_WARN_UNUSED_RESULT __attribute__((warn_unused_result))\n"
"#else\n"
Expand Down
1 change: 1 addition & 0 deletions test/IRGen/Inputs/usr/include/Gizmo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ typedef long NSInteger;
- (void) setFrame: (struct NSRect) rect;
- (void) frob;
- (void) test: (struct Fob) fob;
- (void) perform: (void (^)(NS_CONSUMED Gizmo*)) block;
+ (void) runce;
@end

Expand Down
19 changes: 19 additions & 0 deletions test/IRGen/objc_block_consumed.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@

// RUN: %empty-directory(%t)
// RUN: %build-irgen-test-overlays
// RUN: %target-swift-frontend(mock-sdk: -sdk %S/Inputs -I %t) -primary-file %s -emit-ir -disable-objc-attr-requires-foundation-module
// RUN: %target-swift-frontend(mock-sdk: -sdk %S/Inputs -I %t) -primary-file %s -emit-silgen -disable-objc-attr-requires-foundation-module | %FileCheck %s

// We want to test that IRGen doesn't assert on this code. SIL is the best place
// to file check that the block parameter is actually +1.

// REQUIRES: CPU=x86_64
// REQUIRES: objc_interop

import gizmo

// CHECK-LABEL: sil hidden [ossa] @$s19objc_block_consumed24passBlockWithConsumedArgyySo5GizmoC_ADtF : $@convention(thin) (@guaranteed Gizmo, @guaranteed Gizmo) -> () {
func passBlockWithConsumedArg(_ g: Gizmo, _ other: Gizmo) {
// CHECK: objc_method %0 : $Gizmo, #Gizmo.perform!1.foreign : (Gizmo) -> (((Gizmo?) -> ())?) -> (), $@convention(objc_method) (Optional<@convention(block) (@owned Optional<Gizmo>) -> ()>, Gizmo) -> ()
g.perform { other in }
}
5 changes: 5 additions & 0 deletions test/Interpreter/SDK/Inputs/objc_block_consumed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#import <Foundation/Foundation.h>

static inline void takesBlockWithConsumedArg(void (^ block)(NS_RELEASES_ARGUMENT NSObject *x), NSObject *x) {
block(x);
}
21 changes: 21 additions & 0 deletions test/Interpreter/SDK/objc_block_consumed.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift %s -import-objc-header %S/Inputs/objc_block_consumed.h -o %t/main
// RUN: %target-run %t/main

// REQUIRES: executable_test
// REQUIRES: objc_interop

import Foundation
import StdlibUnittest

class C : NSObject {
var tracked = LifetimeTracked(0)
}

var ObjCBlockConsumedTestSuite = TestSuite("ObjCBlockConsumed")

ObjCBlockConsumedTestSuite.test("Test") {
takesBlockWithConsumedArg({ arg in }, C())
}

runAllTests()
3 changes: 3 additions & 0 deletions test/PrintAsObjC/blocks.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ typealias MyBlockWithNoescapeParam = (() -> ()) -> Int
return input
}

// CHECK-NEXT: - (void)blockWithConsumingArgument:(void (^ _Nonnull)(SWIFT_RELEASES_ARGUMENT NSObject * _Nonnull))block;
@objc func blockWithConsumingArgument(_ block: @escaping (__owned NSObject) -> ()) {}

// CHECK-NEXT: @property (nonatomic, copy) NSInteger (^ _Nullable savedBlock)(NSInteger);
@objc var savedBlock: ((Int) -> Int)?

Expand Down