EDAC: remove unnecessary static in edac_fake_inject_write()

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 at p;
...
x = <+...x...+>

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

-static
T x at 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(-)

drivers/edac/debugfs.c +1 -1

index 92dbb7e..ba0af49 100644
@@ -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)

Signature · 1 line

2.5.0

Quoted 25 lines · 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 at p;
...
x = <+...x...+>

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

Quoted 5 lines

-static
T x at 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.)

Quoted 31 lines

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

drivers/edac/debugfs.c +1 -2

index 92dbb7e..ba0af49 100644
@@ -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)
--

Quoted 2 lines

2.5.0

--
Regards/Gruss,
Boris.

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

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:

Quoted 3 lines · Julia Lawall

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.

Quoted 1 line

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

Quoted 3 lines · Julia Lawall

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.

Quoted 1 line

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

Hi all,

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Julia Lawall

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.

Quoted 6 lines

Quoted 1 line

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

Hi Julia, Borislav,

Quoted 10 lines · Gustavo A. R. Silva

Hi all,

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Julia Lawall

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.

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

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 at p;
... when != x = e
x = <+...x...+>

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

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

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

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

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

-static
T x at 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

Quoted 70 lines · Gustavo A. R. Silva

Hi Julia, Borislav,

Quoted 10 lines · Gustavo A. R. Silva

Hi all,

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Julia Lawall

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.

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

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 at p;
... when != x = e
x = <+...x...+>

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

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

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

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

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

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

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 1 line

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.

Hi Julia,

Quoted 80 lines · Julia Lawall

Quoted 75 lines · Gustavo A. R. Silva

Hi Julia, Borislav,

Quoted 12 lines · Gustavo A. R. Silva

Hi all,

Quoted 9 lines · Borislav Petkov

Quoted 5 lines · Julia Lawall

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.

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

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 at p;
... when != x = e
x = <+...x...+>

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

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

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

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

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

-static
T x at 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.

Quoted 4 lines

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.

Quoted 5 lines

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 ?

Quoted 11 lines

Quoted 11 lines

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.

Quoted 8 lines

Quoted 1 line

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

Quoted 88 lines · Gustavo A. R. Silva

Hi Julia,

Quoted 84 lines · Julia Lawall

Quoted 67 lines · Gustavo A. R. Silva

Hi Julia, Borislav,

Quoted 12 lines · Gustavo A. R. Silva

Hi all,

Quoted 9 lines · Borislav Petkov

Quoted 5 lines · Julia Lawall

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.

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

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 at p;
... when != x = e
x = <+...x...+>

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

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

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

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

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

Quoted 8 lines

-static
T x at 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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 5 lines

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.

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