r351629 - Emit !callback metadata and introduce the callback attribute

[Doerfert, Johannes Rudolf via cfe-commits]

On 01/22, Chandler Carruth wrote:
> On Sat, Jan 19, 2019 at 2:18 AM Johannes Doerfert via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
> 
> > Author: jdoerfert
> > Date: Fri Jan 18 21:36:54 2019
> > New Revision: 351629
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=351629&view=rev
> > Log:
> > Emit !callback metadata and introduce the callback attribute
> >
> >   With commit r351627, LLVM gained the ability to apply (existing) IPO
> >   optimizations on indirections through callbacks, or transitive calls.
> >   The general idea is that we use an abstraction to hide the middle man
> >   and represent the callback call in the context of the initial caller.
> >   It is described in more detail in the commit message of the LLVM patch
> >   r351627, the llvm::AbstractCallSite class description, and the
> >   language reference section on callback-metadata.
> >
> >   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
> >        generated, 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 detection, e.g.,
> >        pthread_create from the POSIX thread API.
> >     3) For user annotated functions that carry the "callback(callee, ...)"
> >        attribute. The attribute has to include the name, or index, of
> >        the callback callee and how the passed arguments can be
> >        identified (as many as the callback callee has). See the callback
> >        attribute documentation for detailed information.
> >
> > Differential Revision: https://reviews.llvm.org/D55483
> >
> > Added: cfe/trunk/test/Sema/attr-callback-broken.c
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-callback-broken.c?rev=351629&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/Sema/attr-callback-broken.c (added)
> > +++ cfe/trunk/test/Sema/attr-callback-broken.c Fri Jan 18 21:36:54 2019
> > @@ -0,0 +1,75 @@
> > +// RUN: %clang_cc1 %s -verify -fsyntax-only
> > +
> > +__attribute__((callback())) void no_callee(void (*callback)(void)); //
> > expected-error {{'callback' attribute specifies no callback callee}}
> > +
> > +__attribute__((callback(1, 1))) void too_many_args_1(void
> > (*callback)(void)) {}      // expected-error {{'callback' attribute takes
> > one argument}}
> > +__attribute__((callback(1, -1))) void too_many_args_2(double
> > (*callback)(void));     // expected-error {{'callback' attribute takes one
> > argument}}
> > +__attribute__((callback(1, 2, 2))) void too_many_args_3(void
> > (*callback)(int), int); // expected-error {{'callback' attribute requires
> > exactly 2 arguments}}
> > +
> > +__attribute__((callback(1, 2))) void too_few_args_1(void (*callback)(int,
> > int), int); // expected-error {{'callback' attribute takes one argument}}
> > +__attribute__((callback(1))) void too_few_args_2(int (*callback)(int));
> >              // expected-error {{'callback' attribute takes no arguments}}
> > +__attribute__((callback(1, -1))) void too_few_args_3(void
> > (*callback)(int, int)) {}   // expected-error {{'callback' attribute takes
> > one argument}}
> > +
> > +__attribute__((callback(-1))) void oob_args_1(void (*callback)(void));
> >      // expected-error {{'callback' attribute specifies invalid callback
> > callee}}
> > +__attribute__((callback(2))) void oob_args_2(int *(*callback)(void)) {}
> >       // expected-error {{'callback' attribute parameter 1 is out of
> > bounds}}
> > +__attribute__((callback(1, 3))) void oob_args_3(short (*callback)(int),
> > int);  // expected-error {{'callback' attribute parameter 2 is out of
> > bounds}}
> > +__attribute__((callback(-2, 2))) void oob_args_4(void *(*callback)(int),
> > int); // expected-error {{'callback' attribute parameter 1 is out of
> > bounds}}
> > +__attribute__((callback(1, -2))) void oob_args_5(void *(*callback)(int),
> > int); // expected-error {{'callback' attribute parameter 2 is out of
> > bounds}}
> > +__attribute__((callback(1, 2))) void oob_args_6(void *(*callback)(int),
> > ...);  // expected-error {{'callback' attribute parameter 2 is out of
> > bounds}}
> > +
> > +__attribute__((callback(1))) __attribute__((callback(1))) void
> > multiple_cb_1(void (*callback)(void));                           //
> > expected-error {{multiple 'callback' attributes specified}}
> > +__attribute__((callback(1))) __attribute__((callback(2))) void
> > multiple_cb_2(void (*callback1)(void), void (*callback2)(void)); //
> > expected-error {{multiple 'callback' attributes specified}}
> > +
> > +#ifdef HAS_THIS
> > +__attribute__((callback(0))) void oob_args_0(void (*callback)(void)); //
> > expected-error {{'callback' attribute specifies invalid callback callee}}
> > +#else
> > +__attribute__((callback(0))) void oob_args_0(void (*callback)(void));
> >              // expected-error {{'callback' argument at position 1
> > references unavailable implicit 'this'}}
> > +__attribute__((callback(1, 0))) void no_this_1(void *(*callback)(void
> > *));            // expected-error {{'callback' argument at position 2
> > references unavailable implicit 'this'}}
> > +__attribute__((callback(1, 0))) void no_this_2(void *(*callback)(int,
> > void *));       // expected-error {{'callback' argument at position 2
> > references unavailable implicit 'this'}}
> > +#endif
> > +
> > +// We could allow the following declarations if we at some point need to:
> > +
> > +__attribute__((callback(1, -1))) void vararg_cb_1(void (*callback)(int,
> > ...)) {}     // expected-error {{'callback' attribute callee may not be
> > variadic}}
> > +__attribute__((callback(1, 1))) void vararg_cb_2(void (*callback)(int,
> > ...), int a); // expected-error {{'callback' attribute callee may not be
> > variadic}}
> > +
> > +__attribute__((callback(1, -1, 1, 2, 3, 4, -1))) void varargs_1(void
> > (*callback)(int, ...), int a, float b, double c) {}               //
> > expected-error {{'callback' attribute requires exactly 6 arguments}}
> > +__attribute__((callback(1, -1, 4, 2, 3, 4, -1))) void varargs_2(void
> > (*callback)(void *, double, int, ...), int a, float b, double c); //
> > expected-error {{'callback' attribute requires exactly 6 arguments}}
> > +
> > +__attribute__((callback(1, -1, 1))) void self_arg_1(void (*callback)(int,
> > ...)) {}          // expected-error {{'callback' attribute requires exactly
> > 2 arguments}}
> > +__attribute__((callback(1, -1, 1, -1, -1, 1))) void self_arg_2(void
> > (*callback)(int, ...)); // expected-error {{'callback' attribute requires
> > exactly 5 arguments}}
> > +
> > +__attribute__((callback(cb))) void unknown_name1(void (*callback)(void))
> > {}     // expected-error {{'callback' attribute argument 'cb' is not a
> > known function parameter}}
> > +__attribute__((callback(cb, ab))) void unknown_name2(void (*cb)(int), int
> > a) {} // expected-error {{'callback' attribute argument 'ab' is not a known
> > function parameter}}
> > +
> > +__attribute__((callback(callback, 1))) void too_many_args_1b(void
> > (*callback)(void)) {}      // expected-error {{'callback' attribute takes
> > one argument}}
> > +__attribute__((callback(callback, __))) void too_many_args_2b(double
> > (*callback)(void));     // expected-error {{'callback' attribute takes one
> > argument}}
> > +__attribute__((callback(callback, 2, 2))) void too_many_args_3b(void
> > (*callback)(int), int); // expected-error {{'callback' attribute requires
> > exactly 2 arguments}}
> > +
> > +__attribute__((callback(callback, a))) void too_few_args_1b(void
> > (*callback)(int, int), int a); // expected-error {{'callback' attribute
> > takes one argument}}
> > +__attribute__((callback(callback))) void too_few_args_2b(int
> > (*callback)(int));                 // expected-error {{'callback' attribute
> > takes no arguments}}
> > +__attribute__((callback(callback, __))) void too_few_args_3b(void
> > (*callback)(int, int)) {}     // expected-error {{'callback' attribute
> > takes one argument}}
> > +
> > +__attribute__((callback(__))) void oob_args_1b(void (*callback)(void));
> > // expected-error {{'callback' attribute specifies invalid callback callee}}
> > +
> > +__attribute__((callback(callback))) __attribute__((callback(callback)))
> > void multiple_cb_1b(void (*callback)(void));                     //
> > expected-error {{multiple 'callback' attributes specified}}
> > +__attribute__((callback(1))) __attribute__((callback(callback2))) void
> > multiple_cb_2b(void (*callback1)(void), void (*callback2)(void)); //
> > expected-error {{multiple 'callback' attributes specified}}
> > +
> > +#ifdef HAS_THIS
> > +__attribute__((callback(this))) void oob_args_0b(void (*callback)(void));
> > // expected-error {{'callback' attribute specifies invalid callback callee}}
> > +#else
> > +__attribute__((callback(this))) void oob_args_0b(void
> > (*callback)(void));           // expected-error {{'callback' argument at
> > position 1 references unavailable implicit 'this'}}
> > +__attribute__((callback(1, this))) void no_this_1b(void *(*callback)(void
> > *));      // expected-error {{'callback' argument at position 2 references
> > unavailable implicit 'this'}}
> > +__attribute__((callback(1, this))) void no_this_2b(void *(*callback)(int,
> > void *)); // expected-error {{'callback' argument at position 2 references
> > unavailable implicit 'this'}}
> > +#endif
> > +
> > +// We could allow the following declarations if we at some point need to:
> > +
> > +__attribute__((callback(callback, __))) void vararg_cb_1b(void
> > (*callback)(int, ...)) {} // expected-error {{'callback' attribute callee
> > may not be variadic}}
> > +__attribute__((callback(1, a))) void vararg_cb_2b(void (*callback)(int,
> > ...), int a);    // expected-error {{'callback' attribute callee may not be
> > variadic}}
> > +
> > +__attribute__((callback(callback, __, callback, a, b, c, __))) void
> > varargs_1b(void (*callback)(int, ...), int a, float b, double c) {} //
> > expected-error {{'callback' attribute requires exactly 6 arguments}}
> > +__attribute__((callback(1, __, c, a, b, c, -1))) void varargs_2b(void
> > (*callback)(void *, double, int, ...), int a, float b, double c); //
> > expected-error {{'callback' attribute requires exactly 6 arguments}}
> > +
> > +__attribute__((callback(1, __, callback))) void self_arg_1b(void
> > (*callback)(int, ...)) {}                        // expected-error
> > {{'callback' attribute requires exactly 2 arguments}}
> > +__attribute__((callback(callback, __, callback, __, __, callback))) void
> > self_arg_2b(void (*callback)(int, ...)); // expected-error {{'callback'
> > attribute requires exactly 5 arguments}}
> >
> > Added: cfe/trunk/test/SemaCXX/attr-callback-broken.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-callback-broken.cpp?rev=351629&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/SemaCXX/attr-callback-broken.cpp (added)
> > +++ cfe/trunk/test/SemaCXX/attr-callback-broken.cpp Fri Jan 18 21:36:54
> > 2019
> > @@ -0,0 +1,7 @@
> > +// RUN: %clang_cc1 %s -verify -fsyntax-only
> > +
> > +class C_in_class {
> > +#define HAS_THIS
> > +#include "../Sema/attr-callback-broken.c"
> >
> 
> I really don't like this pattern of including from one directory of the
> test suite to the others.

I understand.

> For example, we use a distributed build system and it makes it nearly
> impossible to determine what the inputs are to a test.

Is the problem is that they are in different folders or the fact that
there is an include. We have "system/library" includes already, don't
we?

> But beyond this, I think it would be very confusing in the C-only test case
> to have to be aware of this strange re-use here.

I'm unsure. So far I only encountered the case that broken C attributes
should be broken C++ attributes, with the exception of the "macro case
discussed below".

> Plus, the -verify behavior seems quite surprising here....

What about the verify behavior is surprising to you? I probably have to
fix that even if we get rid of the includes.

> Generally, I would just directly write the test case you want here rather
> than trying to share code.

I understand that on a general level and if that is the consensus I will
manually include the code in the source.

> It seems likely to cause more confusion than the factoring is worth in
> practice. Especially given that you need to use macros to "fix" things
> between the two. Much simpler to just write the code IMO.

I didn't think so when I wrote the tests but again I think this is
subjective. My desire was to make sure I have consistent test coverage
between the languages and also consistent error messages, _except_ in
the one case where I did not want consistent but different ones. That is
where the macro comes in. Now the actual code that is guarded is exactly
the same in the consequence and alternative branch but the expected errors
differ. I think I could even get rid of the macro with an explicit list
of things to check passed to the -verify command. I will look into that
in case we keep the current structure but want to eliminate the macro.


> > +#undef HAS_THIS
> > +};
> >
> > Added: cfe/trunk/test/SemaCXX/attr-callback.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-callback.cpp?rev=351629&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/SemaCXX/attr-callback.cpp (added)
> > +++ cfe/trunk/test/SemaCXX/attr-callback.cpp Fri Jan 18 21:36:54 2019
> > @@ -0,0 +1,67 @@
> > +// RUN: %clang_cc1 %s -verify -fsyntax-only
> > +
> > +// expected-no-diagnostics
> > +
> > +class C_in_class {
> > +#include "../Sema/attr-callback.c"
> >
> 
> Same as above.

Similar, without the macro though.

> > +};
> > +
> > +struct Base {
> > +
> > +  void no_args_1(void (*callback)(void));
> > +  __attribute__((callback(1))) void no_args_2(void (*callback)(void));
> > +  __attribute__((callback(callback))) void no_args_3(void
> > (*callback)(void)) {}
> > +
> > +  __attribute__((callback(1, 0))) virtual void
> > +  this_tr(void (*callback)(Base *));
> > +
> > +  __attribute__((callback(1, this, __, this))) virtual void
> > +  this_unknown_this(void (*callback)(Base *, Base *, Base *));
> > +
> > +  __attribute__((callback(1))) virtual void
> > +  virtual_1(void (*callback)(void));
> > +
> > +  __attribute__((callback(callback))) virtual void
> > +  virtual_2(void (*callback)(void));
> > +
> > +  __attribute__((callback(1))) virtual void
> > +  virtual_3(void (*callback)(void));
> > +};
> > +
> > +__attribute__((callback(1))) void
> > +Base::no_args_1(void (*callback)(void)) {
> > +}
> > +
> > +void Base::no_args_2(void (*callback)(void)) {
> > +}
> > +
> > +struct Derived_1 : public Base {
> > +
> > +  __attribute__((callback(1, 0))) virtual void
> > +  this_tr(void (*callback)(Base *)) override;
> > +
> > +  __attribute__((callback(1))) virtual void
> > +  virtual_1(void (*callback)(void)) override {}
> > +
> > +  virtual void
> > +  virtual_3(void (*callback)(void)) override {}
> > +};
> > +
> > +struct Derived_2 : public Base {
> > +
> > +  __attribute__((callback(callback))) virtual void
> > +  virtual_1(void (*callback)(void)) override;
> > +
> > +  virtual void
> > +  virtual_2(void (*callback)(void)) override;
> > +
> > +  virtual void
> > +  virtual_3(void (*callback)(void)) override;
> > +};
> > +
> > +void Derived_2::virtual_1(void (*callback)(void)) {}
> > +
> > +__attribute__((callback(1))) void
> > +Derived_2::virtual_2(void (*callback)(void)) {}
> > +
> > +void Derived_2::virtual_3(void (*callback)(void)) {}
> >

-- 

Johannes Doerfert
Researcher

Argonne National Laboratory
Lemont, IL 60439, USA

jdoerfert at anl.gov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190122/b17d6443/attachment.sig>