Re: [PATCH V2] livepatch: fix non-static warnings

[Nicholas Mc Guire <der.herr@hofr.at>]

On Mon, Dec 17, 2018 at 10:44:36AM -0500, Joe Lawrence wrote:
> On 12/17/2018 07:03 AM, Miroslav Benes wrote:
> > Hi,
> > 
> > I'm sorry for being late to the party.
> > 
> > On Sun, 16 Dec 2018, Nicholas Mc Guire wrote:
> > 
> >> Sparse reported warnings about non-static symbols. For the variables
> >> a simple static attribute is fine - for those symbols referenced by
> >> livepatch via klp_func the symbol-names must be unmodified in the
> >> symbol table - to resolve this the __noclone attribute is used
> >> for the shared statically declared functions.
> >>
> >> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> >> Suggested-by: Joe Lawrence <joe.lawrence@redhat.com>
> >> Link: https://lkml.org/lkml/2018/12/13/827
> > 
> > A nit, but I'd reorder the tags. Link, Suggested-by:, Signed-off-by:. Also 
> > it would be great if you used https://lkml.kernel.org/r/${Msg-ID} 
> > redirection.
> > 
> >> ---
> >>
> >> V2: not all static functions shared need to carry the __noclone
> >>     attribute only those that need to be resolved at runtime by
> >>     livepatch - so drop the unnecessary __noclone attributes as
> >>     well as the Note on __noclone as suggested by Joe Lawrence
> >>     <joe.lawrence@redhat.com> - thanks !
> > 
> > I talked to Martin Jambor (GCC) and he suggested __attribute__((used)). It 
> > should be better than __noclone, which was reportedly implemented only for 
> > testing purposes (which is why it does not imply noinline, although 
> > inlining internally uses cloning). Newer gcc also has "noipa" attribute, 
> > but "used" would definitely be safe.
> > 
> > Sorry for not responding earlier.
> >
> 
> Hi Miroslav,
> 
> Thanks for following up on this. "noipa" would have been nice to use,
> but as you say, is a newer gcc attribute.
> 
> Regarding "used" vs. "noclone", can we assume that "used" implies that
> the function name remains unchanged?
> 
> The gcc online doc [1] speaks about ensuring that "code must be
> emitted".  This reads like it solves our
> static-function-not-directly-referenced problem, but isn't explicit
> about naming.
> 
>     used
> 
>     This attribute, attached to a function, means that code must be
>     emitted for the function even if it appears that the function is not
>     referenced. This is useful, for example, when the function is
>     referenced only in inline assembly.
> 
> Perhaps it's equivalent to a "I want to keep this function and leave
> it's symbols alone" attribute.  FWIW, I modified Nicholas' change on my
> box to use "used" and it worked as Martin advertised, so it would get my
> Ack.
> 
> I'm just being picky about its documentation and how we should note its
> usage in the v3 patch.  Think that s/__noclone/used/g of the v2 commit
> message would be sufficient?
>
should that then not be  __used  as this is provided in compiler_attributes.h
see also: https://lkml.org/lkml/2018/9/20/909
also would it be reasonable to maybe add something like:
  #define __livepatch         __attribute__((__noclone__, __noinline__))
in compiler_attributes.h ? it would make it imediately clear that the attributes
are related to the way lp works internally.

thx!
hofrat
 
> 
> [1]
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noclone-function-attribute
> 
> 
> Thanks,
> 
> -- Joe