Skip to content
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.

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

[BoundsSafety] Parse external bounds attributes in ObjC methods #10068

Open
wants to merge 1 commit into
base: stable/20240723
Choose a base branch
from

Conversation

hnrklssn
Copy link

This parses counted_by et. al. for parameters and return types in Objective-C method signatures. No semantic checking is done to ensure that protocol, interface and implementation are aligned.

rdar://145190177

This parses counted_by et. al. for parameters and return types in
Objective-C method signatures. No semantic checking is done to ensure
that protocol, interface and implementation are aligned.

rdar://145190177
@hnrklssn
Copy link
Author

@swift-ci please smoke test

@hnrklssn
Copy link
Author

@swift-ci please test llvm

// CHECK-NEXT: |-ObjCInterface {{.*}} 'CountedByClass'
// CHECK-NEXT: |-ObjCMethodDecl {{.*}} - simpleParam:: 'void'
// CHECK-NEXT: | |-ImplicitParamDecl {{.*}} self 'CountedByClass *'
// CHECK-NEXT: | |-ImplicitParamDecl {{.*}} _cmd 'SEL':'SEL *'

Choose a reason for hiding this comment

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

We should probably consciously decide what to do with SEL.

Copy link
Author

Choose a reason for hiding this comment

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

As in, we should make it __single? I guess that would apply to self also.

Choose a reason for hiding this comment

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

SEL is typically a C string. I don't know for sure what constraints the runtime puts on the contents of the string, but @mikeash should know.

self (like all other ObjC pointers) should always be __single, and I think that one is un-controversial. I seem to recall that ObjC pointers are not actually PointerTypes, though?

Copy link

Choose a reason for hiding this comment

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

Currently, SEL is always a C string. We reserve the right to change that in the future (there are some advantages to be had by making some common selectors into small integers), but I'm skeptical that we'd be able to actually pull it off, there has to be a lot of code out there that casts to char * and expects it to work.

The only constraints on the contents are that they must be NUL terminated and unique, i.e. you're not allowed to have two distinct SEL values that both point to bytewise-equal strings. And they must remain valid for the lifetime of the process. The runtime ensures these things if you get your SEL by legal means.

Copy link
Author

@hnrklssn hnrklssn Feb 20, 2025

Choose a reason for hiding this comment

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

Right. Is SEL * a char * or char **? Do we ever cast _cmd/*_cmd to char *, or is it ~always accessed through something like NSStringFromSelector? Would making it __single in the public interface but __null_terminated behind the scenes be feasible? I guess I'm wondering how much we expect user code to access the selector internals.

Copy link

Choose a reason for hiding this comment

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

Yes, sel_getName is the right way to get the contents. Right now it’s a glorified cast internally, but it’s guaranteed to give you a readable C string somehow.

Copy link

@apple-fcloutier apple-fcloutier Feb 20, 2025

Choose a reason for hiding this comment

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

From my read, it's not a huge burden to support SEL as a __sized_by(0) pointer (or even as a pointer to an incomplete type); we can have a special diagnostic to direct people to use sel_getName if we fail a cast from SEL to char *.

(We don't have to do it in this PR, but if that sounds like the way to go, we should file a radar for it)

Copy link

Choose a reason for hiding this comment

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

Sounds good. Breaking code that tries to directly cast a selector is fine by me.

Copy link
Author

Choose a reason for hiding this comment

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

not that it'll matter much for a non-dereferenceable pointer, but just to double check: _cmd is never null, right?

Copy link

Choose a reason for hiding this comment

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

It’s unlikely, but possible. You’re allowed to get the IMP of a method and call it with whatever arguments you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants