Re: [RFC PATCH 1/3] static_call: Add static call infrastructure

[Ard Biesheuvel]

On 10 November 2018 at 06:10, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Fri, 9 Nov 2018 14:34:59 -0600
> Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> I'm slowly massaging this to work with tracepoints.
>
> But I hit a snag on this patch.
>
>> On Fri, Nov 09, 2018 at 02:57:46PM -0500, Steven Rostedt wrote:
>> > On Fri, 9 Nov 2018 13:35:05 -0600
>> > Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>> >
>> >
>> > > > > +#define DECLARE_STATIC_CALL(key, func)                                       \
>> > > > > +     extern struct static_call_key key;                              \
>> > > > > +     extern typeof(func) STATIC_CALL_TRAMP(key);                     \
>> > > > > +     /* Preserve the ELF symbol so objtool can access it: */         \
>> > > > > +     __ADDRESSABLE(key)
>> > > >
>> > > > Does the __ADDRESSABLE(key) need to be in the DECLARE part?
>> > > > If so, there needs to be more explanation than just the comment above
>> > > > it.
>> > >
>> > > For each call site, objtool creates a struct in .static_call_sites:
>> > >
>> > >   struct static_call_site {
>> > >           s32 addr;
>> > >           s32 key;
>> > >   };
>> > >
>> > > In order to do that, it needs to create a relocation which references
>> > > the key symbol.  If the key is defined in another .o file, then the
>> > > current .o will not have an ELF symbol associated with the key.  The
>> > > __ADDRESSABLE(key) thing tells GCC to leave the key symbol in the .o
>> > > file, even though it's not referenced anywhere.  That makes objtool's
>> > > job easier, so it doesn't have to edit the symbol table.
>> > >
>> > > I could add a comment saying as much, though it's hard to explain it in
>> > > fewer words than I just did :-)
>> >
>> > Does this have to do with adding the references by relative address?
>> >
>> > In record_mcount, I just picked an existing symbol and referenced that..
>> > But perhaps this is a cleaner way.
>>
>> I think recordmcount is different.  It creates references (in
>> __mcount_loc) to functions which are already in the object file, so they
>> already have symbols associated with them.
>>
>> But in this case, when objtool is creating references, the symbol it
>> needs to reference is outside the .o file, so there's no symbol to
>> associate it with.
>>
>
> The __ADDRESSABLE() appears to fail if you have a header with a
> DECLARE_STATIC_CALL() that is included where the DEFINE_STATIC_CALL()
> is, because I'm getting this:
>
> In file included from <command-line>:
> /work/git/linux-trace.git/include/linux/compiler.h:285:11: error: redefinition of â__addressable___tp_func_sys_enter40â
>    __PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
>            ^~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/compiler_types.h:53:23: note: in definition of macro â___PASTEâ
>  #define ___PASTE(a,b) a##b
>                        ^
> /work/git/linux-trace.git/include/linux/compiler.h:285:3: note: in expansion of macro â__PASTEâ
>    __PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
>    ^~~~~~~
> /work/git/linux-trace.git/include/linux/static_call.h:112:2: note: in expansion of macro â__ADDRESSABLEâ
>   __ADDRESSABLE(key)
>   ^~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/static_call.h:115:2: note: in expansion of macro âDECLARE_STATIC_CALLâ
>   DECLARE_STATIC_CALL(key, _func);    \
>   ^~~~~~~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/tracepoint.h:310:2: note: in expansion of macro âDEFINE_STATIC_CALLâ
>   DEFINE_STATIC_CALL(__tp_func_##name, __tracepoint_iter_##name);
>   ^~~~~~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/trace/define_trace.h:42:2: note: in expansion of macro âDEFINE_TRACE_FNâ
>   DEFINE_TRACE_FN(name, reg, unreg, PARAMS(proto), PARAMS(args))
>   ^~~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/trace/events/syscalls.h:18:1: note: in expansion of macro âTRACE_EVENT_FNâ
>  TRACE_EVENT_FN(sys_enter,
>  ^~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/compiler.h:285:11: note: previous definition of â__addressable___tp_func_sys_enter40â was here
>    __PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
>            ^~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/compiler_types.h:53:23: note: in definition of macro â___PASTEâ
>  #define ___PASTE(a,b) a##b
>                        ^
> /work/git/linux-trace.git/include/linux/compiler.h:285:3: note: in expansion of macro â__PASTEâ
>    __PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
>    ^~~~~~~
> /work/git/linux-trace.git/include/linux/static_call.h:112:2: note: in expansion of macro â__ADDRESSABLEâ
>   __ADDRESSABLE(key)
>   ^~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/tracepoint.h:234:2: note: in expansion of macro âDECLARE_STATIC_CALLâ
>   DECLARE_STATIC_CALL(__tp_func_##name, __tracepoint_iter_##name); \
>   ^~~~~~~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/tracepoint.h:421:2: note: in expansion of macro â__DECLARE_TRACEâ
>   __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),  \
>   ^~~~~~~~~~~~~~~
> /work/git/linux-trace.git/include/linux/tracepoint.h:560:2: note: in expansion of macro âDECLARE_TRACEâ
>   DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
>   ^~~~~~~~~~~~~
> /work/git/linux-trace.git/include/trace/events/syscalls.h:18:1: note: in expansion of macro âTRACE_EVENT_FNâ
>  TRACE_EVENT_FN(sys_enter,
>
> The complaint is on:
>
>         DEFINE_STATIC_CALL(__tp_func_##name, __tracepoint_iter_##name);
>
> And the previous definition is on:
>
>         DECLARE_STATIC_CALL(__tp_func_##name, __tracepoint_iter_##name); \
>

Does the DECLARE really need the __ADDRESSABLE? Its purpose is to
ensure that symbols with static linkage are not optimized away, but
since the reference is from a header file, the symbol should have
external linkage anyway.