linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
@ 2017-07-04 21:44 Gustavo A. R. Silva
  2017-07-17  8:39 ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Gustavo A. R. Silva @ 2017-07-04 21:44 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab
  Cc: linux-edac, linux-kernel, Gustavo A. R. Silva

Remove unnecessary static on local variable _type_.
Such variable is initialized before being used,
on every execution path throughout the function.
The static has no benefit and, removing it reduces
the code size.

This issue was detected using Coccinelle and the following semantic patch:

@bad exists@
position p;
identifier x;
type T;
@@

static T x@p;
...
x = <+...x...+>

@@
identifier x;
expression e;
type T;
position p != bad.p;
@@

-static
 T x@p;
 ... when != x
     when strict
?x = e;

In the following log you can see the difference in the code size and,
also a significant difference in bss segment. This log is the output
of the size command, before and after the code change:

before:
   text    data     bss     dec     hex filename
   2966     920     128    4014     fae drivers/edac/debugfs.o

after:
   text     data     bss    dec     hex filename
   2961     832      64    3857     f11 drivers/edac/debugfs.o

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/edac/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
index 92dbb7e..ba0af49 100644
--- a/drivers/edac/debugfs.c
+++ b/drivers/edac/debugfs.c
@@ -8,7 +8,7 @@ static ssize_t edac_fake_inject_write(struct file *file,
 {
 	struct device *dev = file->private_data;
 	struct mem_ctl_info *mci = to_mci(dev);
-	static enum hw_event_mc_err_type type;
+	enum hw_event_mc_err_type type;
 	u16 errcount = mci->fake_inject_count;
 
 	if (!errcount)
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-04 21:44 [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write() Gustavo A. R. Silva
@ 2017-07-17  8:39 ` Borislav Petkov
  2017-07-21 20:08   ` Julia Lawall
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2017-07-17  8:39 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel, cocci

On Tue, Jul 04, 2017 at 04:44:40PM -0500, Gustavo A. R. Silva wrote:
> Remove unnecessary static on local variable _type_.
> Such variable is initialized before being used,
> on every execution path throughout the function.
> The static has no benefit and, removing it reduces
> the code size.
> 
> This issue was detected using Coccinelle and the following semantic patch:
> 
> @bad exists@
> position p;
> identifier x;
> type T;
> @@
> 
> static T x@p;
> ...
> x = <+...x...+>
> 
> @@
> identifier x;
> expression e;
> type T;
> position p != bad.p;
> @@
> 
> -static
>  T x@p;
>  ... when != x
>      when strict
> ?x = e;

So the fix is ok but I don't understand Coccinelle to be able to judge
whether the above patch is fine or not. If it is, it probably should be
put somewhere in scripts/coccinelle/ so that others can use it too so
that they can catch such useless uses of static too.

Lemme add the Coccinelle ML to CC.

(Leaving in the rest for reference.)

> In the following log you can see the difference in the code size and,
> also a significant difference in bss segment. This log is the output
> of the size command, before and after the code change:
> 
> before:
>    text    data     bss     dec     hex filename
>    2966     920     128    4014     fae drivers/edac/debugfs.o
> 
> after:
>    text     data     bss    dec     hex filename
>    2961     832      64    3857     f11 drivers/edac/debugfs.o
> 
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/edac/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
> index 92dbb7e..ba0af49 100644
> --- a/drivers/edac/debugfs.c
> +++ b/drivers/edac/debugfs.c
> @@ -8,7 +8,7 @@ static ssize_t edac_fake_inject_write(struct file *file,
>  {
>  	struct device *dev = file->private_data;
>  	struct mem_ctl_info *mci = to_mci(dev);
> -	static enum hw_event_mc_err_type type;
> +	enum hw_event_mc_err_type type;
>  	u16 errcount = mci->fake_inject_count;
>  
>  	if (!errcount)
> -- 
> 2.5.0
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-17  8:39 ` Borislav Petkov
@ 2017-07-21 20:08   ` Julia Lawall
  2017-07-22  6:36     ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2017-07-21 20:08 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Gustavo A. R. Silva, Mauro Carvalho Chehab, linux-edac,
	linux-kernel, cocci



On Mon, 17 Jul 2017, Borislav Petkov wrote:

> On Tue, Jul 04, 2017 at 04:44:40PM -0500, Gustavo A. R. Silva wrote:
> > Remove unnecessary static on local variable _type_.
> > Such variable is initialized before being used,
> > on every execution path throughout the function.
> > The static has no benefit and, removing it reduces
> > the code size.
> >
> > This issue was detected using Coccinelle and the following semantic patch:
> >
> > @bad exists@
> > position p;
> > identifier x;
> > type T;
> > @@
> >
> > static T x@p;
> > ...
> > x = <+...x...+>
> >
> > @@
> > identifier x;
> > expression e;
> > type T;
> > position p != bad.p;
> > @@
> >
> > -static
> >  T x@p;
> >  ... when != x
> >      when strict
> > ?x = e;
>
> So the fix is ok but I don't understand Coccinelle to be able to judge
> whether the above patch is fine or not. If it is, it probably should be
> put somewhere in scripts/coccinelle/ so that others can use it too so
> that they can catch such useless uses of static too.

Someone pointed out that the rule is probably not OK when the address of
the static variable is taken, because then it is likely being used as
permanent storage.  An improved rule is:

@bad exists@
position p;
identifier x;
expression e;
type T;
@@

static T x@p;
... when != x = e
x = <+...x...+>

@worse exists@
position p;
identifier x;
type T;
@@

static T x@p;
...
 &x

@@
identifier x;
expression e;
type T;
position p != {bad.p,worse.p};
@@

-static
 T x@p;
 ... when != x
     when strict
?x = e;

julia

>
> Lemme add the Coccinelle ML to CC.
>
> (Leaving in the rest for reference.)
>
> > In the following log you can see the difference in the code size and,
> > also a significant difference in bss segment. This log is the output
> > of the size command, before and after the code change:
> >
> > before:
> >    text    data     bss     dec     hex filename
> >    2966     920     128    4014     fae drivers/edac/debugfs.o
> >
> > after:
> >    text     data     bss    dec     hex filename
> >    2961     832      64    3857     f11 drivers/edac/debugfs.o
> >
> > Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> > ---
> >  drivers/edac/debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
> > index 92dbb7e..ba0af49 100644
> > --- a/drivers/edac/debugfs.c
> > +++ b/drivers/edac/debugfs.c
> > @@ -8,7 +8,7 @@ static ssize_t edac_fake_inject_write(struct file *file,
> >  {
> >  	struct device *dev = file->private_data;
> >  	struct mem_ctl_info *mci = to_mci(dev);
> > -	static enum hw_event_mc_err_type type;
> > +	enum hw_event_mc_err_type type;
> >  	u16 errcount = mci->fake_inject_count;
> >
> >  	if (!errcount)
> > --
> > 2.5.0
> >
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-21 20:08   ` Julia Lawall
@ 2017-07-22  6:36     ` Borislav Petkov
  2017-07-22  6:39       ` Julia Lawall
  2017-07-22 16:22       ` Gustavo A. R. Silva
  0 siblings, 2 replies; 12+ messages in thread
From: Borislav Petkov @ 2017-07-22  6:36 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Gustavo A. R. Silva, Mauro Carvalho Chehab, linux-edac,
	linux-kernel, cocci

On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
> Someone pointed out that the rule is probably not OK when the address of
> the static variable is taken, because then it is likely being used as
> permanent storage.

Makes sense to me.

> An improved rule is:

Do you think it is worth having it in scripts/coccinelle/ ?

I don't think Gustavo would mind putting it there :)

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-22  6:36     ` Borislav Petkov
@ 2017-07-22  6:39       ` Julia Lawall
  2017-07-22 16:22       ` Gustavo A. R. Silva
  1 sibling, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2017-07-22  6:39 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Gustavo A. R. Silva, Mauro Carvalho Chehab, linux-edac,
	linux-kernel, cocci



On Sat, 22 Jul 2017, Borislav Petkov wrote:

> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
> > Someone pointed out that the rule is probably not OK when the address of
> > the static variable is taken, because then it is likely being used as
> > permanent storage.
>
> Makes sense to me.
>
> > An improved rule is:
>
> Do you think it is worth having it in scripts/coccinelle/ ?

Sure.  Now that it has been a bit more sanity checked.

> I don't think Gustavo would mind putting it there :)

OK, I will check with him.

julia

>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-22  6:36     ` Borislav Petkov
  2017-07-22  6:39       ` Julia Lawall
@ 2017-07-22 16:22       ` Gustavo A. R. Silva
  2017-07-23  2:31         ` Gustavo A. R. Silva
  1 sibling, 1 reply; 12+ messages in thread
From: Gustavo A. R. Silva @ 2017-07-22 16:22 UTC (permalink / raw)
  To: Borislav Petkov, Julia Lawall
  Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel, cocci

Hi all,

On 07/22/2017 01:36 AM, Borislav Petkov wrote:
> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
>> Someone pointed out that the rule is probably not OK when the address of
>> the static variable is taken, because then it is likely being used as
>> permanent storage.
>
> Makes sense to me.
>
>> An improved rule is:
>
> Do you think it is worth having it in scripts/coccinelle/ ?
>
> I don't think Gustavo would mind putting it there :)
>

Absolutely, I'd be glad to help out. :)

-- 
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-22 16:22       ` Gustavo A. R. Silva
@ 2017-07-23  2:31         ` Gustavo A. R. Silva
  2017-07-23  5:07           ` Julia Lawall
  0 siblings, 1 reply; 12+ messages in thread
From: Gustavo A. R. Silva @ 2017-07-23  2:31 UTC (permalink / raw)
  To: Borislav Petkov, Julia Lawall
  Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel, cocci,
	Gustavo A. R. Silva

Hi Julia, Borislav,

On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
> Hi all,
>
> On 07/22/2017 01:36 AM, Borislav Petkov wrote:
>> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
>>> Someone pointed out that the rule is probably not OK when the address of
>>> the static variable is taken, because then it is likely being used as
>>> permanent storage.
>>
>> Makes sense to me.
>>
>>> An improved rule is:
>>
>> Do you think it is worth having it in scripts/coccinelle/ ?
>>
>> I don't think Gustavo would mind putting it there :)
>>
>
> Absolutely, I'd be glad to help out. :)
>

I've been working on this issue today and, in my opinion, this script is 
even better:

@bad exists@
position p;
identifier x;
expression e;
type T;
@@

static T x@p;
... when != x = e
x = <+...x...+>

@worse1 exists@
position p;
identifier x;
type T;
@@

static T x@p;
...
return &x;

@worse2 exists@
position p;
identifier x;
type T;
@@

static T *x@p;
...
return x;

@@
identifier x;
expression e;
type T;
position p != {bad.p,worse1.p,worse2.p};
@@

-static
   T x@p;
   ... when != x
       when strict
?x = e;

It ignores all the cases in which the address of the static variable is 
returned to the caller function.

Also, there are some cases in which the maintainer can argue something 
like the following:

https://lkml.org/lkml/2017/7/19/1381

but that depends on the particular conditions in which the code is 
intended to be executed.

What do you think?

Thank you
--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-23  2:31         ` Gustavo A. R. Silva
@ 2017-07-23  5:07           ` Julia Lawall
  2017-07-23  5:42             ` Gustavo A. R. Silva
  0 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2017-07-23  5:07 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel,
	cocci, Gustavo A. R. Silva



On Sat, 22 Jul 2017, Gustavo A. R. Silva wrote:

> Hi Julia, Borislav,
>
> On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
> > Hi all,
> >
> > On 07/22/2017 01:36 AM, Borislav Petkov wrote:
> > > On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
> > > > Someone pointed out that the rule is probably not OK when the address of
> > > > the static variable is taken, because then it is likely being used as
> > > > permanent storage.
> > >
> > > Makes sense to me.
> > >
> > > > An improved rule is:
> > >
> > > Do you think it is worth having it in scripts/coccinelle/ ?
> > >
> > > I don't think Gustavo would mind putting it there :)
> > >
> >
> > Absolutely, I'd be glad to help out. :)
> >
>
> I've been working on this issue today and, in my opinion, this script is even
> better:
>
> @bad exists@
> position p;
> identifier x;
> expression e;
> type T;
> @@
>
> static T x@p;
> ... when != x = e
> x = <+...x...+>
>
> @worse1 exists@
> position p;
> identifier x;
> type T;
> @@
>
> static T x@p;
> ...
> return &x;
>
> @worse2 exists@
> position p;
> identifier x;
> type T;
> @@
>
> static T *x@p;
> ...
> return x;
>
> @@
> identifier x;
> expression e;
> type T;
> position p != {bad.p,worse1.p,worse2.p};
> @@
>
> -static
>   T x@p;
>   ... when != x
>       when strict
> ?x = e;
>
> It ignores all the cases in which the address of the static variable is
> returned to the caller function.

I don't understand why you want to restrict the address of a variable case
to returns.  Storing the address in a field of a structure that has a
lifetime beyond the function body is a problem as well.

On the other hand returning the value stored in a static variable is not a
problem.  That value exists independently of the variable that contains
it.  The variable that conains it doesn't need to live on in any way.

>
> Also, there are some cases in which the maintainer can argue something like
> the following:
>
> https://lkml.org/lkml/2017/7/19/1381
>
> but that depends on the particular conditions in which the code is intended to
> be executed.
>
> What do you think?

The preserving values argument is not relevant.  The rule checks that the
value is never used.  DMA accesses should involve taking an address, which
we now disallow.  It seems likely that anything large would have its
address taken too, but one could check manually for that.  spgen provides
a section where you can describe such issues.

julia

> Thank you
> --
> Gustavo A. R. Silva
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-23  5:07           ` Julia Lawall
@ 2017-07-23  5:42             ` Gustavo A. R. Silva
  2017-07-23  5:53               ` Julia Lawall
  0 siblings, 1 reply; 12+ messages in thread
From: Gustavo A. R. Silva @ 2017-07-23  5:42 UTC (permalink / raw)
  To: Julia Lawall, Gustavo A. R. Silva
  Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel, cocci

Hi Julia,

On 07/23/2017 12:07 AM, Julia Lawall wrote:
>
>
> On Sat, 22 Jul 2017, Gustavo A. R. Silva wrote:
>
>> Hi Julia, Borislav,
>>
>> On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
>>> Hi all,
>>>
>>> On 07/22/2017 01:36 AM, Borislav Petkov wrote:
>>>> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
>>>>> Someone pointed out that the rule is probably not OK when the address of
>>>>> the static variable is taken, because then it is likely being used as
>>>>> permanent storage.
>>>>
>>>> Makes sense to me.
>>>>
>>>>> An improved rule is:
>>>>
>>>> Do you think it is worth having it in scripts/coccinelle/ ?
>>>>
>>>> I don't think Gustavo would mind putting it there :)
>>>>
>>>
>>> Absolutely, I'd be glad to help out. :)
>>>
>>
>> I've been working on this issue today and, in my opinion, this script is even
>> better:
>>
>> @bad exists@
>> position p;
>> identifier x;
>> expression e;
>> type T;
>> @@
>>
>> static T x@p;
>> ... when != x = e
>> x = <+...x...+>
>>
>> @worse1 exists@
>> position p;
>> identifier x;
>> type T;
>> @@
>>
>> static T x@p;
>> ...
>> return &x;
>>
>> @worse2 exists@
>> position p;
>> identifier x;
>> type T;
>> @@
>>
>> static T *x@p;
>> ...
>> return x;
>>
>> @@
>> identifier x;
>> expression e;
>> type T;
>> position p != {bad.p,worse1.p,worse2.p};
>> @@
>>
>> -static
>>   T x@p;
>>   ... when != x
>>       when strict
>> ?x = e;
>>
>> It ignores all the cases in which the address of the static variable is
>> returned to the caller function.
>
> I don't understand why you want to restrict the address of a variable case
> to returns.  Storing the address in a field of a structure that has a
> lifetime beyond the function body is a problem as well.
>

Yeah, I totally agree and, personally I consider that a bad coding 
practice. But I think those kinds of issues should be addressed in a 
different script.

> On the other hand returning the value stored in a static variable is not a
> problem.  That value exists independently of the variable that contains
> it.  The variable that conains it doesn't need to live on in any way.
>

Yeah, I agree, but I don't see exactly where this argument is coming from ?

Notice that for both worse1 and worse2, what is returned is the address, 
not the value of the static variable. At least that was my intention, 
unless I maybe missing something ?

>>
>> Also, there are some cases in which the maintainer can argue something like
>> the following:
>>
>> https://lkml.org/lkml/2017/7/19/1381
>>
>> but that depends on the particular conditions in which the code is intended to
>> be executed.
>>
>> What do you think?
>
> The preserving values argument is not relevant.  The rule checks that the
> value is never used.  DMA accesses should involve taking an address, which
> we now disallow.  It seems likely that anything large would have its
> address taken too, but one could check manually for that.  spgen provides
> a section where you can describe such issues.
>

Yeah, those cases should be analyzed manually and in case of doubt check 
with the maintainers.

Thanks
-- 
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-23  5:42             ` Gustavo A. R. Silva
@ 2017-07-23  5:53               ` Julia Lawall
  2017-07-23  6:25                 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2017-07-23  5:53 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, Borislav Petkov, Mauro Carvalho Chehab,
	linux-edac, linux-kernel, cocci



On Sun, 23 Jul 2017, Gustavo A. R. Silva wrote:

> Hi Julia,
>
> On 07/23/2017 12:07 AM, Julia Lawall wrote:
> >
> >
> > On Sat, 22 Jul 2017, Gustavo A. R. Silva wrote:
> >
> > > Hi Julia, Borislav,
> > >
> > > On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
> > > > Hi all,
> > > >
> > > > On 07/22/2017 01:36 AM, Borislav Petkov wrote:
> > > > > On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
> > > > > > Someone pointed out that the rule is probably not OK when the
> > > > > > address of
> > > > > > the static variable is taken, because then it is likely being used
> > > > > > as
> > > > > > permanent storage.
> > > > >
> > > > > Makes sense to me.
> > > > >
> > > > > > An improved rule is:
> > > > >
> > > > > Do you think it is worth having it in scripts/coccinelle/ ?
> > > > >
> > > > > I don't think Gustavo would mind putting it there :)
> > > > >
> > > >
> > > > Absolutely, I'd be glad to help out. :)
> > > >
> > >
> > > I've been working on this issue today and, in my opinion, this script is
> > > even
> > > better:
> > >
> > > @bad exists@
> > > position p;
> > > identifier x;
> > > expression e;
> > > type T;
> > > @@
> > >
> > > static T x@p;
> > > ... when != x = e
> > > x = <+...x...+>
> > >
> > > @worse1 exists@
> > > position p;
> > > identifier x;
> > > type T;
> > > @@
> > >
> > > static T x@p;
> > > ...
> > > return &x;
> > >
> > > @worse2 exists@
> > > position p;
> > > identifier x;
> > > type T;
> > > @@
> > >
> > > static T *x@p;
> > > ...
> > > return x;
> > >
> > > @@
> > > identifier x;
> > > expression e;
> > > type T;
> > > position p != {bad.p,worse1.p,worse2.p};
> > > @@
> > >
> > > -static
> > >   T x@p;
> > >   ... when != x
> > >       when strict
> > > ?x = e;
> > >
> > > It ignores all the cases in which the address of the static variable is
> > > returned to the caller function.
> >
> > I don't understand why you want to restrict the address of a variable case
> > to returns.  Storing the address in a field of a structure that has a
> > lifetime beyond the function body is a problem as well.
> >
>
> Yeah, I totally agree and, personally I consider that a bad coding practice.
> But I think those kinds of issues should be addressed in a different script.

I don't understand the response at all.  My point was that you have taken
a very general reason to not apply the change, ie the presence of &x
anywhere, and limited it to a special case: you don't apply the change
when there exists return &x and you do apply the script when there exits
a->b = &x.  But the change is not safe to apply in both cases.


>
> > On the other hand returning the value stored in a static variable is not a
> > problem.  That value exists independently of the variable that contains
> > it.  The variable that conains it doesn't need to live on in any way.
> >
>
> Yeah, I agree, but I don't see exactly where this argument is coming from ?
>
> Notice that for both worse1 and worse2, what is returned is the address, not
> the value of the static variable. At least that was my intention, unless I
> maybe missing something ?

return x returns the value of x.  It does not return the address of x.

julia

> > >
> > > Also, there are some cases in which the maintainer can argue something
> > > like
> > > the following:
> > >
> > > https://lkml.org/lkml/2017/7/19/1381
> > >
> > > but that depends on the particular conditions in which the code is
> > > intended to
> > > be executed.
> > >
> > > What do you think?
> >
> > The preserving values argument is not relevant.  The rule checks that the
> > value is never used.  DMA accesses should involve taking an address, which
> > we now disallow.  It seems likely that anything large would have its
> > address taken too, but one could check manually for that.  spgen provides
> > a section where you can describe such issues.
> >
>
> Yeah, those cases should be analyzed manually and in case of doubt check with
> the maintainers.
>
> Thanks
> --
> Gustavo A. R. Silva
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-23  5:53               ` Julia Lawall
@ 2017-07-23  6:25                 ` Gustavo A. R. Silva
  2017-07-24 10:34                   ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Gustavo A. R. Silva @ 2017-07-23  6:25 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Gustavo A. R. Silva, Borislav Petkov, Mauro Carvalho Chehab,
	linux-edac, linux-kernel, cocci



On 07/23/2017 12:53 AM, Julia Lawall wrote:
>
>
> On Sun, 23 Jul 2017, Gustavo A. R. Silva wrote:
>
>> Hi Julia,
>>
>> On 07/23/2017 12:07 AM, Julia Lawall wrote:
>>>
>>>
>>> On Sat, 22 Jul 2017, Gustavo A. R. Silva wrote:
>>>
>>>> Hi Julia, Borislav,
>>>>
>>>> On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
>>>>> Hi all,
>>>>>
>>>>> On 07/22/2017 01:36 AM, Borislav Petkov wrote:
>>>>>> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
>>>>>>> Someone pointed out that the rule is probably not OK when the
>>>>>>> address of
>>>>>>> the static variable is taken, because then it is likely being used
>>>>>>> as
>>>>>>> permanent storage.
>>>>>>
>>>>>> Makes sense to me.
>>>>>>
>>>>>>> An improved rule is:
>>>>>>
>>>>>> Do you think it is worth having it in scripts/coccinelle/ ?
>>>>>>
>>>>>> I don't think Gustavo would mind putting it there :)
>>>>>>
>>>>>
>>>>> Absolutely, I'd be glad to help out. :)
>>>>>
>>>>
>>>> I've been working on this issue today and, in my opinion, this script is
>>>> even
>>>> better:
>>>>
>>>> @bad exists@
>>>> position p;
>>>> identifier x;
>>>> expression e;
>>>> type T;
>>>> @@
>>>>
>>>> static T x@p;
>>>> ... when != x = e
>>>> x = <+...x...+>
>>>>
>>>> @worse1 exists@
>>>> position p;
>>>> identifier x;
>>>> type T;
>>>> @@
>>>>
>>>> static T x@p;
>>>> ...
>>>> return &x;
>>>>
>>>> @worse2 exists@
>>>> position p;
>>>> identifier x;
>>>> type T;
>>>> @@
>>>>
>>>> static T *x@p;
>>>> ...
>>>> return x;
>>>>
>>>> @@
>>>> identifier x;
>>>> expression e;
>>>> type T;
>>>> position p != {bad.p,worse1.p,worse2.p};
>>>> @@
>>>>
>>>> -static
>>>>   T x@p;
>>>>   ... when != x
>>>>       when strict
>>>> ?x = e;
>>>>
>>>> It ignores all the cases in which the address of the static variable is
>>>> returned to the caller function.
>>>
>>> I don't understand why you want to restrict the address of a variable case
>>> to returns.  Storing the address in a field of a structure that has a
>>> lifetime beyond the function body is a problem as well.
>>>
>>
>> Yeah, I totally agree and, personally I consider that a bad coding practice.
>> But I think those kinds of issues should be addressed in a different script.
>
> I don't understand the response at all.  My point was that you have taken
> a very general reason to not apply the change, ie the presence of &x
> anywhere, and limited it to a special case: you don't apply the change
> when there exists return &x and you do apply the script when there exits
> a->b = &x.  But the change is not safe to apply in both cases.
>

I see your point now.

>
>>
>>> On the other hand returning the value stored in a static variable is not a
>>> problem.  That value exists independently of the variable that contains
>>> it.  The variable that conains it doesn't need to live on in any way.
>>>
>>
>> Yeah, I agree, but I don't see exactly where this argument is coming from ?
>>
>> Notice that for both worse1 and worse2, what is returned is the address, not
>> the value of the static variable. At least that was my intention, unless I
>> maybe missing something ?
>
> return x returns the value of x.  It does not return the address of x.
>

You are right, I missed that one. Thank you for pointing it out.

Lesson learned, your original script should remain as is. :)

Have a good day,
Thank you!
-- 
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()
  2017-07-23  6:25                 ` Gustavo A. R. Silva
@ 2017-07-24 10:34                   ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2017-07-24 10:34 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Julia Lawall, Gustavo A. R. Silva, Mauro Carvalho Chehab,
	linux-edac, linux-kernel, cocci

On Sun, Jul 23, 2017 at 01:25:56AM -0500, Gustavo A. R. Silva wrote:
> Lesson learned, your original script should remain as is. :)

Ok, then please send Julia's script as a separate patch and then in the
EDAC patch, reference the path to that coccinelle patch so that it is
clear what has been used.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-07-24 10:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-04 21:44 [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write() Gustavo A. R. Silva
2017-07-17  8:39 ` Borislav Petkov
2017-07-21 20:08   ` Julia Lawall
2017-07-22  6:36     ` Borislav Petkov
2017-07-22  6:39       ` Julia Lawall
2017-07-22 16:22       ` Gustavo A. R. Silva
2017-07-23  2:31         ` Gustavo A. R. Silva
2017-07-23  5:07           ` Julia Lawall
2017-07-23  5:42             ` Gustavo A. R. Silva
2017-07-23  5:53               ` Julia Lawall
2017-07-23  6:25                 ` Gustavo A. R. Silva
2017-07-24 10:34                   ` Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).