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

[Josh Poimboeuf]

On Sat, Nov 10, 2018 at 08:09:17AM -0500, Steven Rostedt wrote:
> On Sat, 10 Nov 2018 12:58:08 +0100
> Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
> 
> 
> > > 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.

Yes, DECLARE needs the __ADDRESSABLE.  In the case where DECLARE
is used, but DEFINE is not, GCC strips the symbol.

> I applied the following change and it compiled fine:
> 
> diff --git a/include/linux/static_call.h b/include/linux/static_call.h
> index 90b580b95303..5f8a0f0e77be 100644
> --- a/include/linux/static_call.h
> +++ b/include/linux/static_call.h
> @@ -108,8 +108,6 @@ extern void arch_static_call_poison_tramp(unsigned long insn);
>  #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)
>  
>  #define DEFINE_STATIC_CALL(key, _func)					\
>  	DECLARE_STATIC_CALL(key, _func);				\
> @@ -117,7 +115,9 @@ extern void arch_static_call_poison_tramp(unsigned long insn);
>  		.func = _func,						\
>  		.site_mods = LIST_HEAD_INIT(key.site_mods),		\
>  	};								\
> -	ARCH_STATIC_CALL_TEMPORARY_TRAMP(key)
> +	ARCH_STATIC_CALL_TEMPORARY_TRAMP(key);				\
> +	/* Preserve the ELF symbol so objtool can access it: */		\
> +	__ADDRESSABLE(key)
>  
>  #define static_call(key, args...) STATIC_CALL_TRAMP(key)(args)

Adding __ADDRESSABLE to the DEFINE macro doesn't do any good.  By
definition, the key is defined in the .o file, so the symbol already
exists.

The issue you're seeing is really an issue with the __ADDRESSABLE macro
not creating unique symbol names.  This should fix it:

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 06396c1cf127..4bb73fd918b5 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -282,7 +282,7 @@ unsigned long read_word_at_a_time(const void *addr)
  */
 #define __ADDRESSABLE(sym) \
 	static void * __section(".discard.addressable") __used \
-		__PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
+		__UNIQUE_ID(__addressable_##sym) = (void *)&sym;
 
 /**
  * offset_to_ptr - convert a relative memory offset to an absolute pointer