Page MenuHomePhabricator

[ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC
ClosedPublic

Authored by erik.pilkington on Dec 18 2018, 4:32 PM.

Details

Summary

This attribute, called "objc_externally_retained", exposes clang's notion of pseudo-__strong variables in ARC. Pseudo-strong variables "borrow" their initializer, meaning that they don't retain/release it, instead assuming that someone else is keeping their value alive. If a parameter is annotated with this attribute, it isn't implicitly retained/released in the function body, and is implicitly const. This is useful to expose for performance reasons, most functions don't need the extra safety of the retain/release, so programmers can opt-out as needed.

Thanks for taking a look!

rdar://20529015

Diff Detail

Event Timeline

aaron.ballman added inline comments.Dec 19 2018, 6:44 AM
clang/include/clang/AST/Decl.h
1357

Can you update ps to be PS for naming conventions?

clang/include/clang/Basic/DiagnosticSemaKinds.td
3492

This wording isn't quite right -- it doesn't apply to types, it applies to variables of those types. How about: ... to local variables or parameters of strong, retainable object pointer type? or something along those lines?

clang/lib/CodeGen/CGDecl.cpp
2102

method -> Method per naming conventions.

clang/lib/CodeGen/CGObjC.cpp
158 ↗(On Diff #178811)

Good catch, but not really part of this patch -- feel free to commit separately.

clang/lib/Sema/SemaDeclAttr.cpp
6086–6087

I think we should warn that the attribute is ignored in this case (because we're dropping the attribute from things like pretty printing, ast dumps, etc).

Instead of handling this semantically, you can do it declaratively by using the LangOpts field in Attr.td. Something like let LangOpts = [ObjCAutoRefCount];

6117

overrelease -> over-release

Btw, this isn't a bit of a hack, it's a huge hack, IMO. Instead, can we simply err if the user hasn't specified const explicitly? I'm not a fan of "we're hiding this rather important language detail behind an attribute that can be ignored". This is especially worrisome because it means the variable is const only when ARC is enabled, which seems like surprising behavioral differences for that compiler flag.

clang/lib/Sema/SemaExpr.cpp
11227

var->hasAttr<> instead?

erik.pilkington marked 10 inline comments as done.

Address @aaron.ballman comments, rebase onto r349535. Thanks!

clang/include/clang/Basic/DiagnosticSemaKinds.td
3492

Sure, that is a bit more clear.

clang/lib/Sema/SemaDeclAttr.cpp
6086–6087

Sure, good point.

6117

An important part of this feature is that it could applied to parameters with #pragma clang attribute over code that has been audited to be safe. If we require adding const to every parameter, then that falls apart :/.

For better or for worse, this is also consistent with other cases of pseudo-strong in the language, i.e.:

void f(NSArray *A) {
  for (id Elem in A)
    Elem = nil; // fine in -fno-objc-arc, error in -fobjc-arc because Elem is implicitly const.
}
aaron.ballman added inline comments.Dec 19 2018, 12:35 PM
clang/lib/Sema/SemaDeclAttr.cpp
6117

An important part of this feature is that it could applied to parameters with #pragma clang attribute over code that has been audited to be safe. If we require adding const to every parameter, then that falls apart :/.

Thanks for letting me know about that use case; it adds some helpful context. However, I don't see why this falls apart -- if you're auditing the code, you could add the const qualifiers at that time, couldn't you?

Alternatively, if we warned instead of erred on the absence of const, then this could be automated through clang-tidy by checking declarations that are marked with the attribute but are not marked const and use the fix-it machinery to update the code.

For better or for worse, this is also consistent with other cases of pseudo-strong in the language,

Yes, but it's weird behavior for an attribute. The attribute is applied to the *declaration* but then it silently modifies the *type* as well, but only for that one declaration (which is at odds with the usual rules for double-square bracket attributes and what they appertain to based on syntactic location). Sometimes, this will lead to good behavior (such as semantic checks and when doing AST matching over const-qualified types) and sometimes it will lead to confusing behavior (pretty-printing the code is going to stick const qualifers where none are written, diagnostics will talk about const qualifiers that aren't written in the source, etc).

Also, doesn't this cause ABI issues? If you write the attribute on a parameter, the presence or absence of that attribute is now part of the ABI for the function call because the parameter type will be mangled as being const-qualified. (Perhaps that's more of a feature than a bug -- I imagine you want both sides of the ABI to agree whether ARC is enabled or not?)

I'm wondering if this is weird enough that it should be using a keyword spelling instead of an attribute spelling? However, I'm not certain if that works with #pragma clang attribute.

rjmccall added inline comments.Dec 19 2018, 12:45 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3492

I'd split this into two diagnostics:

  • "...can only be applied to local variables and parameters of retainable type" (if the type or decl kind is wrong)
  • "...can only be applied to variables with strong ownership" (if the qualifier is wrong)
clang/lib/CodeGen/CGDecl.cpp
806

EmitARCUnsafeUnretainedScalarExpr doesn't just not retain the value, it actually triggers different handling, e.g. causing non-autoreleased return values to be immediately released. Now I think we arguably want that behavior here, but you should be explicit about it in the comment.

2340

Most of this code is there to support an assertion that's no longer true. Just keep the parts about lt == OCL_Strong and qs.hasConst().

clang/lib/CodeGen/CGExpr.cpp
1946

Where are we allowing assignments into pseudo-strong variables?

erik.pilkington marked 8 inline comments as done.

Address some review comments. Thanks!

clang/include/clang/Basic/DiagnosticSemaKinds.td
3492

Sure, I guess a lot of information is crammed into this one. I coalesced the two you suggested into one with a %select.

clang/lib/CodeGen/CGExpr.cpp
1946

We're not, I removed this.

clang/lib/Sema/SemaDeclAttr.cpp
6117

Thanks for letting me know about that use case; it adds some helpful context. However, I don't see why this falls apart -- if you're auditing the code, you could add the const qualifiers at that time, couldn't you?

If we're going to require O(n) annotations for each application, then there isn't really any reason to use the automatic #pragma annotations. I think it would be nice to be able to throw this around more easily, so you could see what parts of your program are worth auditing, and what stuff breaks. I don't think the const annotation is a dealbreaker for me, but I think it makes this feature easier to use and more consistent.

Yes, but it's weird behavior for an attribute. The attribute is applied to the *declaration* but then it silently modifies the *type* as well, but only for that one declaration (which is at odds with the usual rules for double-square bracket attributes and what they appertain to based on syntactic location). Sometimes, this will lead to good behavior (such as semantic checks and when doing AST matching over const-qualified types) and sometimes it will lead to confusing behavior (pretty-printing the code is going to stick const qualifers where none are written, diagnostics will talk about const qualifiers that aren't written in the source, etc).

Well, we would get the bad diagnostics and stuff about const where none was written with a keyword that implied const as well. I don't think that the standard double square bracket attribute rules really matter here, since we don't really follow them for clang extensions. I guess it boils down to the philosophical question: what should an attribute do? I agree that adding const is a bit strange, but I don't think its beyond the pale.

Also, doesn't this cause ABI issues? If you write the attribute on a parameter, the presence or absence of that attribute is now part of the ABI for the function call because the parameter type will be mangled as being const-qualified. (Perhaps that's more of a feature than a bug -- I imagine you want both sides of the ABI to agree whether ARC is enabled or not?)

Top-level cv qualifiers on parameters are ignored in the mangling, so the parameter isn't mangled as const qualified, I'll add a testcase for this.

I'm wondering if this is weird enough that it should be using a keyword spelling instead of an attribute spelling? However, I'm not certain if that works with #pragma clang attribute.

That's not the end of the world, we could just add a new custom pragma if we went with a keyword. I don't think we want to put this in the type system, since I think it only really makes sense to use this with declarations of parameters and variables. AFAIK an attribute is the only way to uniformly add something to every kind of declaration that we want to support (objc method parameter, variable, parameter), so I think it's the best way to expose this.

aaron.ballman added inline comments.Dec 20 2018, 2:23 PM
clang/lib/Sema/SemaDeclAttr.cpp
6117

Well, we would get the bad diagnostics and stuff about const where none was written with a keyword that implied const as well.

True, but at the same time, the keyword is a stronger signal to the user that this is something special that behaves strangely, too.

I don't think that the standard double square bracket attribute rules really matter here, since we don't really follow them for clang extensions. I guess it boils down to the philosophical question: what should an attribute do? I agree that adding const is a bit strange, but I don't think its beyond the pale.

We typically do follow the appertainment rules for extensions and this feature kind of muddies that because the attribute appertains to both the declaration and the type in a way. But, as you say, this is an extension and we can be weird (on reflection, there is some precedent here too -- calling conventions come to mind as they're sort of type attributes and sort of declaration attributes).

Top-level cv qualifiers on parameters are ignored in the mangling, so the parameter isn't mangled as const qualified, I'll add a testcase for this.

That's a compelling point, thank you for pointing that out!

...so I think it's the best way to expose this.

Okay, I'll hold my nose on the const qualifier then; there's enough reasons to do it, and "ewwwww" isn't a strong enough reason not to do it.

clang/test/SemaObjC/externally-retained.m
3

Can you add a test that runs with ARC disabled to test out the LangOpts behavior from Attr.td? (Can be done in a separate test file if you want, too.)

11

Can you add a test that demonstrates the attribute accepts no args?

15

Should this be useful for function pointer parameters as well? e.g.,

typedef void (*fp)(EXT_RET __strong ObjCTy *);

void f(__strong ObjCTy *);

void g(EXT_RET ObjCTy *Ptr) {
  fp Fn = f; // Good idea? Bad idea?
  Fn(Ptr); // Which behavior "wins" in this call?
}
erik.pilkington marked 5 inline comments as done.

Add some more Sema tests as per Aaron's comments.

clang/test/SemaObjC/externally-retained.m
15

The attribute doesn't have any effect on the caller side, so when used with a function pointer type the attribute doesn't really do anything (the function definition always "wins").

rjmccall added inline comments.Dec 21 2018, 12:52 AM
clang/include/clang/Basic/AttrDocs.td
3783

These function names should be typeset in mono.

3784

Typo

3787

You should add a paragraph contrasting this with `__unsafe_unretained`. For example:

Unlike an `__unsafe_unretained` variable, an externally-retained variable remains
semantically `__strong`.  This affects features like block capture which copy the
current value of the variable into a capture field of the same type: the block's
capture field will still be `__strong`, and so the value will be retained as part of
capturing it and released when the block is destroyed.  It also affects C++ features
such as lambda capture, `decltype`, and template argument deduction.

You should also describe this attribute in AutomaticReferenceCounting.rst.
You can add it in a new arc.misc.externally_retained section immediately above
arc.misc.self. You can borrow some of the existing wording from the two
following sections, arc.misc.self and arc.misc.enumeration, and then make
those sections refer back to the new concept.

3789

The hyphen is unneeded here.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3492

WFM

clang/lib/CodeGen/CGDecl.cpp
807

typo

2334

I wouldn't enumerate the cases in this comment. You might want to enumerate them in a comment on isARCPseudoStrong(), though.

I think Aaron and John have covered any comments I would make! I'm glad this came out so simply, though. I was afraid it'd be a much larger change than just expanding what was already there.

aaron.ballman added inline comments.Dec 21 2018, 11:52 AM
clang/test/SemaObjC/externally-retained.m
15

Perhaps we should warn (and drop the attribute) if you try to write it on a parameter in a function pointer type rather than a function declaration? That way users won't think code like the above does anything useful. Alternatively, we could warn if there was a mismatch between markings (so the attribute is still a noop with function pointer types, but the consistent markings make that benign).

erik.pilkington marked 10 inline comments as done.

Address review comments. Also, make the attribute apply to parameters via the function declaration instead to parameters directly. i.e.:

__attribute__((objc_externally_retained(1)))
void foo(Widget *w) { ... }

The rule being that when this is applied to a non-param variable, it behaves as before, but when applied to a function it takes zero or more indices that correspond to which parameters are pseudo-strong. If no indices are specified, then it applies to every parameter that isn't explicitly __strong. This has a couple of advantages:

  1. It fixes a more "theoretical" ABI break when used with objective-c++ decltype:
void foo(Widget *w, decltype(w) *) {}

Previously, adding an attribute to w would have caused the mangling of foo to change because decltype(w) was Widget * __strong const. Now, we wait until the FunctionType is constructed and the decltype(w) is resolved before tampering with the parameter type, so this gets the same mangling, but we still get the extra safety.

  1. It makes it more obvious what this attribute can/can't be applied to. i.e. it now is impossible to annotate parameters of function types, which doesn't make any sense.
  1. We don't have to add special-case hacks to #pragma clang attribute to make this work. i.e.:
#pragma clang attribute push (__attribute__((objc_externally_retained)), apply_to=any(function,objc_method,block))
void f(Widget *w, __strong Widget *other) {} // w is externally-retained, other is not
#pragma clang attribute pop

Whereas previously we would have had to add something like apply_to=is_objc_retainable_pointer_parameter_of_function_definition_that_is_implicitly_strong in order to get these semantics.

Thanks for taking a look!

clang/include/clang/Basic/AttrDocs.td
3787

Sure, I pretty much just copied that in verbatim. New patch updates ARC.rst as well.

clang/test/SemaObjC/externally-retained.m
15

In the new patch, applying the attribute to a parameter in a function pointer type is impossible.

rjmccall added inline comments.Jan 2 2019, 10:20 PM
clang/docs/AutomaticReferenceCounting.rst
1748

I think "can apply to parameters and local variables" is probably the clearest way to express this.

I would suggest turning this around a bit. Don't name the section after the attribute; instead,
start by talking about externally-retained variables as a general concept, then begin a new
paragraph to discuss how they arise: (1) certain implicit places which will be discussed below
and (2) this attribute, which you can then discuss. It will then be natural to add a third
paragraph to talk about the pragma or whatever it ends up being.

The first paragraph should talk about things like capture semantics like the attribute docs do.

clang/include/clang/Basic/AttrDocs.td
3811

Worth adding after this sentence: "(Note that the block capture field is not considered
externally retained.)"

erik.pilkington marked 4 inline comments as done.

Address John's comments. Thanks!

rjmccall added inline comments.Jan 3 2019, 1:44 PM
clang/docs/AutomaticReferenceCounting.rst
1749

Nit-picks: it's should be its, An shouldn't be capitalized, and the comma before "and release it" is a splice.

1779

I hadn't noticed the parameter-index feature. Is that a good idea? Surely if this level of control is needed, it would be better to add the attribute to each parameter individually — or still better, just use explicit ownership qualifiers to mark the exceptions. And it creates all sorts of ugly questions about the indexing base and how it accounts for implicit parameters.

erik.pilkington marked an inline comment as done.Jan 3 2019, 1:52 PM
erik.pilkington added inline comments.
clang/docs/AutomaticReferenceCounting.rst
1779

Oh, good point. I never even considered just opting out with __strong instead of indices. That makes a lot more sense and has better ergonomics. I'll remove that feature and update the docs...

FWIW: I explained why I went with the attribute on the FunctionDecl instead of the ParmVarDecl a few comments up if you missed it.

erik.pilkington marked an inline comment as done.

Remove support for parameter indices.

Sorry, I keep coming up with small tweaks to the documentation. These should be the last of them, so if Aaron's also satisfied, feel free to commit with those modifications.

clang/docs/AutomaticReferenceCounting.rst
1788

Consider adding a paragraph describing how to test for support for this attribute.

clang/include/clang/Basic/AttrDocs.td
3801

Please add a paragraph saying that this attribute has no effect when ARC isn't enabled.

erik.pilkington marked 3 inline comments as done.

Mention __has_attribute and the ignored without -fobjc-arc thing. @aaron.ballman: Do you have any more thoughts here?

aaron.ballman accepted this revision.Jan 4 2019, 9:57 AM

LGTM aside from a few minor nits/questions.

clang/docs/AutomaticReferenceCounting.rst
1747

deinitialization -> destruction ?

clang/lib/Sema/SemaDeclAttr.cpp
6160

The const_cast<> here feels a bit nasty, but I'm not certain it's worth an overload of getFunctionOrMethodParam() to hide it away. Your call on whether you want to add the overload or not.

This revision is now accepted and ready to land.Jan 4 2019, 9:57 AM
This revision was automatically updated to reflect the committed changes.