Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject_write()

[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