[PATCH V2] livepatch: fix non-static warnings

[PATCH V2] livepatch: fix non-static warnings

[Nicholas Mc Guire]

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

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 !

Sparse reported the following findings:

CHECK   samples/livepatch/livepatch-shadow-mod.c
samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
 'dummy_list' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
 'dummy_list_mutex' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
 'dummy_alloc' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:128:15: warning: symbol
 'dummy_free' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:136:15: warning: symbol
 'dummy_check' was not declared. Should it be static?

CHECK   samples/livepatch/livepatch-shadow-fix1.c
samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
 'livepatch_fix1_dummy_alloc' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-fix1.c:111:6: warning: symbol
 'livepatch_fix1_dummy_free' was not declared. Should it be static?

CHECK   samples/livepatch/livepatch-shadow-fix2.c
samples/livepatch/livepatch-shadow-fix2.c:53:6: warning: symbol
 'livepatch_fix2_dummy_check' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-fix2.c:81:6: warning: symbol
 'livepatch_fix2_dummy_free' was not declared. Should it be static?

Patch was compile tested with: x86_64_defconfig + FTRACE=y
FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
SAMPLE_LIVEPATCH=y

Patch was runtested with:
   insmod samples/livepatch/livepatch-shadow-mod.ko
   insmod samples/livepatch/livepatch-shadow-fix1.ko
   insmod samples/livepatch/livepatch-shadow-fix2.ko
   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
   rmmod livepatch-shadow-fix2
   rmmod livepatch-shadow-fix1
   rmmod livepatch-shadow-mod
and dmesg output compared with the run before the patch was
applied.

Patch is against 4.20-rc6 (localversion-next is next-20181214)

 samples/livepatch/livepatch-shadow-fix1.c |  4 ++--
 samples/livepatch/livepatch-shadow-fix2.c |  4 ++--
 samples/livepatch/livepatch-shadow-mod.c  | 11 ++++++-----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 49b1355..ebe78e2 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
 	return 0;
 }
 
-struct dummy *livepatch_fix1_dummy_alloc(void)
+static struct dummy *livepatch_fix1_dummy_alloc(void)
 {
 	struct dummy *d;
 	void *leak;
@@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 			 __func__, d, *shadow_leak);
 }
 
-void livepatch_fix1_dummy_free(struct dummy *d)
+static void livepatch_fix1_dummy_free(struct dummy *d)
 {
 	void **shadow_leak;
 
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index b34c7bf..b6dac2b 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -50,7 +50,7 @@ struct dummy {
 	unsigned long jiffies_expire;
 };
 
-bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
+static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
 {
 	int *shadow_count;
 
@@ -78,7 +78,7 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
 			 __func__, d, *shadow_leak);
 }
 
-void livepatch_fix2_dummy_free(struct dummy *d)
+static void livepatch_fix2_dummy_free(struct dummy *d)
 {
 	void **shadow_leak;
 	int *shadow_count;
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
index 4c54b25..2168d57 100644
--- a/samples/livepatch/livepatch-shadow-mod.c
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -96,15 +96,15 @@ MODULE_DESCRIPTION("Buggy module for shadow variable demo");
  * Keep a list of all the dummies so we can clean up any residual ones
  * on module exit
  */
-LIST_HEAD(dummy_list);
-DEFINE_MUTEX(dummy_list_mutex);
+static LIST_HEAD(dummy_list);
+static DEFINE_MUTEX(dummy_list_mutex);
 
 struct dummy {
 	struct list_head list;
 	unsigned long jiffies_expire;
 };
 
-noinline struct dummy *dummy_alloc(void)
+static __noclone noinline struct dummy *dummy_alloc(void)
 {
 	struct dummy *d;
 	void *leak;
@@ -125,7 +125,7 @@ noinline struct dummy *dummy_alloc(void)
 	return d;
 }
 
-noinline void dummy_free(struct dummy *d)
+static __noclone noinline void dummy_free(struct dummy *d)
 {
 	pr_info("%s: dummy @ %p, expired = %lx\n",
 		__func__, d, d->jiffies_expire);
@@ -133,7 +133,8 @@ noinline void dummy_free(struct dummy *d)
 	kfree(d);
 }
 
-noinline bool dummy_check(struct dummy *d, unsigned long jiffies)
+static __noclone noinline bool dummy_check(struct dummy *d,
+					   unsigned long jiffies)
 {
 	return time_after(jiffies, d->jiffies_expire);
 }
-- 
2.1.4

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

[Petr Mladek]

On Sun 2018-12-16 14:07:37, 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

This is misleading. The "Link:" tag should point to the mail
where the commited version of the patch was being discussed.
It can be added by the maintainer.

Also please do not use links to https://lkml.org.
Messages disappear from time to time. It is much better
to use the redirector:

      https://lkml.kernel.org/r/<message_id>

Best Regards,
Petr


PS: I was about to add Reviewed-by tag. But Miroslav (sitting
next to me) found out that __noclone was not the best solution.

I suggest to always wait few days between re-sending a new
version. This subsystem has many active reviewers ;-)

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

[Miroslav Benes]

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.

Miroslav
 
> Sparse reported the following findings:
> 
> CHECK   samples/livepatch/livepatch-shadow-mod.c
> samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
>  'dummy_list' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
>  'dummy_list_mutex' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
>  'dummy_alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:128:15: warning: symbol
>  'dummy_free' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:136:15: warning: symbol
>  'dummy_check' was not declared. Should it be static?
> 
> CHECK   samples/livepatch/livepatch-shadow-fix1.c
> samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
>  'livepatch_fix1_dummy_alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-fix1.c:111:6: warning: symbol
>  'livepatch_fix1_dummy_free' was not declared. Should it be static?
> 
> CHECK   samples/livepatch/livepatch-shadow-fix2.c
> samples/livepatch/livepatch-shadow-fix2.c:53:6: warning: symbol
>  'livepatch_fix2_dummy_check' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-fix2.c:81:6: warning: symbol
>  'livepatch_fix2_dummy_free' was not declared. Should it be static?
> 
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> SAMPLE_LIVEPATCH=y
> 
> Patch was runtested with:
>    insmod samples/livepatch/livepatch-shadow-mod.ko
>    insmod samples/livepatch/livepatch-shadow-fix1.ko
>    insmod samples/livepatch/livepatch-shadow-fix2.ko
>    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
>    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
>    rmmod livepatch-shadow-fix2
>    rmmod livepatch-shadow-fix1
>    rmmod livepatch-shadow-mod
> and dmesg output compared with the run before the patch was
> applied.

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

[Joe Lawrence]

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?


[1]
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noclone-function-attribute


Thanks,

-- Joe

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

[Nicholas Mc Guire]

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

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

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

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

[Miroslav Benes]

On Mon, 17 Dec 2018, Nicholas Mc Guire wrote:

> 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

Yes, __used is better.

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

No, I don't think so. The samples (and selftests) are special in that we 
would like to provide minimal examples (or testcases). GCC is in the way 
due to its optimizations. It is not the case with the common code (the 
problem there is a bit different. Some optimizations are just dangerous 
for livepatching. However that is tangential to this patch and will be 
solved elsewhere).

Thanks,
Miroslav

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

[Joe Lawrence]

On 12/18/2018 03:49 AM, Miroslav Benes wrote:
> On Mon, 17 Dec 2018, Joe Lawrence wrote:
> 
>> 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."
> 
> ?

That works for me.

-- Joe

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

[Joe Lawrence]

On 12/18/18 10:18 AM, Joe Lawrence wrote:
> On 12/18/2018 03:49 AM, Miroslav Benes wrote:
>> On Mon, 17 Dec 2018, Joe Lawrence wrote:
>>
>>> 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."
>>
>> ?
> 
> That works for me.
> 
Hi Nicholas,

Did you still want to post a v3 for this fix?  I think there were only a 
few v3 suggestions (link tag, tag order, __used attribute, and commit 
msg phrasing.)

The context has been clipped in the quoting above, so here's an archive 
link if you need it:

https://lore.kernel.org/lkml/1544965657-26804-1-git-send-email-hofrat@osadl.org/

Thanks,

-- Joe

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

[Nicholas Mc Guire]

On Tue, Jan 22, 2019 at 11:30:30AM -0500, Joe Lawrence wrote:
> On 12/18/18 10:18 AM, Joe Lawrence wrote:
> >On 12/18/2018 03:49 AM, Miroslav Benes wrote:
> >>On Mon, 17 Dec 2018, Joe Lawrence wrote:
> >>
> >>>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."
> >>
> >>?
> >
> >That works for me.
> >
> Hi Nicholas,
> 
> Did you still want to post a v3 for this fix?  I think there were only a few
> v3 suggestions (link tag, tag order, __used attribute, and commit msg
> phrasing.)
>
yup - will go cleanup and repost.

thx!
hofrat