Page MenuHomePhabricator

[attributes] Extend os_returns_(not_?)_retained attributes to parameters.
ClosedPublic

Authored by george.karpenkov on Thu, Jan 3, 2:43 PM.

Details

Summary

When applied to out-parameters, the attributes specify the expected lifetime of the written-into object.

Additionally, introduce OSReturnsRetainedOn(Non)Zero attributes, which specify that an ownership transfer happens depending on a return code.

Diff Detail

Event Timeline

aaron.ballman added inline comments.Sat, Jan 5, 10:26 AM
clang/include/clang/Basic/Attr.td
844

The documentation should be updated to mention this change.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3429

Do we have no semantic tests that verified the pointer-to-CF-pointer case was actually a pointer to a CF pointer?

clang/test/Sema/attr-osobject.cpp
52

This is not a pointer-to-pointer parameter. I assume the behavior here is correct, but it suggests that in C++ mode the diagnostic should say "pointer- or reference-to-pointer", perhaps. This should also be called out in the docs, I think.

60

Can you also add some test cases for qualified pointers? e.g., should this work?

void write_into_out_parameter_not_retained(__attribute__((os_returns_not_retained)) S volatile * restrict * const out) {}
george.karpenkov marked 6 inline comments as done.
george.karpenkov retitled this revision from [attributes] Extend os_returns_(not_?)_retained attributes to parameters to [attributes] Extend os_returns_(not_?)_retained attributes to parameters..
george.karpenkov edited the summary of this revision. (Show Details)

Addressed comments, added conditional out parameters.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3429

Fair point.
We do have such a test. I was too lazy too add another case to diagnostics, as it's a bit of extra code. But maybe we should do that.

clang/test/Sema/attr-osobject.cpp
52

Sorry I don't quite agree here: while it's technically not pointer to pointer, the semantics is nearly identical (save for null parameters).

I think it's good to be flexible enough to accept those, without unnecessarily complicating the documentation or diagnostics.

60

I don't know what "restrict" is, and it does not compile, but added others.

aaron.ballman added inline comments.Tue, Jan 8, 6:20 AM
clang/include/clang/Basic/AttrDocs.td
899 ↗(On Diff #180590)

I'd spell out "iff" if you really meant "if and only if".

902 ↗(On Diff #180590)

non-retained objects

clang/include/clang/Basic/DiagnosticSemaKinds.td
3429

Should it be pointer-to-OS-pointer, or perhaps pointer-to-CFObject-pointer? Or do we not need to be consistent with nomenclature that way?

clang/lib/Sema/SemaDeclAttr.cpp
429–430

Comments can be re-flowed.

434

I think the function name is a bit of a misnomer now because there's no real check being performed within the call -- the caller does the checking. Perhaps handleSimpleAttributeWithDiagnostic() instead, and the parameter can be named ShouldDiagnose?

4920

Comment is incorrect here.

clang/test/Sema/attr-osobject.cpp
52

I think it's good to be flexible enough to accept those, without unnecessarily complicating the documentation or diagnostics.

I agree that it seems reasonable to accept references because the semantics are so similar to those of pointers, but I disagree that being precise with the documentation and diagnostics is unnecessary complication. When the diagnostic says "this attribute only applies to", users may reasonably expect that "only" is meaningful and miss out on the fact that there's an even better, supported construct for them to use when C++ language features are enabled.

I thought of one more test case we don't have coverage for (it's not super specific to your patch, but you do touch on it). Can you add:

bool two_stars_good_three_stars_bad(__attribute__((os_returns_retained_on_zero)) S*** out) {}

I assume that should diagnose because it's a pointer to a pointer to a pointer to an OS object.

60

It's a C99 qualifier: https://godbolt.org/z/jz2im5, but it's not terribly important for the test either. That said, I don't see the new test case added?

george.karpenkov marked 9 inline comments as done.
george.karpenkov added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3429

No, here it does not need to be consistent: CF is an accepted unambiguous abbreviation if you are interacting with Apple frameworks, and OS is not.

clang/lib/Sema/SemaDeclAttr.cpp
434

I agree that the name is not perfect, but I think handleSimpleAttributeWithDiagnostic
with ShouldDiagnose is worse, because (to me) it implies additive action: we handle the attribute, and then we potentially emit diagnostics.
Whereas the actual semantics is: we handle the attribute if it passes the test,
and emit the diagnostics otherwise. But encoding that seems to verbose.

clang/test/Sema/attr-osobject.cpp
52

I assume that should diagnose because it's a pointer to a pointer to a pointer to an OS object.

Actually we don't diagnose it, but perhaps you are right and we should.

miss out on the fact that there's an even better, supported construct for them to use when C++ language features are enabled.

OK. It does get more verbose though, which IMO can be bad for diagnostics (e.g. in my setup clangd shows warnings and errors in the vim statusbar, and it gets cut if it does not fit. I imagine many are like that)

60

Ooops, sorry, must have lost it.

@aaron.ballman I'm half-tempted to rewrite most of attribute handling to just have a single attribute class with multiple spelling, and then I would be able to differentiate between them without meta-programming template trickery.
That poses the problem though: dumping both parameter and function/method (actually, the latter two together are ugly already) into the same attribute class would make the code quite a bit less declarative, and would require writing quite a bit of imperative logic to distinguish between the two.
Dumping them into two buckets (for parameters and for callables) would have a problem of not having a nice way to share an enum between the two (c.f Ownership::OwnershipKind in Attr.td. Or maybe it's OK for one attribute to refer to an enum inside the other?)

What do you think?

OTOH using a single attribute class for all attributes makes usage from Sema very unwieldy,
as then it would not be possible to write D->hasAttr<BlahAttr> anymore =/

aaron.ballman accepted this revision.Fri, Jan 11, 5:35 AM

LGTM aside from some minor nits.

clang/lib/Sema/SemaDeclAttr.cpp
434

How about handleSimpleAttributeOrDiagnose()?

4860–4861

Comments can be re-flowed to 80 col.

4863

const auto *

clang/test/Sema/attr-osobject.cpp
52

OK. It does get more verbose though, which IMO can be bad for diagnostics (e.g. in my setup clangd shows warnings and errors in the vim statusbar, and it gets cut if it does not fit. I imagine many are like that)

So long as the diagnostic reasonably continues to fit in a single sentence fragment, I think it's "short enough" -- I don't think we should leave useful information out of diagnostics because some editors have poor Ux for displaying longer diagnostic messages. I don't think we've reached that tipping point here yet, so thank you for the change!

This revision is now accepted and ready to land.Fri, Jan 11, 5:35 AM
george.karpenkov marked 3 inline comments as done.Fri, Jan 11, 9:56 AM
This revision was automatically updated to reflect the committed changes.