Re: [Cocci] [PATCH v3 0/5] bq27xxx_battery data memory update

[Liam Breck]

On Thu, Aug 31, 2017 at 2:54 AM, Julia Lawall  wrote:
>
>
> On Tue, 29 Aug 2017, Liam Breck wrote:
>
>> Coccinelle folks,
>>
>> On Tue, Aug 29, 2017 at 5:29 PM, Sebastian Reichel
>>  wrote:
>> > Hi,
>> >
>> > On Tue, Aug 29, 2017 at 04:07:12PM -0700, Liam Breck wrote:
>> >> I don't see a Julia in CC list...
>> >
>> > <_< let's try that again.
>> >
>> >> On Tue, Aug 29, 2017 at 2:22 PM, Sebastian Reichel
>> >>  wrote:
>> >> > [adding Julia to Cc for Coccinelle question]
>> >> >
>> >> > Hi,
>> >> >
>> >> > On Tue, Aug 29, 2017 at 10:31:57AM -0500, Andrew F. Davis wrote:
>> >> >> On 08/29/2017 05:54 AM, Sebastian Reichel wrote:
>> >> >> > On Wed, Aug 23, 2017 at 08:36:12PM -0700, Liam Breck wrote:
>> >> >> >> Overview:
>> >> >> >> * Reorganizes chip data definitions
>> >> >> >> * Enables features landed in these patches:
>> >> >> >>   dt-bindings: power: supply: bq27xxx: Add monitored-battery 
>> >> >> >> documentation
>> >> >> >>   power: supply: bq27xxx: Add chip data memory read/write support
>> >> >> >>   power: supply: bq27xxx: Add power_supply_battery_info support
>> >> >> >> * Supports the following chips (only BQ27425 is active)
>> >> >> >>   BQ27500, 545, 425, 421, 441, 621
>> >> >> >>
>> >> >> >> Changes in v3:
>> >> >> >> * BQ27425 tested; workaround minor chip bug
>> >> >> >> * Dropped driver_version
>> >> >> >> * Fixed dbg_dupes logic for .props & .dm_regs
>> >> >> >> * Dropped two props array dupes
>> >> >> >>
>> >> >> >> Changes in v2:
>> >> >> >> * Added di->opts flags for remaining chip features
>> >> >> >> * Commented out untested bq27xxx_dm_regs parameters
>> >> >> >> * Changed dbg_dupes to run only once
>> >> >> >>
>> >> >> >> Notes on v1:
>> >> >> >> * Not fully tested (hence RFC tag)
>> >> >> >
>> >> >> > Thanks, full series queued.
>> >> >> >
>> >> >> > -- Sebastian
>> >> >>
>> >> >> Anyway, I've not got the time to fight these changes anymore, but at
>> >> >> very least could you drop 4/5, it's static analysis code made into a
>> >> >> runtime check built into a kernel driver, if not at least add my
>> >> >> nacked-by. :)
>> >> >
>> >> > Since it's not critical at all and nobody depends on it, I dropped 4/5
>> >> > for now. I agree, that checking it at runtime is not nice. On the other
>> >> > hand I do think a duplication check makes sense. Doing a static
>> >> > check should be possible, but I have no idea how to implement this
>> >> > (without much effort). I suspect Coccinelle can do it, so I added
>> >> > Julia.
>> >> >
>> >> > For reference this is the runtime check:
>> >> > https://patchwork.kernel.org/patch/9918953/
>>
>> The data structures being checked start here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n138
>>
>> And are aggregated here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n743
>
> I worked a bit on this, but unfortunately there is a major parsing
> problem, and most of the definitions are ignored.  Specifically, the
> definitions of the regs variables are interspersed with eg:
>
> #define bq27510g1_regs bq27500_regs

You can transform the macros with sed to either of:

static u8 *bq27510g1_regs = 0 // skip comparison if x == 0

static u8 bq27510g1_regs[BQ27XXX_REG_MAX] = { 0xFF } // skip
comparison if x[0] == 0xff

Does that help?

> I'm not sure whether this would be convenient to fix, since parsing is
> rather delicate.  Coccinelle doesn't run cpp first.  It tries to parse
> macros definitions so that their code can be transformed as needed t the
> place of the definition.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v3 0/5] bq27xxx_battery data memory update

[Julia Lawall]

On Tue, 29 Aug 2017, Liam Breck wrote:

> Coccinelle folks,
>
> On Tue, Aug 29, 2017 at 5:29 PM, Sebastian Reichel
>  wrote:
> > Hi,
> >
> > On Tue, Aug 29, 2017 at 04:07:12PM -0700, Liam Breck wrote:
> >> I don't see a Julia in CC list...
> >
> > <_< let's try that again.
> >
> >> On Tue, Aug 29, 2017 at 2:22 PM, Sebastian Reichel
> >>  wrote:
> >> > [adding Julia to Cc for Coccinelle question]
> >> >
> >> > Hi,
> >> >
> >> > On Tue, Aug 29, 2017 at 10:31:57AM -0500, Andrew F. Davis wrote:
> >> >> On 08/29/2017 05:54 AM, Sebastian Reichel wrote:
> >> >> > On Wed, Aug 23, 2017 at 08:36:12PM -0700, Liam Breck wrote:
> >> >> >> Overview:
> >> >> >> * Reorganizes chip data definitions
> >> >> >> * Enables features landed in these patches:
> >> >> >>   dt-bindings: power: supply: bq27xxx: Add monitored-battery 
> >> >> >> documentation
> >> >> >>   power: supply: bq27xxx: Add chip data memory read/write support
> >> >> >>   power: supply: bq27xxx: Add power_supply_battery_info support
> >> >> >> * Supports the following chips (only BQ27425 is active)
> >> >> >>   BQ27500, 545, 425, 421, 441, 621
> >> >> >>
> >> >> >> Changes in v3:
> >> >> >> * BQ27425 tested; workaround minor chip bug
> >> >> >> * Dropped driver_version
> >> >> >> * Fixed dbg_dupes logic for .props & .dm_regs
> >> >> >> * Dropped two props array dupes
> >> >> >>
> >> >> >> Changes in v2:
> >> >> >> * Added di->opts flags for remaining chip features
> >> >> >> * Commented out untested bq27xxx_dm_regs parameters
> >> >> >> * Changed dbg_dupes to run only once
> >> >> >>
> >> >> >> Notes on v1:
> >> >> >> * Not fully tested (hence RFC tag)
> >> >> >
> >> >> > Thanks, full series queued.
> >> >> >
> >> >> > -- Sebastian
> >> >>
> >> >> Anyway, I've not got the time to fight these changes anymore, but at
> >> >> very least could you drop 4/5, it's static analysis code made into a
> >> >> runtime check built into a kernel driver, if not at least add my
> >> >> nacked-by. :)
> >> >
> >> > Since it's not critical at all and nobody depends on it, I dropped 4/5
> >> > for now. I agree, that checking it at runtime is not nice. On the other
> >> > hand I do think a duplication check makes sense. Doing a static
> >> > check should be possible, but I have no idea how to implement this
> >> > (without much effort). I suspect Coccinelle can do it, so I added
> >> > Julia.
> >> >
> >> > For reference this is the runtime check:
> >> > https://patchwork.kernel.org/patch/9918953/
>
> The data structures being checked start here:
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n138
>
> And are aggregated here:
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n743

I worked a bit on this, but unfortunately there is a major parsing
problem, and most of the definitions are ignored.  Specifically, the
definitions of the regs variables are interspersed with eg:

#define bq27510g1_regs bq27500_regs

I'm not sure whether this would be convenient to fix, since parsing is
rather delicate.  Coccinelle doesn't run cpp first.  It tries to parse
macros definitions so that their code can be transformed as needed t the
place of the definition.

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script

[Reshetova, Elena]

> On Wed, 30 Aug 2017, Reshetova, Elena wrote:
> 
> >
> > >
> > > On Wed, 30 Aug 2017, Elena Reshetova wrote:
> > >
> > > > atomic_as_refcounter.cocci script allows detecting
> > > > cases when refcount_t type and API should be used
> > > > instead of atomic_t.
> > > >
> > > > Signed-off-by: Elena Reshetova 
> > >
> > > Acked-by: Julia Lawall 
> >
> > Thank you very much Julia! What is the correct path to merge this?
> > I will resend with your acked-by, but what is the tree that should merge it?
> 
> Marek will take it.  If you want to resend, please get rid of the
> metavariable y in the rule r1. It causes an unused variable warning.

Sure, will do. Thank you!

Best Regards,
Elena.
> 
> julia
> 
> >
> > Best Regards,
> > Elena.
> > >
> > > > ---
> > > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 131
> > > ++
> > > >  1 file changed, 131 insertions(+)
> > > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > >
> > > > diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > > new file mode 100644
> > > > index 000..f83771b
> > > > --- /dev/null
> > > > +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > > @@ -0,0 +1,131 @@
> > > > +// Check if refcount_t type and API should be used
> > > > +// instead of atomic_t type when dealing with refcounters
> > > > +//
> > > > +// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
> > > > +//
> > > > +// Confidence: Moderate
> > > > +// URL: http://coccinelle.lip6.fr/
> > > > +// Options: --include-headers --very-quiet
> > > > +
> > > > +virtual report
> > > > +
> > > > +@r1 exists@
> > > > +identifier a, x, y;
> > > > +position p1, p2;
> > > > +identifier fname =~ ".*free.*";
> > > > +identifier fname2 =~ ".*destroy.*";
> > > > +identifier fname3 =~ ".*del.*";
> > > > +identifier fname4 =~ ".*queue_work.*";
> > > > +identifier fname5 =~ ".*schedule_work.*";
> > > > +identifier fname6 =~ ".*call_rcu.*";
> > > > +
> > > > +@@
> > > > +
> > > > +(
> > > > + atomic_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + atomic_dec_and_lock@p1(&(a)->x, ...)
> > > > +|
> > > > + atomic_long_dec_and_lock@p1(&(a)->x, ...)
> > > > +|
> > > > + atomic_long_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + atomic64_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + local_dec_and_test@p1(&(a)->x)
> > > > +)
> > > > +...
> > > > +(
> > > > + fname@p2(a, ...);
> > > > +|
> > > > + fname2@p2(...);
> > > > +|
> > > > + fname3@p2(...);
> > > > +|
> > > > + fname4@p2(...);
> > > > +|
> > > > + fname5@p2(...);
> > > > +|
> > > > + fname6@p2(...);
> > > > +)
> > > > +
> > > > +
> > > > +@script:python depends on report@
> > > > +p1 << r1.p1;
> > > > +p2 << r1.p2;
> > > > +@@
> > > > +msg = "atomic_dec_and_test variation before object free at line %s."
> > > > +coccilib.report.print_report(p1[0], msg % (p2[0].line))
> > > > +
> > > > +@r4 exists@
> > > > +identifier a, x, y;
> > > > +position p1, p2;
> > > > +identifier fname =~ ".*free.*";
> > > > +
> > > > +@@
> > > > +
> > > > +(
> > > > + atomic_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + atomic_dec_and_lock@p1(&(a)->x, ...)
> > > > +|
> > > > + atomic_long_dec_and_lock@p1(&(a)->x, ...)
> > > > +|
> > > > + atomic_long_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + atomic64_dec_and_test@p1(&(a)->x)
> > > > +|
> > > > + local_dec_and_test@p1(&(a)->x)
> > > > +)
> > > > +...
> > > > +y=a
> > > > +...
> > > > +fname@p2(y, ...);
> > > > +
> > > > +
> > > > +@script:python depends on report@
> > > > +p1 << r4.p1;
> > > > +p2 << r4.p2;
> > > > +@@
> > > > +msg = "atomic_dec_and_test variation before object free at line %s."
> > > > +coccilib.report.print_report(p1[0], msg % (p2[0].line))
> > > > +
> > > > +@r2 exists@
> > > > +identifier a, x;
> > > > +position p1;
> > > > +@@
> > > > +
> > > > +(
> > > > +atomic_add_unless(&(a)->x,-1,1)@p1
> > > > +|
> > > > +atomic_long_add_unless(&(a)->x,-1,1)@p1
> > > > +|
> > > > +atomic64_add_unless(&(a)->x,-1,1)@p1
> > > > +)
> > > > +
> > > > +@script:python depends on report@
> > > > +p1 << r2.p1;
> > > > +@@
> > > > +msg = "atomic_add_unless"
> > > > +coccilib.report.print_report(p1[0], msg)
> > > > +
> > > > +@r3 exists@
> > > > +identifier x;
> > > > +position p1;
> > > > +@@
> > > > +
> > > > +(
> > > > +x = atomic_add_return@p1(-1, ...);
> > > > +|
> > > > +x = atomic_long_add_return@p1(-1, ...);
> > > > +|
> > > > +x = atomic64_add_return@p1(-1, ...);
> > > > +)
> > > > +
> > > > +@script:python depends on report@
> > > > +p1 << r3.p1;
> > > > +@@
> > > > +msg = "x = atomic_add_return(-1, ...)"
> > > > +coccilib.report.print_report(p1[0], msg)
> > > > +
> > > > +
> > > > --
> > > > 2.7.4
> > > >
> > > >
> >
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v3 0/5] bq27xxx_battery data memory update

[Liam Breck]

On Thu, Aug 31, 2017 at 4:33 AM, Julia Lawall  wrote:
>
>
> On Thu, 31 Aug 2017, Liam Breck wrote:
>
>> On Thu, Aug 31, 2017 at 2:54 AM, Julia Lawall  wrote:
>> >
>> >
>> > On Tue, 29 Aug 2017, Liam Breck wrote:
>> >
>> >> Coccinelle folks,
>> >>
>> >> On Tue, Aug 29, 2017 at 5:29 PM, Sebastian Reichel
>> >>  wrote:
>> >> > Hi,
>> >> >
>> >> > On Tue, Aug 29, 2017 at 04:07:12PM -0700, Liam Breck wrote:
>> >> >> I don't see a Julia in CC list...
>> >> >
>> >> > <_< let's try that again.
>> >> >
>> >> >> On Tue, Aug 29, 2017 at 2:22 PM, Sebastian Reichel
>> >> >>  wrote:
>> >> >> > [adding Julia to Cc for Coccinelle question]
>> >> >> >
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Tue, Aug 29, 2017 at 10:31:57AM -0500, Andrew F. Davis wrote:
>> >> >> >> On 08/29/2017 05:54 AM, Sebastian Reichel wrote:
>> >> >> >> > On Wed, Aug 23, 2017 at 08:36:12PM -0700, Liam Breck wrote:
>> >> >> >> >> Overview:
>> >> >> >> >> * Reorganizes chip data definitions
>> >> >> >> >> * Enables features landed in these patches:
>> >> >> >> >>   dt-bindings: power: supply: bq27xxx: Add monitored-battery 
>> >> >> >> >> documentation
>> >> >> >> >>   power: supply: bq27xxx: Add chip data memory read/write support
>> >> >> >> >>   power: supply: bq27xxx: Add power_supply_battery_info support
>> >> >> >> >> * Supports the following chips (only BQ27425 is active)
>> >> >> >> >>   BQ27500, 545, 425, 421, 441, 621
>> >> >> >> >>
>> >> >> >> >> Changes in v3:
>> >> >> >> >> * BQ27425 tested; workaround minor chip bug
>> >> >> >> >> * Dropped driver_version
>> >> >> >> >> * Fixed dbg_dupes logic for .props & .dm_regs
>> >> >> >> >> * Dropped two props array dupes
>> >> >> >> >>
>> >> >> >> >> Changes in v2:
>> >> >> >> >> * Added di->opts flags for remaining chip features
>> >> >> >> >> * Commented out untested bq27xxx_dm_regs parameters
>> >> >> >> >> * Changed dbg_dupes to run only once
>> >> >> >> >>
>> >> >> >> >> Notes on v1:
>> >> >> >> >> * Not fully tested (hence RFC tag)
>> >> >> >> >
>> >> >> >> > Thanks, full series queued.
>> >> >> >> >
>> >> >> >> > -- Sebastian
>> >> >> >>
>> >> >> >> Anyway, I've not got the time to fight these changes anymore, but at
>> >> >> >> very least could you drop 4/5, it's static analysis code made into a
>> >> >> >> runtime check built into a kernel driver, if not at least add my
>> >> >> >> nacked-by. :)
>> >> >> >
>> >> >> > Since it's not critical at all and nobody depends on it, I dropped 
>> >> >> > 4/5
>> >> >> > for now. I agree, that checking it at runtime is not nice. On the 
>> >> >> > other
>> >> >> > hand I do think a duplication check makes sense. Doing a static
>> >> >> > check should be possible, but I have no idea how to implement this
>> >> >> > (without much effort). I suspect Coccinelle can do it, so I added
>> >> >> > Julia.
>> >> >> >
>> >> >> > For reference this is the runtime check:
>> >> >> > https://patchwork.kernel.org/patch/9918953/
>> >>
>> >> The data structures being checked start here:
>> >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n138
>> >>
>> >> And are aggregated here:
>> >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n743
>> >
>> > I worked a bit on this, but unfortunately there is a major parsing
>> > problem, and most of the definitions are ignored.  Specifically, the
>> > definitions of the regs variables are interspersed with eg:
>> >
>> > #define bq27510g1_regs bq27500_regs
>>
>> You can transform the macros with sed to either of:
>>
>> static u8 *bq27510g1_regs = 0 // skip comparison if x == 0
>>
>> static u8 bq27510g1_regs[BQ27XXX_REG_MAX] = { 0xFF } // skip
>> comparison if x[0] == 0xff
>>
>> Does that help?
>
> Not quite, because it's really a list of variable declarations, like
>
> int a, b, c, d;
>
> The following could be fine:
>
> *bq27510g1_regs = 0,

i forgot about the static u8 at the top of the array set. But yes,
just drop static u8 from either of the alternatives I gave.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Wed, Aug 30, 2017 at 03:05:04PM +0200, Julia Lawall wrote:
> On Wed, 30 Aug 2017, Ondřej Kuzník wrote:
>> OK, I understand. I any case, the suggestion below has very few false
>> negatives and no false positives that I could find, thanks a lot!
>>
>> The only false negatives are related to macro expansion, which probably
>> ties into the same limitation. It's strings like this:
>>
>> "string" BACKSQL_IDFMT "rest of string"
>>
>> Where BACKSQL_IDFMT is "%llu" or similar. Given how few of these there
>> are and that there have been no false positives from that that I could
>> see, I'm happy.
> 
> Maybe if you put this macro in with the others, then it will find it.

I already have, but I guess the Ocaml code doesn't have access to the
expanded version.

Moving onto the next step, I'm having problems with coccinelle rejecting
some patches for reasons I don't understand. For example it throws an
error applying the patch below even though some other files are fine and
the file in question parses fine without having to pass a builtin macro
file.

The patch tries to merge an snprintf and Debug if Debug is the only
consumer of that buffer plus removes the LogTest and buffer declaration
if appropriate.

This is the error I get on the file:

-8<--
HANDLING: servers/slapd/back-asyncmeta/bind.c
 
previous modification:
MINUS
  >>> Debug(L, merged, args_before, args, args_after);

According to environment 10:
   shortcut.p1 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p 
ld=%p rc=%d\n"
   shortcut_replace.args_after -> <>
   shortcut_replace.args_before -> <>
   shortcut_replace.L -> LDAP_DEBUG_ANY
   shortcut_replace.args -> <>
   shortcut.p1 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p 
ld=%p rc=%d\n"

current modification:
MINUS
According to environment 9:
   shortcut.p1 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p 
ld=%p rc=%d\n"
   shortcut_replace.L -> LDAP_DEBUG_ANY
   shortcut_replace.args_before -> <>
   shortcut_replace.args_after -> <>
   shortcut.p1 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> 
poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p 
ld=%p rc=%d\n"

shortcut_replace: already tagged token:
C code context
File "servers/slapd/back-asyncmeta/bind.c", line 1867, column 2, charpos = 45042
  around = 'Debug',
  whole content =   Debug( LDAP_DEBUG_ANY, "### %s %s\n",
-8<--

The patch in question:

-8<--
@initialize:python@
@@

#regex from 
https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
import re
fmtstring = '''\
(  # start of capture group 1
%  # literal "%"
(?:# first option
(?:[-+0 #]{0,5})   # optional flags
(?:\d+|\*)?# width
(?:\.(?:\d+|\*))?  # precision
(?:h|l|ll|w|I|I32|I64)?# size
[cCdiouxXeEfgGaAnpsSZ] # type
) |# OR
%%)# literal "%%"
'''

regex = re.compile(fmtstring, re.X)

def parse_format(f):
return tuple((m.span(), m.group()) for m in
regex.finditer(f))

def insert_at_pos(fmt, s, pos):
formats = parse_format(fmt)
span, format = formats[pos]
acc = fmt[:span[0]]
if s.startswith('"'):
acc += s[1:]
else:
acc += '" '
acc += s
if acc.endswith('"'):
acc = acc[:-1] + fmt[span[1]:]
else:
acc += ' "'
acc += fmt[span[1]:]
return acc

// covered by others but processing those can take hours on some files
@shortcut@
type T;
identifier buf;
expression I, E, L;
expression list args_before, args, args_after;
expression format1, format2;
position p1, p2;
@@

(
{
\( T buf = {...}; \| T buf = I; \| T buf; \)
snprintf@p1( buf, E, format1, args );
Debug@p2( L, format2, args_before, buf, args_after );
}
|
{
\( T buf = {...}; \| T buf = I; \| T buf; \| \)
snprintf@p1( buf, E, format1, args );
Debug@p2( L, 

Re: [Cocci] [PATCH v3 0/5] bq27xxx_battery data memory update

[Julia Lawall]

On Thu, 31 Aug 2017, Liam Breck wrote:

> On Thu, Aug 31, 2017 at 2:54 AM, Julia Lawall  wrote:
> >
> >
> > On Tue, 29 Aug 2017, Liam Breck wrote:
> >
> >> Coccinelle folks,
> >>
> >> On Tue, Aug 29, 2017 at 5:29 PM, Sebastian Reichel
> >>  wrote:
> >> > Hi,
> >> >
> >> > On Tue, Aug 29, 2017 at 04:07:12PM -0700, Liam Breck wrote:
> >> >> I don't see a Julia in CC list...
> >> >
> >> > <_< let's try that again.
> >> >
> >> >> On Tue, Aug 29, 2017 at 2:22 PM, Sebastian Reichel
> >> >>  wrote:
> >> >> > [adding Julia to Cc for Coccinelle question]
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > On Tue, Aug 29, 2017 at 10:31:57AM -0500, Andrew F. Davis wrote:
> >> >> >> On 08/29/2017 05:54 AM, Sebastian Reichel wrote:
> >> >> >> > On Wed, Aug 23, 2017 at 08:36:12PM -0700, Liam Breck wrote:
> >> >> >> >> Overview:
> >> >> >> >> * Reorganizes chip data definitions
> >> >> >> >> * Enables features landed in these patches:
> >> >> >> >>   dt-bindings: power: supply: bq27xxx: Add monitored-battery 
> >> >> >> >> documentation
> >> >> >> >>   power: supply: bq27xxx: Add chip data memory read/write support
> >> >> >> >>   power: supply: bq27xxx: Add power_supply_battery_info support
> >> >> >> >> * Supports the following chips (only BQ27425 is active)
> >> >> >> >>   BQ27500, 545, 425, 421, 441, 621
> >> >> >> >>
> >> >> >> >> Changes in v3:
> >> >> >> >> * BQ27425 tested; workaround minor chip bug
> >> >> >> >> * Dropped driver_version
> >> >> >> >> * Fixed dbg_dupes logic for .props & .dm_regs
> >> >> >> >> * Dropped two props array dupes
> >> >> >> >>
> >> >> >> >> Changes in v2:
> >> >> >> >> * Added di->opts flags for remaining chip features
> >> >> >> >> * Commented out untested bq27xxx_dm_regs parameters
> >> >> >> >> * Changed dbg_dupes to run only once
> >> >> >> >>
> >> >> >> >> Notes on v1:
> >> >> >> >> * Not fully tested (hence RFC tag)
> >> >> >> >
> >> >> >> > Thanks, full series queued.
> >> >> >> >
> >> >> >> > -- Sebastian
> >> >> >>
> >> >> >> Anyway, I've not got the time to fight these changes anymore, but at
> >> >> >> very least could you drop 4/5, it's static analysis code made into a
> >> >> >> runtime check built into a kernel driver, if not at least add my
> >> >> >> nacked-by. :)
> >> >> >
> >> >> > Since it's not critical at all and nobody depends on it, I dropped 4/5
> >> >> > for now. I agree, that checking it at runtime is not nice. On the 
> >> >> > other
> >> >> > hand I do think a duplication check makes sense. Doing a static
> >> >> > check should be possible, but I have no idea how to implement this
> >> >> > (without much effort). I suspect Coccinelle can do it, so I added
> >> >> > Julia.
> >> >> >
> >> >> > For reference this is the runtime check:
> >> >> > https://patchwork.kernel.org/patch/9918953/
> >>
> >> The data structures being checked start here:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n138
> >>
> >> And are aggregated here:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n743
> >
> > I worked a bit on this, but unfortunately there is a major parsing
> > problem, and most of the definitions are ignored.  Specifically, the
> > definitions of the regs variables are interspersed with eg:
> >
> > #define bq27510g1_regs bq27500_regs
>
> You can transform the macros with sed to either of:
>
> static u8 *bq27510g1_regs = 0 // skip comparison if x == 0
>
> static u8 bq27510g1_regs[BQ27XXX_REG_MAX] = { 0xFF } // skip
> comparison if x[0] == 0xff
>
> Does that help?

Not quite, because it's really a list of variable declarations, like

int a, b, c, d;

The following could be fine:

*bq27510g1_regs = 0,

julia



>
> > I'm not sure whether this would be convenient to fix, since parsing is
> > rather delicate.  Coccinelle doesn't run cpp first.  It tries to parse
> > macros definitions so that their code can be transformed as needed t the
> > place of the definition.
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] OpenLDAP debug statements

[Julia Lawall]

The problem is in the rule shortcut_replace.  You have:

(
-{
-\( T buf = {...}; \| T buf = I; \| T buf; \| \)
-snprintf@p1( buf, E, format1, args );
-Debug@p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
-}
|
{
-\( T buf = {...}; \| T buf = I; \| T buf; \| \)
-snprintf@p1( buf, E, format1, args );
-Debug@p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
}
|
-\( T buf = {...}; \| T buf = I; \| T buf; \| \)
-snprintf@p1( buf, E, format1, args );
-Debug@p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
)

There are three case (1, 2, and 3).  Normally a disjunction like this
matches from top to bottom, ie if the first case matches the ohers are not
considered.  Here however the patterns don' all start matching at the same
node.  The first two do, ie at an {, but the third does not.  So the third
will match over top of what was matched by the previous ones.  So there
will b multiple matches, and Coccinelle would prefer to do nothing rather
than to do something inconsistent.

As far as I can see, there is no real difference between the cases.  It
could probably be easier to just put the third case, and then have some
other rules to clean up any kinds of {} that occur.

I think that it should also be possible to define a metavariable

initializer i;

And then replace

T buf = {...}; \| T buf = I;

by T buf = i;

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] cocci script to add static to const declarations ?

[Julia Lawall]

On Wed, 30 Aug 2017, Joe Perches wrote:

> On Wed, 2017-08-30 at 23:41 +0200, Julia Lawall wrote:
> > New version.
>
> Thanks.
>
> fyi: This doesn't find const structs that could be static.
>
> Things like: drivers/gpu/drm/i915/selftests/i915_vma.c
> []
> static int igt_vma_rotate(void *arg)
> {
> []
>   const struct intel_rotation_plane_info planes[] = {
>   { .width = 1, .height = 1, .stride = 1 },
>   { .width = 2, .height = 2, .stride = 2 },
>   { .width = 4, .height = 4, .stride = 4 },
>   { .width = 8, .height = 8, .stride = 8 },
>
>   { .width = 3, .height = 5, .stride = 3 },
>   { .width = 3, .height = 5, .stride = 4 },
>   { .width = 3, .height = 5, .stride = 5 },
>
>   { .width = 5, .height = 3, .stride = 5 },
>   { .width = 5, .height = 3, .stride = 7 },
>   { .width = 5, .height = 3, .stride = 9 },
>
>   { .width = 4, .height = 6, .stride = 6 },
>   { .width = 6, .height = 4, .stride = 6 },
>   { }
>   }, *a, *b;

Here's a new version.  Unfortuntely, the ability of Coccinelle to break up
declarations of multiple variables like the above (planes, a, and b) is
rather limited.  So it doesn't get the above case.  But it does get some
others that have structures containing constants.  The new output is
attached.  You also need the latest version of Coccinelle from github.

julia

@initialize:ocaml@
@@

let diff(p,i) = not ((List.hd p).current_element = i)

@promising disable optional_storage@
position p,ok;
constant c;
type t;
identifier i,f;
@@

(
const t i@p[] = { ...,c@ok,..., };
|
const t i@p[] = { ...,{ .f = c@ok, },..., };
)

@worrisome@
position promising.p,bad != promising.ok;
expression e;
type t;
identifier i,f;
@@

(
const t i@p[] = { ...,e@bad,..., };
|
const t i@p[] = { ...,{ .f = e@bad, },..., };
)

@depends on !worrisome@
position promising.p : script:ocaml(promising.i) { diff(p,i) };
type t;
identifier i;
@@

+ static
  const t i@p[] = { ... };
diff -u -p a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -85,8 +85,8 @@ static FORCE_INLINE int LZ4_decompress_g
const BYTE * const lowLimit = lowPrefix - dictSize;
 
const BYTE * const dictEnd = (const BYTE *)dictStart + dictSize;
-   const unsigned int dec32table[] = { 0, 1, 2, 1, 4, 4, 4, 4 };
-   const int dec64table[] = { 0, 0, 0, -1, 0, 1, 2, 3 };
+   static const unsigned int dec32table[] = { 0, 1, 2, 1, 4, 4, 4, 4 };
+   static const int dec64table[] = { 0, 0, 0, -1, 0, 1, 2, 3 };
 
const int safeDecode = (endOnInput == endOnInputSize);
const int checkOffset = ((safeDecode) && (dictSize < (int)(64 * KB)));
diff -u -p a/lib/zstd/fse_compress.c b/lib/zstd/fse_compress.c
--- a/lib/zstd/fse_compress.c
+++ b/lib/zstd/fse_compress.c
@@ -618,7 +618,7 @@ size_t FSE_normalizeCount(short *normali
return ERROR(GENERIC); /* Too small tableLog, compression 
potentially impossible */
 
{
-   U32 const rtbTable[] = {0, 473195, 504333, 520860, 55, 
70, 75, 83};
+   static U32 const rtbTable[] = {0, 473195, 504333, 520860, 
55, 70, 75, 83};
U64 const scale = 62 - tableLog;
U64 const step = div_u64((U64)1 << 62, (U32)total); /* <== 
here, one division ! */
U64 const vStep = 1ULL << (scale - 20);
diff -u -p a/lib/test_printf.c b/lib/test_printf.c
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -389,7 +389,7 @@ static void __init
 bitmap(void)
 {
DECLARE_BITMAP(bits, 20);
-   const int primes[] = {2,3,5,7,11,13,17,19};
+   static const int primes[] = {2,3,5,7,11,13,17,19};
int i;
 
bitmap_zero(bits, 20);
diff -u -p a/drivers/power/supply/cpcap-battery.c 
b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -620,7 +620,7 @@ static int cpcap_battery_init_irq(struct
 static int cpcap_battery_init_interrupts(struct platform_device *pdev,
 struct cpcap_battery_ddata *ddata)
 {
-   const char * const cpcap_battery_irqs[] = {
+   static const char * const cpcap_battery_irqs[] = {
"eol", "lowbph", "lowbpl",
"chrgcurr1", "battdetb"
};
diff -u -p a/drivers/clk/microchip/clk-pic32mzda.c 
b/drivers/clk/microchip/clk-pic32mzda.c
--- a/drivers/clk/microchip/clk-pic32mzda.c
+++ b/drivers/clk/microchip/clk-pic32mzda.c
@@ -156,7 +156,7 @@ static int pic32_fscm_nmi(struct notifie
 
 static int pic32mzda_clk_probe(struct platform_device *pdev)
 {
-   const char *const pll_mux_parents[] = {"posc_clk", "frc_clk"};
+   static const char *const pll_mux_parents[] = {"posc_clk", "frc_clk"};
struct device_node *np = pdev->dev.of_node;
struct 

Re: [Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Thu, Aug 31, 2017 at 04:09:30PM +0200, Julia Lawall wrote:
> The problem is in the rule shortcut_replace.  You have:
> 
> [...]
> 
> There are three case (1, 2, and 3).  Normally a disjunction like this
> matches from top to bottom, ie if the first case matches the ohers are not
> considered.  Here however the patterns don' all start matching at the same
> node.  The first two do, ie at an {, but the third does not.  So the third
> will match over top of what was matched by the previous ones.  So there
> will b multiple matches, and Coccinelle would prefer to do nothing rather
> than to do something inconsistent.
> 
> As far as I can see, there is no real difference between the cases.  It
> could probably be easier to just put the third case, and then have some
> other rules to clean up any kinds of {} that occur.

Yes, that has worked quite well actually. It has caused one problem,
though, splitting it up has made coccinelle strip curly brackets all
over the place, which is a bit problematic in places like this:

if (...) {
/* comment */
Debug( ... );
}

turning it into something very hard to read, especially if there is an
else branch:

if (...)
/* comment */
Debug( ... );

So far, nothing I've tried has helped limiting this, those include:
- pinning it on "+Debug@p3" which coccinelle rejects.
- using the below, probably because of your observation above
(
 if (...) {
 Debug(...);
 }
|
-{
 Debug(...);
-}
)
- the above but splitting that in two rules and a remembered location

The current patch I'm using is:
-8<--
@initialize:python@
@@

#regex from 
https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
import re
fmtstring = '''\
(  # start of capture group 1
%  # literal "%"
(?:# first option
(?:[-+0 #]{0,5})   # optional flags
(?:\d+|\*)?# width
(?:\.(?:\d+|\*))?  # precision
(?:h|l|ll|w|I|I32|I64)?# size
[cCdiouxXeEfgGaAnpsSZ] # type
) |# OR
%%)# literal "%%"
'''

regex = re.compile(fmtstring, re.X)

def parse_format(f):
return tuple((m.span(), m.group()) for m in
regex.finditer(f))

def insert_at_pos(fmt, s, pos):
formats = parse_format(fmt)
span, format = formats[pos]
acc = fmt[:span[0]]
if s.startswith('"'):
acc += s[1:]
else:
acc += '" '
acc += s
if acc.endswith('"'):
acc = acc[:-1] + fmt[span[1]:]
else:
acc += ' "'
acc += fmt[span[1]:]
return acc

// covered by others but processing that can take hours on some files
@shortcut@
identifier buf;
expression E, L;
expression list args_before, args, args_after;
expression format1, format2;
position p1, p2;
@@

snprintf@p1( buf, E, format1, args );
Debug@p2( L, format2, args_before, buf, args_after );

@script:python shortcut_process@
format1 << shortcut.format1;
format2 << shortcut.format2;
args_before << shortcut.args_before;
merged;
@@

pos = len(args_before.elements)
coccinelle.merged = insert_at_pos(format2, format1, pos)

@shortcut_replace@
position shortcut.p1, shortcut.p2;
identifier shortcut_process.merged;

identifier buf;
expression E, L;
expression list args_before, args, args_after;
expression format1, format2;
@@

-snprintf@p1( buf, E, format1, args );
-Debug@p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );

@@
type T;
initializer I;
@@
{
-\( T buf = I; \| T buf; \)
 Debug( ... );
}

@@
@@
-{
 Debug( ... );
-}

@useless_if@
expression L;
@@

-if ( LogTest( L ) ) {
 Debug( L, ... );
-}
-8<--

> I think that it should also be possible to define a metavariable
> 
> initializer i;
> 
> And then replace
> 
> T buf = {...}; \| T buf = I;
> 
> by T buf = i;

That does work, thanks.

-- 
Ondřej Kuzník
Senior Software Engineer
Symas Corporation   http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] OpenLDAP debug statements

[Julia Lawall]

On Thu, 31 Aug 2017, Ondřej Kuzník wrote:

> On Thu, Aug 31, 2017 at 06:01:07PM +0200, Julia Lawall wrote:
> > On Thu, 31 Aug 2017, Ondřej Kuzník wrote:
> >> On Thu, Aug 31, 2017 at 05:52:14PM +0200, Julia Lawall wrote:
> >>> So you want to drop the braces if there is no comment and keep the brace
> >>> if there is a comment?  You can't match on comments.  The only thing you
> >>> could do is check the number of lines between the { and Debug.
> >>
> >> Something slightly different, I want to remove the braces only if they
> >> do not "belong" to an if statement.
> >
> > Does that occur?
> >
> > In any case, just do:
> >
> > @r@
> > position p1;
> > statement S;
> > @@
> >
> > if (...) {@p1  Debug(...); } else S
> >
> > @@
> > position p1 != r.p1;
> > @@
> >
> > -{@p1
> >   Debug(...);
> > -}
>
> I have tried the following:
>
> @guard@
> position p;
> @@
>
> if ( ... ) {@p
>  Debug( ... );
> }

You didn't put the else S.

If you add it, the rule will still match cases with no else.

julia

>
> @@
> position p != guard.p;
> @@
> -{@p
>  Debug( ... );
> -}
>
> But it still modifies servers/slapd/back-asyncmeta/bind.c:879
>
> --
> Ondřej Kuzník
> Senior Software Engineer
> Symas Corporation   http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] OpenLDAP debug statements

[Julia Lawall]

On Thu, 31 Aug 2017, Ondřej Kuzník wrote:

> On Thu, Aug 31, 2017 at 04:09:30PM +0200, Julia Lawall wrote:
> > The problem is in the rule shortcut_replace.  You have:
> >
> > [...]
> >
> > There are three case (1, 2, and 3).  Normally a disjunction like this
> > matches from top to bottom, ie if the first case matches the ohers are not
> > considered.  Here however the patterns don' all start matching at the same
> > node.  The first two do, ie at an {, but the third does not.  So the third
> > will match over top of what was matched by the previous ones.  So there
> > will b multiple matches, and Coccinelle would prefer to do nothing rather
> > than to do something inconsistent.
> >
> > As far as I can see, there is no real difference between the cases.  It
> > could probably be easier to just put the third case, and then have some
> > other rules to clean up any kinds of {} that occur.
>
> Yes, that has worked quite well actually. It has caused one problem,
> though, splitting it up has made coccinelle strip curly brackets all
> over the place, which is a bit problematic in places like this:
>
> if (...) {
> /* comment */
> Debug( ... );
> }
>
> turning it into something very hard to read, especially if there is an
> else branch:
>
> if (...)
> /* comment */
> Debug( ... );

So you want to drop the braces if there is no comment and keep the brace
if there is a comment?  You can't match on comments.  The only thing you
could do is check the number of lines between the { and Debug.

@r@
position p1,p2;
@@

{@p1
Debug@p2(...)
}

@script:python@
p1 << r.p1;
p2 << r.p2;
@@

if p2[0].line - p1.line >= 2:
   cocci.include_match(False)

@@
position r.p1,r.p2;
@@

-{@p1
Debug@p2(...)
-}

julia


>
> So far, nothing I've tried has helped limiting this, those include:
> - pinning it on "+Debug@p3" which coccinelle rejects.
> - using the below, probably because of your observation above
> (
>  if (...) {
>  Debug(...);
>  }
> |
> -{
>  Debug(...);
> -}
> )
> - the above but splitting that in two rules and a remembered location
>
> The current patch I'm using is:
> -8<--
> @initialize:python@
> @@
>
> #regex from 
> https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
> import re
> fmtstring = '''\
> (  # start of capture group 1
> %  # literal "%"
> (?:# first option
> (?:[-+0 #]{0,5})   # optional flags
> (?:\d+|\*)?# width
> (?:\.(?:\d+|\*))?  # precision
> (?:h|l|ll|w|I|I32|I64)?# size
> [cCdiouxXeEfgGaAnpsSZ] # type
> ) |# OR
> %%)# literal "%%"
> '''
>
> regex = re.compile(fmtstring, re.X)
>
> def parse_format(f):
> return tuple((m.span(), m.group()) for m in
> regex.finditer(f))
>
> def insert_at_pos(fmt, s, pos):
> formats = parse_format(fmt)
> span, format = formats[pos]
> acc = fmt[:span[0]]
> if s.startswith('"'):
> acc += s[1:]
> else:
> acc += '" '
> acc += s
> if acc.endswith('"'):
> acc = acc[:-1] + fmt[span[1]:]
> else:
> acc += ' "'
> acc += fmt[span[1]:]
> return acc
>
> // covered by others but processing that can take hours on some files
> @shortcut@
> identifier buf;
> expression E, L;
> expression list args_before, args, args_after;
> expression format1, format2;
> position p1, p2;
> @@
>
> snprintf@p1( buf, E, format1, args );
> Debug@p2( L, format2, args_before, buf, args_after );
>
> @script:python shortcut_process@
> format1 << shortcut.format1;
> format2 << shortcut.format2;
> args_before << shortcut.args_before;
> merged;
> @@
>
> pos = len(args_before.elements)
> coccinelle.merged = insert_at_pos(format2, format1, pos)
>
> @shortcut_replace@
> position shortcut.p1, shortcut.p2;
> identifier shortcut_process.merged;
>
> identifier buf;
> expression E, L;
> expression list args_before, args, args_after;
> expression format1, format2;
> @@
>
> -snprintf@p1( buf, E, format1, args );
> -Debug@p2( L, format2, args_before, buf, args_after );
> +Debug( L, merged, args_before, args, args_after );
>
> @@
> type T;
> initializer I;
> @@
> {
> -\( T buf = I; \| T buf; \)
>  Debug( ... );
> }
>
> @@
> @@
> -{
>  Debug( ... );
> -}
>
> @useless_if@
> expression L;
> @@
>
> -if ( LogTest( L ) ) {
>  Debug( L, ... );
> -}
> -8<--
>
> > I think that it should also be possible to define a metavariable
> >
> > initializer i;
> >
> > And then replace
> >
> > T buf = {...}; \| T buf = I;
> >
> > by T buf = i;
>
> That does work, thanks.
>
> --
> Ondřej Kuzník
> Senior Software Engineer
> Symas Corporation   http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>___
Cocci mailing list
Cocci@systeme.lip6.fr

Re: [Cocci] cocci script to add static to const declarations ?

[Joe Perches]

On Thu, 2017-08-31 at 16:22 +0200, Julia Lawall wrote:
> On Wed, 30 Aug 2017, Joe Perches wrote:
> > fyi: This doesn't find const structs that could be static.
> > 
> > Things like: drivers/gpu/drm/i915/selftests/i915_vma.c
> > []
> > static int igt_vma_rotate(void *arg)
> > {
> > []
> > const struct intel_rotation_plane_info planes[] = {
> > { .width = 1, .height = 1, .stride = 1 },
[]
> > { }
> > }, *a, *b;
> 
> Here's a new version.  Unfortuntely, the ability of Coccinelle to break up
> declarations of multiple variables like the above (planes, a, and b) is
> rather limited.  So it doesn't get the above case.  But it does get some
> others that have structures containing constants.  The new output is
> attached.  You also need the latest version of Coccinelle from github.

Thanks again.  This looks very good to me.

Could you (or one of your minions) please submit these?

gcc doesn't currently optimize these declarations into
.text sections and instead places these bits onto stack
along with initialization code for these bits.

With the addition of static, the object code is smaller.

And btw:

Some of the output could still be improved by addition
of even more static markers.

For instance:

diff -u -p a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -627,7 +627,7 @@ static void hdmi_dbg_sta(struct seq_file
 static void hdmi_dbg_sw_di_cfg(struct seq_file *s, int val)
 {
int tmp;
-   char *const en_di[] = {"no transmission",
+   static char *const en_di[] = {"no transmission",
   "single transmission",
   "once every field",
   "once every frame"};

Perhaps this would be nicer as

static const char * const en_di[] = {...

Although that is a very small nit indeed.

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] cocci script to add static to const declarations ?

[Julia Lawall]

On Thu, 31 Aug 2017, Joe Perches wrote:

> On Thu, 2017-08-31 at 16:22 +0200, Julia Lawall wrote:
> > On Wed, 30 Aug 2017, Joe Perches wrote:
> > > fyi: This doesn't find const structs that could be static.
> > >
> > > Things like: drivers/gpu/drm/i915/selftests/i915_vma.c
> > > []
> > > static int igt_vma_rotate(void *arg)
> > > {
> > > []
> > >   const struct intel_rotation_plane_info planes[] = {
> > >   { .width = 1, .height = 1, .stride = 1 },
> []
> > >   { }
> > >   }, *a, *b;
> >
> > Here's a new version.  Unfortuntely, the ability of Coccinelle to break up
> > declarations of multiple variables like the above (planes, a, and b) is
> > rather limited.  So it doesn't get the above case.  But it does get some
> > others that have structures containing constants.  The new output is
> > attached.  You also need the latest version of Coccinelle from github.
>
> Thanks again.  This looks very good to me.
>
> Could you (or one of your minions) please submit these?

I think there was a discussion recently that suggested that one should
just submit a patch to gcc instead of 100 patches to the Linux kernel?

> gcc doesn't currently optimize these declarations into
> .text sections and instead places these bits onto stack
> along with initialization code for these bits.
>
> With the addition of static, the object code is smaller.
>
> And btw:
>
> Some of the output could still be improved by addition
> of even more static markers.
>
> For instance:
>
> diff -u -p a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -627,7 +627,7 @@ static void hdmi_dbg_sta(struct seq_file
>  static void hdmi_dbg_sw_di_cfg(struct seq_file *s, int val)
>  {
> int tmp;
> -   char *const en_di[] = {"no transmission",
> +   static char *const en_di[] = {"no transmission",
>    "single transmission",
>    "once every field",
>    "once every frame"};
>
> Perhaps this would be nicer as
>
>   static const char * const en_di[] = {...
>
> Although that is a very small nit indeed.

I'm not sure what you are suggesting here.  Are there some cases where an
opportunity for static is missed?

thanks,
julia___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] cocci script to add static to const declarations ?

[Joe Perches]

On Thu, 2017-08-31 at 21:25 +0200, Julia Lawall wrote:
> On Thu, 31 Aug 2017, Joe Perches wrote:
> > On Thu, 2017-08-31 at 16:22 +0200, Julia Lawall wrote:
> > > On Wed, 30 Aug 2017, Joe Perches wrote:
> > > > fyi: This doesn't find const structs that could be static.
> > > > 
> > > > Things like: drivers/gpu/drm/i915/selftests/i915_vma.c
> > > > []
> > > > static int igt_vma_rotate(void *arg)
> > > > {
> > > > []
> > > > const struct intel_rotation_plane_info planes[] = {
> > > > { .width = 1, .height = 1, .stride = 1 },
> > 
> > []
> > > > { }
> > > > }, *a, *b;
> > > 
> > > Here's a new version.  Unfortuntely, the ability of Coccinelle to break up
> > > declarations of multiple variables like the above (planes, a, and b) is
> > > rather limited.  So it doesn't get the above case.  But it does get some
> > > others that have structures containing constants.  The new output is
> > > attached.  You also need the latest version of Coccinelle from github.
> > 
> > Thanks again.  This looks very good to me.
> > 
> > Could you (or one of your minions) please submit these?
> 
> I think there was a discussion recently that suggested that one should
> just submit a patch to gcc instead of 100 patches to the Linux kernel?

That's of course a good idea, but older versions
of gcc (4,5,6, and probably 7 too) would not be
fixed.

> > gcc doesn't currently optimize these declarations into
> > .text sections and instead places these bits onto stack
> > along with initialization code for these bits.
> > 
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH 01/31] coccinelle: Improve setup_timer.cocci matching

[Kees Cook]

This improves the patch mode of setup_timer.cocci. Several patterns
were missing:
 - assignments-before-init_timer() cases
 - limit the .data case removal to the specific struct timer_list instance
 - handling calls by dereference (timer->field vs timer.field)

Cc: Julia Lawall 
Cc: Gilles Muller 
Cc: Nicolas Palix 
Cc: Michal Marek 
Cc: cocci@systeme.lip6.fr
Signed-off-by: Kees Cook 
---
 scripts/coccinelle/api/setup_timer.cocci | 129 +--
 1 file changed, 105 insertions(+), 24 deletions(-)

diff --git a/scripts/coccinelle/api/setup_timer.cocci 
b/scripts/coccinelle/api/setup_timer.cocci
index eb6bd9e4ab1a..279767f3bbef 100644
--- a/scripts/coccinelle/api/setup_timer.cocci
+++ b/scripts/coccinelle/api/setup_timer.cocci
@@ -2,6 +2,7 @@
 /// and data fields
 // Confidence: High
 // Copyright: (C) 2016 Vaishali Thakkar, Oracle. GPLv2
+// Copyright: (C) 2017 Kees Cook, Google. GPLv2
 // Options: --no-includes --include-headers
 // Keywords: init_timer, setup_timer
 
@@ -10,60 +11,123 @@ virtual context
 virtual org
 virtual report
 
+// Match the common cases first to avoid Coccinelle parsing loops with
+// "... when" clauses.
+
 @match_immediate_function_data_after_init_timer
 depends on patch && !context && !org && !report@
 expression e, func, da;
 @@
 
--init_timer ();
-+setup_timer (, func, da);
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+(
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+
+@match_immediate_function_data_before_init_timer
+depends on patch && !context && !org && !report@
+expression e, func, da;
+@@
 
 (
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+
+@match_function_and_data_after_init_timer
+depends on patch && !context && !org && !report@
+expression e, e2, e3, e4, e5, func, da;
+@@
+
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+ ... when != func = e2
+ when != da = e3
+(
 -e.function = func;
+... when != da = e4
 -e.data = da;
 |
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
 -e.data = da;
+... when != func = e5
 -e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
 )
 
-@match_function_and_data_after_init_timer
+@match_function_and_data_before_init_timer
 depends on patch && !context && !org && !report@
-expression e1, e2, e3, e4, e5, a, b;
+expression e, e2, e3, e4, e5, func, da;
 @@
-
--init_timer ();
-+setup_timer (, a, b);
-
-... when != a = e2
-when != b = e3
 (
--e1.function = a;
-... when != b = e4
--e1.data = b;
+-e.function = func;
+... when != da = e4
+-e.data = da;
 |
--e1.data = b;
-... when != a = e5
--e1.function = a;
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
+-e.data = da;
+... when != func = e5
+-e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
 )
+... when != func = e2
+when != da = e3
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
 
 @r1 exists@
+expression t;
 identifier f;
 position p;
 @@
 
 f(...) { ... when any
-  init_timer@p(...)
+  init_timer@p(\(\|t\))
   ... when any
 }
 
 @r2 exists@
+expression r1.t;
 identifier g != r1.f;
-struct timer_list t;
 expression e8;
 @@
 
 g(...) { ... when any
-  t.data = e8
+  \(t.data\|t->data\) = e8
   ... when any
 }
 
@@ -77,14 +141,31 @@ p << r1.p;
 cocci.include_match(False)
 
 @r3 depends on patch && !context && !org && !report@
-expression e6, e7, c;
+expression r1.t, func, e7;
 position r1.p;
 @@
 
--init_timer@p ();
-+setup_timer (, c, 0UL);
-... when != c = e7
--e6.function = c;
+(
+-init_timer@p();
++setup_timer(, func, 0UL);
+... when != func = e7
+-t.function = func;
+|
+-t.function = func;
+... when != func = e7
+-init_timer@p();
++setup_timer(, func, 0UL);
+|
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+... when != func = e7
+-t->function = func;
+|
+-t->function = func;
+... when != func = e7
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+)
 
 // 
 
-- 
2.7.4

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci