Page MenuHomePhabricator

[Clang] Add support for attribute 'escape'
AcceptedPublic

Authored by guitard0g on Jul 28 2021, 8:55 PM.

Details

Summary

The 'escape' attribute indicates that the annotated pointer parameter
may escape the scope of the function. This attribute is meant to be
used for compiler diagnostics/static analysis checks. The attribute
will first be used by the Swift compiler in a new implicit bridging
diagnostic, but may have other non-Swift use-cases for diagnostics.

Diff Detail

Event Timeline

guitard0g requested review of this revision.Jul 28 2021, 8:55 PM
guitard0g created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2021, 8:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ added a comment.Jul 28 2021, 9:08 PM

may have other non-Swift use-cases for diagnostics.

I'm looking forward to taking advantage of this attribute in the static analyzer's StackAddrEscapeChecker!

clang/include/clang/Basic/Attr.td
1952

Shouldn't both this attribute and the one above be InheritableAttr?

Comments in this file say:

/// An inheritable attribute is inherited by later redeclarations.

Which is arguably exactly what we want? Like, it should be sufficient to put the attribute into the header, there's no need to duplicate it in the implementation?

clang/include/clang/Basic/AttrDocs.td
260

Are you sure you want "can escape" instead of "will escape"? In the former case it's impossible to implement the warning without false positives ("well, it says it may escape, but I know for sure that if I pass these other arguments then it won't escape"). In the latter case, of course, a lot of places where the value escapes conditionally won't be able to wear the attribute. Do I understand correctly that you want to proceed with the more aggressive diagnostic?

guitard0g added inline comments.Jul 28 2021, 9:38 PM
clang/include/clang/Basic/AttrDocs.td
260

Hmm that's an interesting question. Initially my thought was that any possibility of escaping should eliminate the option of passing in a temporary pointer, which I think is reasonable in Swift's implicit bridging use-case. In terms of the API contract, I was thinking that any API author who annotates their function parameter with 'escaping' is effectively saying you shouldn't pass in a pointer that you're not comfortable escaping. I think it may be a little restrictive to say that the pointer will always escape for certain, but I do think that anybody using a function that takes an escaping parameter should treat this as if it were always escaping (if that makes sense). I suppose my question then would be, do you think it's better to say "will escape" if that's what the client calling into an API should expect (even though the pointer might not always escape in the implementation)?

Change Escape/NoEscape to use InheritableAttr and update an attribute test to fix test failure.

guitard0g marked an inline comment as done.Jul 28 2021, 10:39 PM
guitard0g added inline comments.
clang/include/clang/Basic/Attr.td
1952

Oh! Good point, I totally agree.

vsavchenko added a comment.EditedJul 29 2021, 4:03 AM

Great job! It looks good, but I have a couple of minor tweaks to suggest.

I see that the "applies to pointer arguments only" warning is not tested for noescape, but I still find it to be a good practice to write a test with a bunch of cases with attributes applied in wrong places.

Additionally, I believe that escape and noescape convey the same information, and it is totally okay to have two attributes because the user has to have a choice to say either "I do escape it" or "I definitely do not escape it" when the default is unspecified at all. But I do think that we should add a check disallowing both of these attributes to be used at the same time for the same parameter, since they directly contradict one another.

guitard0g marked an inline comment as done.

Add tests for diagnostics of incorrect usage. Diagnose when escape and noescape are used on the same parameter.

@vsavchenko

I see that the "applies to pointer arguments only" warning is not tested for noescape, but I still find it to be a good practice to write a test with a bunch of cases with attributes applied in wrong places.

Updated with some tests for this!

Additionally, I believe that escape and noescape convey the same information, and it is totally okay to have two attributes because the user has to have a choice to say either "I do escape it" or "I definitely do not escape it" when the default is unspecified at all. But I do think that we should add a check disallowing both of these attributes to be used at the same time for the same parameter, since they directly contradict one another.

Ah that's a great point, most recent update makes these two mutually exclusive.

vsavchenko accepted this revision.Jul 29 2021, 1:55 PM

Awesome, I have nothing to add at this point!
Let's still wait for @aaron.ballman to check it.

This revision is now accepted and ready to land.Jul 29 2021, 1:55 PM

The attribute will first be used by the Swift compiler in a new implicit bridging diagnostic, but may have other non-Swift use-cases for diagnostics.

This means there's no use of this attribute within Clang which suggests to me that this attribute belongs in the Swift compiler until there's a reason to upstream it. Or are you planning on using this attribute within Clang in the Very Near Future?

clang/include/clang/Basic/Attr.td
1957

You should define these attributes as being mutually exclusive; e.g.,

def : MutualExclusions<[Escape, NoEscape]>;
clang/include/clang/Basic/AttrDocs.td
260

`escape` placed on a function parameter of a pointer type is used to indicate that the pointer can escape the function.

FWIW, this reads a bit strangely to me because that's already the case without the attribute. Anything that's not marked *can* escape. I think "will escape" more clearly identifies to the person reading the function signature that the parameter should be treated as though it will escape. If it does not escape, that's not necessarily an issue because that shouldn't change how the user treats the argument (for example, it may not escape today but it may start to escape in the future).

One thing the docs are missing is an explanation of what benefit you get from using this annotation. Does this improve optimizations? Enable better checking somewhere? That sort of thing (with examples) would be good to add.

clang/lib/Sema/SemaDeclAttr.cpp
1568

This should be handled directly from Attr.td, I'll put some comments in there.

clang/test/SemaObjCXX/escape.mm
1–2

I don't see any use of blocks, so I don't think you need -fblocks. I think you can also remove the C++11 RUN line as it doesn't really add any test coverage.

24

Can you also add test coverage for the more likely case of inheriting mutually exclusive attributes? e.g.,

void foo() __attribute__((escape));
void foo() __attribute__((noescape));

void bar() __attribute__((noescape));
void bar() __attribute__((escape));
NoQ added inline comments.Jul 30 2021, 2:18 PM
clang/include/clang/Basic/AttrDocs.td
260

If I understand correctly, the new attribute proclaims "Do not, under any circumstances, pass stack pointers here". Like, it doesn't necessarily escape every time, but you should expect the function to be able to stash that pointer somewhere without telling you, for indefinite amount of time, so if you put your temporaries into this function you're 100% doing something wrong and you want to be notified about this.

This take probably raises more questions though. Like, if the pointer is a heap pointer but we see a nearby delete of that pointer with our own eyes, do we warn too? Where exactly is the boundary between short-lived and long-lived objects? (IIUC the answer is yes, we do want to warn, because the function stashes the pointer for *indefinite* amount of time so it should be the judge for when to delete it, not you).

I think there could be a simple compiler warning associated with that attribute which would pattern-match the argument to see if it's an immediate stack address such as &Class() or &Var where Var is a local variable. A static analyzer check for the more sophisticated cases works too of course.