Add @PointerBounds macro #76969

hnrklssn · 2024-10-10T22:51:59Z

@PointerBounds is a macro intended to be applied by ClangImporter when importing functions with pointer parameters from C headers. By leveraging C attributes we can get insight into bounds, esapability, and (eventually) lifetimes of pointers, allowing us to map them to safe(r) and more ergonomic types than UnsafePointer.

This initial macro implementation supports CountedBy and Sizedby, but not yet EndedBy. It can generate function overloads with and without an explicit count parameter, as well as with UnsafeBufferPointer or Span (if marked nonescaping), and any of their combinations. It supports nullable/optional pointers, and both mutable and immutable pointers. It supports arbitrary count expressions. These are passed to the macro as a string literal since any parameters referred to in the count expression will not have been declared yet when parsing the macro.

It does not support indirect pointers or inout parameters. It supports functions with return values, but returned pointers can not be bounds checked yet.

Bounds checked pointers must be of type Unsafe[Mutable]Pointer[?]. Count expressions must conform to the BinaryInteger protocol, and have an initializer with signature "init(exactly: Int) -> T?" (or accept a supertype of Int).

rdar://137628612

hnrklssn · 2024-10-10T22:52:31Z

@swift-ci Please smoke test

DougGregor · 2024-10-11T21:37:45Z

stdlib/public/PointerBounds/CMakeLists.txt

@@ -0,0 +1,11 @@
+add_swift_target_library(swift_PointerBounds ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} IS_STDLIB


We shouldn't create a new target here. The actual macro declaration and the types it depend on will go into the standard library so it's always available. In the short term, we can put them into the test cases themselves or (better) a module within the test directory that we build and feed in to the test command.

Wouldn't placing it in the test dir rather than conditionally making it part of the stdlib make it harder to create pre-release versions for testing on real codebases?

stdlib/public/PointerBounds/PointerBounds.swift

lib/Macros/Sources/SwiftMacros/CMakeLists.txt

DougGregor · 2024-10-11T21:51:57Z

test/Macros/PointerBounds/CountedBy/CountExpr.swift

+// CHECK-NEXT: }
+// CHECK-NEXT: 
+// CHECK-NEXT: func myFunc(_ ptr: UnsafeBufferPointer<CInt>, _ size: CInt, _ count: CInt) {
+// CHECK-NEXT:     let _ptrCount: some BinaryInteger = size * count


I'd rather not have _ptrCount typed to some BinaryInteger, which is erasing information when we could be more precise. Maybe just leave off the type annotation and let the compiler figure it out?

I did this because the expression could be any type, and in the case that it does have the wrong type, the type error here makes it clearer that the error is because of the user annotation rather than a bug in the macro. Maybe there's a neater way to do something like this?

let _ptrCount = size * count let _assertPtrCountBinaryIntegerType: some BinaryInteger = _ptrCount // unused, only for asserting type

I guess if we have a function like this we could form a call to it:

@_alwaysEmitIntoClient func _assertBinaryInteger<T: BinaryInteger>(_: T) { }

... but the result is basically the same, so what you've done will work. My main concern is that the opaque type we're creating here will have some runtime impact that doesn't get optimized away.

test/Macros/PointerBounds/CountedBy/CountExpr.swift

test/Macros/PointerBounds/CountedBy/MultipleParams.swift

hnrklssn · 2024-10-15T01:21:21Z

Managed to get things to build and run properly now. Will start addressing comments tomorrow.

hnrklssn · 2024-10-16T06:57:57Z

@swift-ci Please smoke test

lib/Macros/Sources/SwiftMacros/PointerBoundsMacro.swift

DougGregor · 2024-10-21T17:44:50Z

lib/Macros/Sources/SwiftMacros/PointerBoundsMacro.swift

+
+struct CountedBy: ParamInfo {
+    var pointerIndex: Int
+    var count: ExprSyntax


Should this be normalized to a count index? That would be more consistent with the other parameters that are resolved as indices.

How would you then represent count expressions with e.g. addition or multiplication?

To get there, we'd have to "parse" the C/C++ expression and map it into something. We can represent that here with an expression in terms of Swift if we need to.

Indeed. So far I've punted that issue, because the basic operations that cover most use cases have the same syntax in Swift. I imagine a macro syntax like this: .mul(.param(2), .param(3)). This has the upside of only being able to represent expressions we support, and downside of requiring us to add explicit support for every type of expression. It would also let us add an enum case representing the return value of a function, so malloc could be annotated with @PointerBounds(.sizedByOrNull(pointer: .retVal, size: .param(1))).

lib/Macros/Sources/SwiftMacros/PointerBoundsMacro.swift

stdlib/public/PointerBounds/CMakeLists.txt

DougGregor · 2024-10-21T18:56:29Z

stdlib/public/PointerBounds/PointerBounds.swift

+// Stub interfaces for testing until Span lands
+public struct Span<T> {
+    public var count: Int
+    var ptr: UnsafeBufferPointer<T>
+    public func withUnsafeBufferPointer<R>(_ body: (UnsafeBufferPointer<T>) throws -> R) rethrows -> R {
+        return try body(ptr)
+    }
+}
+public protocol RawSpan {
+    var byteCount: Int { get }
+    func withUnsafeBytes<R>(_ body: (UnsafeRawBufferPointer) throws -> R) rethrows -> R
+}


At this point, I expect Span will land before this PR, now that it's been accepted.

Great! I'll keep it for now for my own testing, seeing as Span hasn't landed, but I'll make sure to remove it before merging this.

DougGregor · 2024-10-21T19:02:32Z

test/Macros/PointerBounds/CountedBy/CountExpr.swift

+// CHECK-NEXT: }
+// CHECK-NEXT: 
+// CHECK-NEXT: func myFunc(_ ptr: UnsafeBufferPointer<CInt>, _ size: CInt, _ count: CInt) {
+// CHECK-NEXT:     let _ptrCount: some BinaryInteger = size * count


I guess if we have a function like this we could form a call to it:

@_alwaysEmitIntoClient func _assertBinaryInteger<T: BinaryInteger>(_: T) { }

... but the result is basically the same, so what you've done will work. My main concern is that the opaque type we're creating here will have some runtime impact that doesn't get optimized away.

test/lit.cfg

hnrklssn · 2024-10-29T17:09:39Z

depends on swiftlang/swift-syntax#2886

stdlib/public/core/CMakeLists.txt

DougGregor · 2024-11-04T23:34:45Z

depends on swiftlang/swift-syntax#2886

I'd like to break this dependency on the swift-syntax change, which I feel needs more discussion independently of this PR. Can you bring in whatever bits from swiftlang/swift-syntax#2886 that you need here so we can land this PR without any changes to swift-syntax? Once we do get to an answer there, we can move over to the corresponding API.

hnrklssn · 2024-11-05T21:36:22Z

@swift-ci Please smoke test

hnrklssn · 2024-11-06T00:57:22Z

@swift-ci Please smoke test

hnrklssn · 2024-11-06T21:36:59Z

@swift-ci Please smoke test

hnrklssn · 2024-11-07T01:11:34Z

@swift-ci Please smoke test

hnrklssn · 2024-11-07T01:14:36Z

@swift-ci Please smoke test

DougGregor · 2024-11-07T22:09:35Z

@swift-ci Please smoke test

hnrklssn · 2024-11-07T23:51:03Z

/home/build-user/swift/lib/Macros/Sources/SwiftMacros/DebugDescriptionMacro.swift:244:7: warning: default will never be executed
      @unknown default:
      ^
/home/build-user/swift/lib/Macros/Sources/SwiftMacros/DebugDescriptionMacro.swift:440:5: warning: default will never be executed
    @unknown default:
    ^
/home/build-user/swift/lib/Macros/Sources/SwiftMacros/OptionSetMacro.swift:122:12: error: cannot convert return expression of type '(StructDeclSyntax, EnumDeclSyntax, GenericArgumentSyntax.Argument)' to return type '(StructDeclSyntax, EnumDeclSyntax, TypeSyntax)?'
    return (structDecl, optionsEnum, rawType)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                              as! (StructDeclSyntax, EnumDeclSyntax, TypeSyntax)
/home/build-user/swift/lib/Macros/Sources/SwiftMacros/DistributedResolvableMacro.swift:145:39: error: value of type 'SameTypeRequirementSyntax.LeftType' has no member 'isActorSystem'
           where sameTypeReq.leftType.isActorSystem:
                 ~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~
/home/build-user/swift/lib/Macros/Sources/SwiftMacros/DistributedResolvableMacro.swift:146:40: error: cannot assign value of type 'SameTypeRequirementSyntax.RightType' to type 'TypeSyntax?'
        specificActorSystemRequirement = sameTypeReq.rightType.trimmed
                                       ^

@DougGregor this doesn't look related to this patch at all. Is it a failure in main?

DougGregor · 2024-11-07T23:53:39Z

I suspect you need to rebase your swift and update your swift-syntax

@PointerBounds is a macro intended to be applied by ClangImporter when importing functions with pointer parameters from C headers. By leveraging C attributes we can get insight into bounds, esapability, and (eventually) lifetimes of pointers, allowing us to map them to safe(r) and more ergonomic types than UnsafePointer. This initial macro implementation supports CountedBy and Sizedby, but not yet EndedBy. It can generate function overloads with and without an explicit count parameter, as well as with UnsafeBufferPointer or Span (if marked nonescaping), and any of their combinations. It supports nullable/optional pointers, and both mutable and immutable pointers. It supports arbitrary count expressions. These are passed to the macro as a string literal since any parameters referred to in the count expression will not have been declared yet when parsing the macro. It does not support indirect pointers or inout parameters. It supports functions with return values, but returned pointers can not be bounds checked yet. Bounds checked pointers must be of type Unsafe[Mutable]Pointer[?]<T> or Unsafe[Mutable]RawPointer[?]. Count expressions must conform to the BinaryInteger protocol, and have an initializer with signature "init(exactly: Int) -> T?" (or accept a supertype of Int). rdar://137628612

hnrklssn · 2024-11-07T23:59:25Z

@swift-ci Please smoke test

hnrklssn · 2024-11-08T02:38:00Z

@swift-ci Please smoke test

DougGregor

Bah, Python lint issue due to the long name

utils/swift_build_support/swift_build_support/products/wasmstdlib.py

DougGregor · 2024-11-08T07:18:48Z

@swift-ci please smoke test

DougGregor · 2024-11-08T16:37:15Z

@hnrklssn theSpan PR landed before this :)

error: 'Span' is ambiguous for type lookup in this context

hnrklssn · 2024-11-08T20:31:53Z

@swift-ci Please smoke test

DougGregor · 2024-11-10T15:25:10Z

@swift-ci please smoke test Windows

DougGregor · 2024-11-11T15:46:14Z

test/Macros/PointerBounds/CountedBy/MutableSpan.swift

+}
+
+// CHECK:      @_alwaysEmitIntoClient
+// CHECK-NEXT: func myFunc(_ ptr: MutableSpan<CInt>) {


MutableSpan isn't actually defined anywhere. Not sure why the Windows tests are crashing here, but at this point it isn't expected to really work.

DougGregor

XFAIL'ing tests that need more standard library support to work properly

test/Macros/PointerBounds/CountedBy/MutableSpan.swift

test/Macros/PointerBounds/SizedBy/MutableRawSpan.swift

DougGregor · 2024-11-11T17:49:47Z

@swift-ci please smoke test

hnrklssn · 2024-11-11T20:33:01Z

test/Macros/PointerBounds/CountedBy/MutableSpan.swift

@@ -1,6 +1,6 @@
 // REQUIRES: swift_swift_parser
 // REQUIRES: pointer_bounds
-
+// XFAIL: OS=windows-msvc


@DougGregor if MutableSpan is unavailable, shouldn't we XFAIL this on all platforms?

I suspect that the macro is being expanded but nothing ever type-checks it, so it's "working" for now but will fail with subsequent changes. We can disable then.

hnrklssn requested a review from a team as a code owner October 10, 2024 22:51

hnrklssn requested review from DougGregor, airspeedswift, rapidsna, tbkka, Xazax-hun, atrick and egorzhdan October 10, 2024 22:53

DougGregor reviewed Oct 11, 2024

View reviewed changes

hnrklssn force-pushed the pointer-bounds-macro branch from 55744d9 to 20ad631 Compare October 15, 2024 00:43

DougGregor reviewed Oct 21, 2024

View reviewed changes

DougGregor approved these changes Nov 4, 2024

View reviewed changes

stdlib/public/core/CMakeLists.txt Show resolved Hide resolved

hnrklssn force-pushed the pointer-bounds-macro branch from 56ffed7 to d668cfb Compare November 5, 2024 05:42

hnrklssn mentioned this pull request Nov 5, 2024

Infer @PointerBounds macro from clang __counted_by parameters #77387

Merged

hnrklssn added 3 commits November 7, 2024 15:57

Don't enable SymbolLinkageMarkers, mark @_alwaysEmitIntoClient

eca4e00

Document @PointerBounds and PointerParam

d42aa46

hnrklssn added 7 commits November 7, 2024 15:57

Move @PointerBounds to stdlib core, support Implicitly Unwrapped types

69ec3fc

Handle FQTNs, always emit return even with Void-return type

ac3e684

run swift-format

b3f4801

add util properties for TypeSyntaxProtocol

133789f

don't use if- or switch-expressions

e06c1d6

address build failures

7932c90

fix builds support tests

fed9049

hnrklssn force-pushed the pointer-bounds-macro branch from 65898f3 to fed9049 Compare November 7, 2024 23:58

DougGregor reviewed Nov 8, 2024

View reviewed changes

utils/swift_build_support/swift_build_support/products/wasmstdlib.py Outdated Show resolved Hide resolved

Fix Python linting issue

b254623

remove Span stubs

eb96e51

DougGregor reviewed Nov 11, 2024

View reviewed changes

DougGregor approved these changes Nov 11, 2024

View reviewed changes

test/Macros/PointerBounds/CountedBy/MutableSpan.swift Outdated Show resolved Hide resolved

test/Macros/PointerBounds/SizedBy/MutableRawSpan.swift Outdated Show resolved Hide resolved

XFAIL tests that can't work without more library support

ae716bc

hnrklssn commented Nov 11, 2024

View reviewed changes

hnrklssn merged commit 0678829 into swiftlang:main Nov 11, 2024
3 checks passed

		@@ -0,0 +1,11 @@
		add_swift_target_library(swift_PointerBounds ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} IS_STDLIB

Add @PointerBounds macro #76969

Add @PointerBounds macro #76969

Conversation

hnrklssn commented Oct 10, 2024

hnrklssn commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnrklssn commented Oct 15, 2024

hnrklssn commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnrklssn commented Oct 29, 2024

DougGregor commented Nov 4, 2024

hnrklssn commented Nov 5, 2024

hnrklssn commented Nov 6, 2024

hnrklssn commented Nov 6, 2024

hnrklssn commented Nov 7, 2024

hnrklssn commented Nov 7, 2024

DougGregor commented Nov 7, 2024

hnrklssn commented Nov 7, 2024

DougGregor commented Nov 7, 2024

hnrklssn commented Nov 7, 2024

hnrklssn commented Nov 8, 2024

DougGregor left a comment

Choose a reason for hiding this comment

DougGregor commented Nov 8, 2024

DougGregor commented Nov 8, 2024

hnrklssn commented Nov 8, 2024

DougGregor commented Nov 10, 2024

Choose a reason for hiding this comment

DougGregor left a comment

Choose a reason for hiding this comment

DougGregor commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment