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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
--
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 <[email protected]>
> > ---
> > 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.
> --
>
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.
--
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.
> --
>
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
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
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
>
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
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
>
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
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.
--