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