[Cocci] Parse error with genl-const.cocci

[Johannes Berg]

+cocci list

Hi,

Looks like you made a really deep investigation here - awesome, thanks
for doing that!


On Thu, 2017-11-30 at 21:43 -0800, Remington Furman wrote:
>
> Full log using genl-const.cocci
> init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
> meta: parse error:
> File "./patches/0027-genl-const/genl-const.cocci", line 2, column 10,
> charpos = 13
> around = '__genl_const',
> whole content = attribute __genl_const;
> [...snip callstack...]

FWIW, the patch is pretty simple:

@@
attribute __genl_const;
@@
(
-const struct genl_multicast_group
+__genl_const struct genl_multicast_group
|
-const struct genl_ops
+__genl_const struct genl_ops
)

(yes, that's the full patch)


> This is very similar to this problem:
> <https://www.spinics.net/lists/backports/msg04216.html>
> The conclusion of that thread is that the --integrate workflow is
> no longer supported.
>
> However, I am not using --integrate, and I am still getting
> errors about parsing '__genl_const'.

Yeah this --integrate is unrelated to the problem - the problem is that
spatch can't parse this patch (any more).

> Is that a bug in the patch
> or possibly a regression in spatch?

I think it's a regression in spatch.

> I've tried v4.14-rc2 also,
> but nothing older.

That's a kernel version so not really relevant - spatch doesn't even
attempt to apply the changes since it can't parse them.

> This page has a lot of great example info, and suggested that it
> could be a problem with my version of spatch. Specifically, that
> I was missing the menhir and libmenhir-ocaml-dev packages when
> building Coccinelle and a rebuild should help.
> <http://stonyslp.blogspot.com/2017/08/backport-driver.html>

That I don't know - I've never built spatch myself.

> Unfortunately, I still have the same errors after running make
> distclean and rebuilding.
>
> I have:
> coccinelle $ git describe --tags
> 1.0.6-365-gd73c6e6
> coccinelle $ spatch --version
> spatch version 1.0.6-00365-gd73c6e6 compiled with OCaml version 4.01.0
> Flags passed to the configure script: [none]
> OCaml scripting support: yes
> Python scripting support: yes
> Syntax of regular expresssions: PCRE

That seems reasonable as far as configuration is concerned.

> I found this patch which describes how older versions can't
> handle the __genl_const changes (20150209):
> <https://www.spinics.net/lists/backports/msg03223.html>
>
> But, I also see commit 748116c8 from shortly after (20150507)
> which introduced genl-const.cocci and says only spatch 1.0.0-rc23
> is needed.

Right.

> I feel like I'm close to figuring this out, but would appreciate
> any help or pointers.

Yeah I think you're pretty close.

Can you try obtaining/building spatch 1.0.4? That definitely works for
me, so I think there's probably a regression in spatch somewhere
between 1.0.4 and the version that you're running.

johannes

[Julia Lawall]

On Fri, 1 Dec 2017, Johannes Berg wrote:

> +cocci list
>
> Hi,
>
> Looks like you made a really deep investigation here - awesome, thanks
> for doing that!

Hello,

The attribute metavariable declaration has become attribute name, to be
more like eg declarer name, iterator name, etc.

The handling of attributes has also been extended so that you can actually
match against them in some cases, such as on function declarations. On
the oher hand, nothing was done to allow attributes in front of types that
are alone.

I can try to fix this. In the short term, perhaps using 1.0.4 is
acceptable.

julia

>
>
> On Thu, 2017-11-30 at 21:43 -0800, Remington Furman wrote:
> >
> > Full log using genl-const.cocci
> > init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
> > meta: parse error:
> > File "./patches/0027-genl-const/genl-const.cocci", line 2, column 10,
> > charpos = 13
> > around = '__genl_const',
> > whole content = attribute __genl_const;
> > [...snip callstack...]
>
> FWIW, the patch is pretty simple:
>
> @@
> attribute __genl_const;
> @@
> (
> -const struct genl_multicast_group
> +__genl_const struct genl_multicast_group
> |
> -const struct genl_ops
> +__genl_const struct genl_ops
> )
>
> (yes, that's the full patch)
>
>
> > This is very similar to this problem:
> > <https://www.spinics.net/lists/backports/msg04216.html>
> > The conclusion of that thread is that the --integrate workflow is
> > no longer supported.
> >
> > However, I am not using --integrate, and I am still getting
> > errors about parsing '__genl_const'.
>
> Yeah this --integrate is unrelated to the problem - the problem is that
> spatch can't parse this patch (any more).
>
> > Is that a bug in the patch
> > or possibly a regression in spatch?
>
> I think it's a regression in spatch.
>
> > I've tried v4.14-rc2 also,
> > but nothing older.
>
> That's a kernel version so not really relevant - spatch doesn't even
> attempt to apply the changes since it can't parse them.
>
> > This page has a lot of great example info, and suggested that it
> > could be a problem with my version of spatch. Specifically, that
> > I was missing the menhir and libmenhir-ocaml-dev packages when
> > building Coccinelle and a rebuild should help.
> > <http://stonyslp.blogspot.com/2017/08/backport-driver.html>
>
> That I don't know - I've never built spatch myself.
>
> > Unfortunately, I still have the same errors after running make
> > distclean and rebuilding.
> >
> > I have:
> > coccinelle $ git describe --tags
> > 1.0.6-365-gd73c6e6
> > coccinelle $ spatch --version
> > spatch version 1.0.6-00365-gd73c6e6 compiled with OCaml version 4.01.0
> > Flags passed to the configure script: [none]
> > OCaml scripting support: yes
> > Python scripting support: yes
> > Syntax of regular expresssions: PCRE
>
> That seems reasonable as far as configuration is concerned.
>
> > I found this patch which describes how older versions can't
> > handle the __genl_const changes (20150209):
> > <https://www.spinics.net/lists/backports/msg03223.html>
> >
> > But, I also see commit 748116c8 from shortly after (20150507)
> > which introduced genl-const.cocci and says only spatch 1.0.0-rc23
> > is needed.
>
> Right.
>
> > I feel like I'm close to figuring this out, but would appreciate
> > any help or pointers.
>
> Yeah I think you're pretty close.
>
> Can you try obtaining/building spatch 1.0.4? That definitely works for
> me, so I think there's probably a regression in spatch somewhere
> between 1.0.4 and the version that you're running.
>
> johannes
> _______________________________________________
> Cocci mailing list
> ***@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[Johannes Berg]

Hi Julia,

On Fri, 2017-12-01 at 10:18 +0100, Julia Lawall wrote:
>
> The attribute metavariable declaration has become attribute name, to be
> more like eg declarer name, iterator name, etc.

So we have to write

attribute name __genl_const;

right?

> The handling of attributes has also been extended so that you can actually
> match against them in some cases, such as on function declarations. On
> the oher hand, nothing was done to allow attributes in front of types that
> are alone.

Very cool!

> I can try to fix this. In the short term, perhaps using 1.0.4 is
> acceptable.

To me that's OK, I'll try to keep it in mind.

Can we do some "#ifdef" type syntax in spatch on the version of it? :-)

I guess I could also have two different versions of the patch and write
some code to pick up which one to use, but that's a bit awkward ...

But let me know if you can't actually fix this (easily), then I can do
that instead.

johannes

[Julia Lawall]

On Fri, 1 Dec 2017, Johannes Berg wrote:

> Hi Julia,
>
> On Fri, 2017-12-01 at 10:18 +0100, Julia Lawall wrote:
> >
> > The attribute metavariable declaration has become attribute name, to be
> > more like eg declarer name, iterator name, etc.
>
> So we have to write
>
> attribute name __genl_const;
>
> right?

Yes.

> > The handling of attributes has also been extended so that you can actually
> > match against them in some cases, such as on function declarations. On
> > the oher hand, nothing was done to allow attributes in front of types that
> > are alone.
>
> Very cool!
>
> > I can try to fix this. In the short term, perhaps using 1.0.4 is
> > acceptable.
>
> To me that's OK, I'll try to keep it in mind.
>
> Can we do some "#ifdef" type syntax in spatch on the version of it? :-)

There's nothing for that available at the moment.

> I guess I could also have two different versions of the patch and write
> some code to pick up which one to use, but that's a bit awkward ...
>
> But let me know if you can't actually fix this (easily), then I can do
> that instead.

OK, I'll try to look at it soon.

julia

[Remington Furman]

On 12/01/2017 02:00 AM, Julia Lawall wrote:
> On Fri, 1 Dec 2017, Johannes Berg wrote:
>> Hi Julia,
>>
>> On Fri, 2017-12-01 at 10:18 +0100, Julia Lawall wrote:
>>> The handling of attributes has also been extended so that you can actually
>>> match against them in some cases, such as on function declarations. On
>>> the oher hand, nothing was done to allow attributes in front of types that
>>> are alone.
>> Very cool!
>>> I can try to fix this. In the short term, perhaps using 1.0.4 is
>>> acceptable.
>> To me that's OK, I'll try to keep it in mind.
>>
>> Can we do some "#ifdef" type syntax in spatch on the version of it? :-)
> There's nothing for that available at the moment.
>> I guess I could also have two different versions of the patch and write
>> some code to pick up which one to use, but that's a bit awkward ...
>>
>> But let me know if you can't actually fix this (easily), then I can do
>> that instead.
> OK, I'll try to look at it soon.
>
> julia
I thought I'd point out the layers of coevolution that result from the
changing spatch behavior.  :) It's a bit fractal when every layer in the
dependency graph has the possibility to change.  I suppose a #ifdef type
solution might remove the potential need to backport the .cocci patches
themselves going forward.

But, I think it might be sufficient to just document what version of the
tools, in this case spatch, were used for a particular backports
commit/release.  It's easy enough to build any version of spatch from
git.  Or perhaps each .cocci patch could have a comment listing the
spatch version it was developed with.  Future backports releases could
then update the patch syntax as needed, document the spatch version, and
leave it at that.

-Remington

[Julia Lawall]

On Tue, 12 Dec 2017, Remington Furman wrote:

> On 12/01/2017 02:00 AM, Julia Lawall wrote:
> > On Fri, 1 Dec 2017, Johannes Berg wrote:
> > > Hi Julia,
> > >
> > > On Fri, 2017-12-01 at 10:18 +0100, Julia Lawall wrote:
> > > > The handling of attributes has also been extended so that you can
> > > > actually
> > > > match against them in some cases, such as on function declarations. On
> > > > the oher hand, nothing was done to allow attributes in front of types
> > > > that
> > > > are alone.
> > > Very cool!
> > > > I can try to fix this. In the short term, perhaps using 1.0.4 is
> > > > acceptable.
> > > To me that's OK, I'll try to keep it in mind.
> > >
> > > Can we do some "#ifdef" type syntax in spatch on the version of it? :-)
> > There's nothing for that available at the moment.
> > > I guess I could also have two different versions of the patch and write
> > > some code to pick up which one to use, but that's a bit awkward ...
> > >
> > > But let me know if you can't actually fix this (easily), then I can do
> > > that instead.
> > OK, I'll try to look at it soon.
> >
> > julia
> I thought I'd point out the layers of coevolution that result from the
> changing spatch behavior.  :) It's a bit fractal when every layer in the
> dependency graph has the possibility to change.  I suppose a #ifdef type
> solution might remove the potential need to backport the .cocci patches
> themselves going forward.
>
> But, I think it might be sufficient to just document what version of the
> tools, in this case spatch, were used for a particular backports
> commit/release.  It's easy enough to build any version of spatch from git.  Or
> perhaps each .cocci patch could have a comment listing the spatch version it
> was developed with.  Future backports releases could then update the patch
> syntax as needed, document the spatch version, and leave it at that.

Hello,

I updated the github version of Coccinelle such that
patches/0027-genl-const/genl-const.cocci
will almost parse correctly. The only thing that remains is to change
attribute into attribute name.

julia

[Johannes Berg]

On Thu, 2017-12-14 at 16:16 +0100, Julia Lawall wrote:

> I updated the github version of Coccinelle such that
> patches/0027-genl-const/genl-const.cocci
> will almost parse correctly. The only thing that remains is to change
> attribute into attribute name.

Ok, thanks, I'll keep that in mind when I update the version. It's a b
it unfortunate you couldn't get it to be completely backwards
compatible, but we'll deal with it :)

johannes

[Johannes Berg]

Julia,

> I updated the github version of Coccinelle such that
> patches/0027-genl-const/genl-const.cocci
> will almost parse correctly. The only thing that remains is to change
> attribute into attribute name.

I'm not sure why, but for some reason I thought back then (https://git.k
ernel.org/pub/scm/linux/kernel/git/backports/backports.git/commit/?id=98
272f479c2126a135dfcb12484e93d5888164ab) that the following would work
with 1.0.6:

@@
attribute name __genl_const;
@@
(
-const struct genl_multicast_group
+__genl_const struct genl_multicast_group
|
-const struct genl_ops
+__genl_const struct genl_ops
)



However, Hauke pointed out it doesn't, and I can confirm. On 1.0.6, I
can't get this to work at all, it refuses to parse "attribute name" and
it refuses to parse the patch part with just "attribute".

Is there any way to get this to work again?

johannes

[Johannes Berg]

On Fri, 2017-12-01 at 09:57 -0800, Remington Furman wrote:
>
> Ok, I've rebuilt with 1.0.4, and it applied genl-const.cocci just
> fine. But, later on my CPU was held at 100% for 29 minutes:

Yeah, known issue. Give it more time - that particular patch is
*really* slow. Yes, more than 29 minutes, on my system as well.

What I can recommend is that you remove things from copy-list that you
don't care about, e.g. bluetooth drivers if you don't need them,
certain wifi drivers you don't need, etc. (before running gentree.py)

johannes

[Julia Lawall]

On Fri, 1 Dec 2017, Johannes Berg wrote:

> On Fri, 2017-12-01 at 09:57 -0800, Remington Furman wrote:
> >
> > Ok, I've rebuilt with 1.0.4, and it applied genl-const.cocci just
> > fine. But, later on my CPU was held at 100% for 29 minutes:
>
> Yeah, known issue. Give it more time - that particular patch is
> *really* slow. Yes, more than 29 minutes, on my system as well.
>
> What I can recommend is that you remove things from copy-list that you
> don't care about, e.g. bluetooth drivers if you don't need them,
> certain wifi drivers you don't need, etc. (before running gentree.py)

You can also set a timeout, for example --timeout 120 (120 seconds). It
will report on the files that timed out and you can check on them
manually. Some other useful options are:

--profile: see how long it spends in each rule or various parts of the
OCaml code

--show-trying: see what function it is working on

--debug see what rule and with what inputs it is working on

It may work better to put --debug before the others.

julia

>
> johannes
> _______________________________________________
> Cocci mailing list
> ***@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[Johannes Berg]

On Fri, 2017-12-01 at 19:57 +0100, Julia Lawall wrote:

> You can also set a timeout, for example --timeout 120 (120 seconds). It
> will report on the files that timed out and you can check on them
> manually. Some other useful options are:

Right, but the result won't compile if you do that and it doesn't
modify some files :-)

> --profile: see how long it spends in each rule or various parts of the
> OCaml code
>
> --show-trying: see what function it is working on
>
> --debug see what rule and with what inputs it is working on

I guess I should play with these and figure out why this particular
spatch is so slow, thanks for the pointers!

johannes

[Remington Furman]

On 12/01/2017 10:02 AM, Johannes Berg wrote:
> On Fri, 2017-12-01 at 09:57 -0800, Remington Furman wrote:
>> Ok, I've rebuilt with 1.0.4, and it applied genl-const.cocci just
>> fine. But, later on my CPU was held at 100% for 29 minutes:
> Yeah, known issue. Give it more time - that particular patch is
> *really* slow. Yes, more than 29 minutes, on my system as well.
>
> What I can recommend is that you remove things from copy-list that you
> don't care about, e.g. bluetooth drivers if you don't need them,
> certain wifi drivers you don't need, etc. (before running gentree.py)
>
> johannes

Ok, that's great to know.  I will restart it.  I didn't know
about the copy list yet, that will be helpful.

Thank you again for your help.

-Remington

[Remington Furman]

On 12/01/2017 10:09 AM, Remington Furman wrote:
> On 12/01/2017 10:02 AM, Johannes Berg wrote:
>> On Fri, 2017-12-01 at 09:57 -0800, Remington Furman wrote:
>>> Ok, I've rebuilt with 1.0.4, and it applied genl-const.cocci just
>>> fine.  But, later on my CPU was held at 100% for 29 minutes:
>> Yeah, known issue. Give it more time - that particular patch is
>> *really* slow. Yes, more than 29 minutes, on my system as well.
>>
>> What I can recommend is that you remove things from copy-list that you
>> don't care about, e.g. bluetooth drivers if you don't need them,
>> certain wifi drivers you don't need, etc. (before running gentree.py)
>>
>> johannes
>
> Ok, that's great to know.  I will restart it.  I didn't know
> about the copy list yet, that will be helpful.
>
> Thank you again for your help.
>
> -Remington
Following up, running gentree.py again (without modifying the
copy list) worked fine.

-Remington

[Johannes Berg]

On Fri, 2017-12-01 at 15:48 -0800, Remington Furman wrote:
>
> Following up, running gentree.py again (without modifying the
> copy list) worked fine.

Good to hear :-)

Out of curiosity - how long did it take?

johannes