Linux EDAC development
 help / color / mirror / Atom feed
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: Julia Lawall <julia.lawall@lip6.fr>,
	"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Cc: Borislav Petkov <bp@alien8.de>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	cocci@systeme.lip6.fr
Subject: EDAC: remove unnecessary static in edac_fake_inject_write()
Date: Sun, 23 Jul 2017 00:42:38 -0500	[thread overview]
Message-ID: <6867696d-edd1-3d21-eb7e-af68e087750a@embeddedor.com> (raw)

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

             reply	other threads:[~2017-07-23  5:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-23  5:42 Gustavo A. R. Silva [this message]
  -- strict thread matches above, loose matches on Subject: below --
2017-07-24 10:34 EDAC: remove unnecessary static in edac_fake_inject_write() Borislav Petkov
2017-07-23  6:25 Gustavo A. R. Silva
2017-07-23  5:53 Julia Lawall
2017-07-23  5:07 Julia Lawall
2017-07-23  2:31 Gustavo A. R. Silva
2017-07-22 16:22 Gustavo A. R. Silva
2017-07-22  6:39 Julia Lawall
2017-07-22  6:36 Borislav Petkov
2017-07-21 20:08 Julia Lawall
2017-07-17  8:39 Borislav Petkov
2017-07-04 21:44 Gustavo A. R. Silva

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6867696d-edd1-3d21-eb7e-af68e087750a@embeddedor.com \
    --to=gustavo@embeddedor.com \
    --cc=bp@alien8.de \
    --cc=cocci@systeme.lip6.fr \
    --cc=garsilva@embeddedor.com \
    --cc=julia.lawall@lip6.fr \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mchehab@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox