[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 11 12:36:41 PST 2018


aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1204
+              VariadicUnsignedArgument<"PayloadIndices">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > Should this also apply to Objective-C methods?
> > 
> > Why should the user specify this attribute on the function as opposed to on the parameter? e.g.,
> > ```
> > // Why this:
> > __attribute__((callback (1, 2, 3)))
> > void* broker0(void* (*callee)(void *), void *payload, int otherPayload) {
> >   return callee(payload);
> > }
> > 
> > // Instead of this:
> > void* broker0(void* (*callee)(void *) __attribute__((callback (2, 3))), void *payload, int otherPayload) {
> >   return callee(payload);
> > }
> > 
> > // Or this:
> > void* broker0(void* (*callee)(void *) __attribute__((callback (payload, otherPayload))), void *payload, int otherPayload) {
> >   return callee(payload);
> > }
> > ```
> > I ask because these "use an index" attributes are really hard for users to use in practice. They have to account for 0-vs-1 based indexing, implicit this parameters, etc and if we can avoid that, it may be worth the effort.
> > Should this also apply to Objective-C methods?
> 
> I don't need it to and unless somebody does, I'd say no.
> 
> 
> > I ask because these "use an index" attributes are really hard for users to use in practice. They have to account for 0-vs-1 based indexing, implicit this parameters, etc and if we can avoid that, it may be worth the effort.
> 
> I was thinking that the function notation makes it clear that there is *only one callback per function* allowed right now. I don't expect many manual users of this feature until we improve the middle-end support, so it is unclear to me if this requirement needs to be removed as well.
> 
> Other than that, some thoughts: 
> - I do not feel strongly about this.
> - The middle requirement seems not much better n the first, we would still need to deal with index numbers (callbacks without arguments are not really interesting for now). 
> - The last encoding requires us to define a symbol for "unknown argument" (maybe _ or ?).
> I was thinking that the function notation makes it clear that there is *only one callback per function* allowed right now.

I don't see how that follows. Users may still try writing:
```
__attribute__((callback (1, 3, 4)))
__attribute__((callback (2, 3, 4)))
void broker0(void (*cb1)(void *, int), void (*cb2)(void *, int), void *payload, int otherPayload) {
  cb1(payload, otherPayload);
  cb2(payload, otherPayload);
}
```
and reasonably expect that to work (we should have this as a test case, and probably warn on it).

I'm not strongly opposed to the current way this is exposed to users, just wondering if we can find a better way to surface the feature.

> The last encoding requires us to define a symbol for "unknown argument" (maybe _ or ?).

Ah, I wasn't aware that this was part of the feature, but the documentation you wrote helped to clarify for me. Personal preference is for `?`, but any symbol will do (so long as we aren't hoping users can count commas, e.g., `callback(,,,,frobble,,,foo)`).


================
Comment at: include/clang/Basic/AttrDocs.td:3711
+The ``callback`` attribute specifies that the annotated function may invoke the
+specified callback callee zero or more times. The callback callee, as well as
+the passed arguments, are identified by their parameter position (starting with
----------------
I'd remove `callee` here in both places.


================
Comment at: include/clang/Basic/AttrDocs.td:3714
+1!) in the annotated function. The first position identifies the callback callee,
+the following indices the forwarded arguments. The callback callee is required
+to be callable with the number, and order, of the specified arguments. To
----------------
and the following indicies are the forwarded arguments.


================
Comment at: include/clang/Basic/AttrDocs.td:3716
+to be callable with the number, and order, of the specified arguments. To
+represent unknown arguments a zero shall be used.
+
----------------
The index '0' is used to represent a callback callee argument which is not present in the declared parameter list.


================
Comment at: include/clang/Basic/AttrDocs.td:3718
+
+The ``callback`` attributes, which are directly translated to ``callback``
+metadata <http://llvm.org/docs/LangRef.html#callback-metadata>, allow to make
----------------
attributes -> attribute


================
Comment at: include/clang/Basic/AttrDocs.td:3719
+The ``callback`` attributes, which are directly translated to ``callback``
+metadata <http://llvm.org/docs/LangRef.html#callback-metadata>, allow to make
+the connection between the call to the annotated function and the callback
----------------
, allow to make the connection -> , make the connection


================
Comment at: include/clang/Basic/AttrDocs.td:3721
+the connection between the call to the annotated function and the callback
+callee.  This can enable interprocedural optimizations which were otherwise
+impossible.  If a function parameter is mentioned in the ``callback``
----------------
Switch to consistently using a single space after the period (as was done in the previous paragraph).


================
Comment at: include/clang/Basic/AttrDocs.td:3723
+impossible.  If a function parameter is mentioned in the ``callback``
+attribute, through its position, it is undefinied if that parameter is used for
+anything else than then actual callback. Thus, inspected, captured, or modified
----------------
undefinied -> undefined


================
Comment at: include/clang/Basic/AttrDocs.td:3724
+attribute, through its position, it is undefinied if that parameter is used for
+anything else than then actual callback. Thus, inspected, captured, or modified
+parameters shall may be mentioned in the ``callback`` metadata.
----------------
else than then actual callback -> anything other than the actual callback.

Thus, inspected -> Inspected, 


================
Comment at: include/clang/Basic/AttrDocs.td:3725
+anything else than then actual callback. Thus, inspected, captured, or modified
+parameters shall may be mentioned in the ``callback`` metadata.
+
----------------
shall may be mentioned -> are listed


================
Comment at: include/clang/Basic/AttrDocs.td:3730-3731
+(`start_routine`) is called zero or more times by the `pthread_create` function,
+and that the fourth parameter (`arg`) is passed along. Note that this case is
+actually automatically recognized.
+
----------------
You should mention the circumstances under which we automatically recognize a callback so that users know when they're required to write the annotation vs allowing the compiler to infer it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55483/new/

https://reviews.llvm.org/D55483





More information about the cfe-commits mailing list