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

[Miroslav Benes]

On Mon, 17 Dec 2018, 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@xxxxxxxxx>
> >> Suggested-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx>
> >> 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@xxxxxxxxxx> - 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?

I am not sure. I'd argue that it does imply that, but it could just be 
a consequence without any guarantees. My understanding is that gcc cannot 
assume about a symbol and its references. So it should be preserved as is.
 
> 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.

Correct.

>     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?

We could rephrase it. After all it is not only about symbol names in the 
symbol table. The traceable/patchable code has to be present...

"Sparse reported warnings about non-static symbols. For the variables
a simple static attribute is fine - for the functions referenced by
livepatch via klp_func the symbol-names must be unmodified in the
symbol table and the patchable code has to be emitted.

Attach __used attribute to the shared statically declared functions."

?

Miroslav