With commit rXXXXX (currently https://reviews.llvm.org/D54498), LLVM gained the ability to apply existing optimizations on indirections through callbacks. This is based on an abstraction that hides the middle man as described in rXXXXX and the llvm::AbstractCallSite class. This commit enables clang to emit !callback metadata that is understood by LLVM. It does so in three different cases: 1) For known broker functions declarations that are directly emitted, e.g., __kmpc_fork_call for the OpenMP pragma parallel. 2) For known broker functions that are identified by their name and source location through the builtin mechanism, e.g., pthread_create from the POSIX thread API. 3) For user annotated functions that carry the "callback(idx, ...)" attribute. The attribute has to include the index of the callback callee and how the passed arguments can be identified (as many as the callback callee has). For additional information, also consider the commit message and discussion for the LLVM patch: https://reviews.llvm.org/D54498
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
This is missing all of the Sema and SemaCXX tests. Should have tests for member functions, variadic functions, incorrect arguments, incorrect subjects, etc.
include/clang/Basic/Attr.td | ||
---|---|---|
1205 ↗ | (On Diff #177404) | No new undocumented attributes, please. |
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) | 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. |
I will write more tests and update this revision.
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) |
I don't need it to and unless somebody does, I'd say no.
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:
|
1205 ↗ | (On Diff #177404) | Ok, I can write documentation similar to the commit message and the lang-ref documentation for the callback metadata. |
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) |
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.
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)). |
include/clang/Basic/AttrDocs.td | ||
3711 ↗ | (On Diff #177662) | I'd remove callee here in both places. |
3714 ↗ | (On Diff #177662) | and the following indicies are the forwarded arguments. |
3716 ↗ | (On Diff #177662) | The index '0' is used to represent a callback callee argument which is not present in the declared parameter list. |
3718 ↗ | (On Diff #177662) | attributes -> attribute |
3719 ↗ | (On Diff #177662) | , allow to make the connection -> , make the connection |
3721 ↗ | (On Diff #177662) | Switch to consistently using a single space after the period (as was done in the previous paragraph). |
3723 ↗ | (On Diff #177662) | undefinied -> undefined |
3724 ↗ | (On Diff #177662) | else than then actual callback -> anything other than the actual callback. Thus, inspected -> Inspected, |
3725 ↗ | (On Diff #177662) | shall may be mentioned -> are listed |
3730–3731 ↗ | (On Diff #177662) | 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. |
Thanks for the feedback.
Once we decided on the style of the annotation, I will implement that and change the tests/documentation accordingly.
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) |
We have a test case and we'll spit out an error. (Sema/attr-callback-broken.c line 21 & 22)
I can change it to the inlined style if nobody disagrees: void broker(int foo, void (*callback)(int, int, int, int) __attribute__((callback(foo, ?, bar, ?))), int bar); As I mentioned, I don't have a strong opinion on this but I just don't want to change it back and forth :) |
include/clang/Basic/Builtins.def | ||
---|---|---|
942 ↗ | (On Diff #177771) | qsort_r callback argument order is different on Linux, macOS and FreeBSD so those constants can't be hardcoded: |
test/CodeGen/callback_qsort_r.c | ||
1 ↗ | (On Diff #177771) | This should use a linux triple otherwise the qsort_r declaration is wrong. Ideally it should also handle macos+FreeBSD with the inverted argument order. |
include/clang/Basic/Builtins.def | ||
---|---|---|
942 ↗ | (On Diff #177771) | Really good point. I'll remove qsort_r in the next version I guess. If we find another similar function we can extend the encoding to sth like: C:(<3,-1,-1,4>|<5,4,-1,-1>) which represents both possibilities. The Builtin::Context::performsCallback(...) will then need to check which is appropriate based on the type of the actual declaration. |
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) | You can now use parameter names or indices in the callback attribute. The special identifiers "" (double underscore) and "this" can be used to specify an unknown (-1 in the index notation) and the implicit "this" (0 in the index notation) argument. |
test/CodeGen/callback_qsort_r.c | ||
1 ↗ | (On Diff #177771) | I postponed qsort_r until we have another similar function or somebody want to implement the necessary functionality. |
include/clang/Basic/Attr.td | ||
---|---|---|
1204 ↗ | (On Diff #177404) | Nice! I like that approach. |
include/clang/Basic/AttrDocs.td | ||
3778 ↗ | (On Diff #180971) | I'd use backticks around 0 and this rather than single and double quotes, to make it clear that these are syntax and not prose. |
3782–3783 ↗ | (On Diff #180971) | Grammatical issue here -- I think it should probably say "or one that is, but is potentially..." |
3786 ↗ | (On Diff #180971) | are -> is |
include/clang/Basic/DiagnosticSemaKinds.td | ||
2578 ↗ | (On Diff #180971) | I think this should say 'callback' attribute (here and in the other diagnostics); WDYT? |
2579–2580 ↗ | (On Diff #180971) | This is not needed, see err_attribute_argument_out_of_bounds. |
2586 ↗ | (On Diff #180971) | I'd reword this to use "may not" instead of "shall not". |
2589–2592 ↗ | (On Diff #180971) | These are not needed, see err_attribute_too_many_arguments and err_attribute_too_few_arguments (or err_attribute_wrong_number_arguments). |
lib/Sema/SemaDecl.cpp | ||
13587–13588 ↗ | (On Diff #180971) | You can combine these predicates into one if statement. |
lib/Sema/SemaDeclAttr.cpp | ||
3481 ↗ | (On Diff #180971) | This doesn't match the documentation -- I assume you switched from ? to __ because __ at least parses as a valid identifier, whereas ? would require extra parsing support? If so, that's fine by me. |
3483 ↗ | (On Diff #180971) | This doesn't match the documentation either, but I'm less clear on why the double underscores are used. |
3492 ↗ | (On Diff #180971) | Identifiers don't match the usual naming conventions. Prefer ++U as well. |
3493 ↗ | (On Diff #180971) | Spurious newline |
3502 ↗ | (On Diff #180971) | You can pass in IdLoc->Ident and the diagnostic engine should properly print and quote it for you. |
3508 ↗ | (On Diff #180971) | Spurious newline. |
3528 ↗ | (On Diff #180971) | Newline |
test/CodeGen/callback_annotated.c | ||
18–19 ↗ | (On Diff #180971) | Can you add a test like this: void* broker2(void *(*callee)(void)); void* test(void) { return broker2(test); } __attribute__((callback (callee))) void* broker2(void *(*callee)(void)); Basically, I'm trying to see whether we properly merge the declarations and note that the call to broker2() within test() really does have the callback attribute. |
test/Sema/attr-callback-broken.c | ||
5 ↗ | (On Diff #180971) | Please run the tests through clang-format. |
Updated according to your comments.
lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3481 ↗ | (On Diff #180971) | Yes, __, and __this where chosen because they work without lexer/parser changes and are in the implementation namespace. I forgot to update the documentation though. Will be fixed. |
3483 ↗ | (On Diff #180971) | If you use this, the lexer will generate the special "this" token. That one is checked explicitly to be only used inside of non-static class methods. If you have an idea how to avoid this check or make it consider uses in the attribute as OK, please let me know. |
3492 ↗ | (On Diff #180971) | OK.
Out of curiosity, why? |
3493 ↗ | (On Diff #180971) | That was intentional but OK. |
include/clang/Basic/AttrDocs.td | ||
---|---|---|
3780 ↗ | (On Diff #181420) | The '?' here needs to change to be '__' instead. |
include/clang/Basic/Builtins.def | ||
96 ↗ | (On Diff #181420) | Wrap to 80-col |
lib/Sema/SemaDeclAttr.cpp | ||
3483 ↗ | (On Diff #180971) | Why do you want to avoid that check? It seems like that's a very good check to have -- what circumstances would you expect a user to use this in a something other than a non-static member function? |
3492 ↗ | (On Diff #180971) | Because the previous value of U is not needed (you're executing the instruction only for its side effects, not its value). |
LGTM aside from some minor nits.
However, please hold off on committing this until *after* the 8.0 branch happens. While I don't expect there to be issues with this patch, it does introduce a pretty novel new way to interact with attributes and I want to ensure that the community has the opportunity to react to any major issues with this approach, and I'm not certain a month or so will be enough time for that. If you need this patch to go in *before* the branch for some reason, please mention it! I'm not strictly opposed to it going in sooner rather than later, but if there's not a pressing need for the functionality, I'd prefer to wait out of an abundance of caution.
include/clang/Basic/AttrDocs.td | ||
---|---|---|
3778 ↗ | (On Diff #181631) | __this should be this now. |
utils/TableGen/ClangAttrEmitter.cpp | ||
2169 ↗ | (On Diff #181631) | const Record * |
2177–2178 ↗ | (On Diff #181631) | The formatting looks off here. |