Weirdness in parsing cpp macros

Weirdness in parsing cpp macros

[Ville Syrjälä]

Hi Julia et al,

In Linux drm/i915 driver (drivers/gpu/drm/i915/display/intel_pps.[ch])
we have a magic macro like this:

#define with_intel_pps_lock(dp, wf) \
        for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), (wf)))


which we can then use like so:
...
with_intel_pps_lock(intel_dp, wakeref)
	wait_panel_power_cycle(intel_dp);
...


If I try to modify this code with eg.

@@
@@
- wait_panel_power_cycle
+ _wait_panel_power_cycle

spatch fails to parse the macro and won't do the changes here.


While experimenting with this I discovered that
I can make it work by:
- moving the macro definition into intel_pps.c file from intel_pps.h
- renaming the macro to contain the substring "for" (doesn't matter
  where in the macro name the "for" appears)

What on earth is going on here?

-- 
Ville Syrjälä
Intel

Re: Weirdness in parsing cpp macros

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]



On Wed, 20 Mar 2024, Ville Syrjälä wrote:

> Hi Julia et al,
>
> In Linux drm/i915 driver (drivers/gpu/drm/i915/display/intel_pps.[ch])
> we have a magic macro like this:
>
> #define with_intel_pps_lock(dp, wf) \

Did you try declaring:

iterator name with_intel_pps_lock;

up with the metavariables?

julia


>         for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), (wf)))
>
>
> which we can then use like so:
> ...
> with_intel_pps_lock(intel_dp, wakeref)
> 	wait_panel_power_cycle(intel_dp);
> ...
>
>
> If I try to modify this code with eg.
>
> @@
> @@
> - wait_panel_power_cycle
> + _wait_panel_power_cycle
>
> spatch fails to parse the macro and won't do the changes here.
>
>
> While experimenting with this I discovered that
> I can make it work by:
> - moving the macro definition into intel_pps.c file from intel_pps.h
> - renaming the macro to contain the substring "for" (doesn't matter
>   where in the macro name the "for" appears)
>
> What on earth is going on here?
>
> --
> Ville Syrjälä
> Intel
>

Re: Weirdness in parsing cpp macros

[Ville Syrjälä]

On Wed, Mar 20, 2024 at 02:24:08PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 20 Mar 2024, Ville Syrjälä wrote:
> 
> > Hi Julia et al,
> >
> > In Linux drm/i915 driver (drivers/gpu/drm/i915/display/intel_pps.[ch])
> > we have a magic macro like this:
> >
> > #define with_intel_pps_lock(dp, wf) \
> 
> Did you try declaring:
> 
> iterator name with_intel_pps_lock;
> 
> up with the metavariables?

Nope, didn't know about that one.

Seems to work fine with that. Thanks.

> 
> julia
> 
> 
> >         for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), (wf)))
> >
> >
> > which we can then use like so:
> > ...
> > with_intel_pps_lock(intel_dp, wakeref)
> > 	wait_panel_power_cycle(intel_dp);
> > ...
> >
> >
> > If I try to modify this code with eg.
> >
> > @@
> > @@
> > - wait_panel_power_cycle
> > + _wait_panel_power_cycle
> >
> > spatch fails to parse the macro and won't do the changes here.
> >
> >
> > While experimenting with this I discovered that
> > I can make it work by:
> > - moving the macro definition into intel_pps.c file from intel_pps.h
> > - renaming the macro to contain the substring "for" (doesn't matter
> >   where in the macro name the "for" appears)
> >
> > What on earth is going on here?
> >
> > --
> > Ville Syrjälä
> > Intel
> >


-- 
Ville Syrjälä
Intel

Re: Weirdness in parsing cpp macros

[Jani Nikula]

On Wed, 20 Mar 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Mar 20, 2024 at 02:24:08PM +0100, Julia Lawall wrote:
>> 
>> 
>> On Wed, 20 Mar 2024, Ville Syrjälä wrote:
>> 
>> > Hi Julia et al,
>> >
>> > In Linux drm/i915 driver (drivers/gpu/drm/i915/display/intel_pps.[ch])
>> > we have a magic macro like this:
>> >
>> > #define with_intel_pps_lock(dp, wf) \
>> 
>> Did you try declaring:
>> 
>> iterator name with_intel_pps_lock;
>> 
>> up with the metavariables?
>
> Nope, didn't know about that one.
>
> Seems to work fine with that. Thanks.

Okay, I have another one wrt macros. :)

I'm trying to add a completely new variadic macro, but it fails at
"...". I've tried all sorts of things, but can't seem to be able to add
a literal "...".

I've tested that my cocci patch works with x's:

+ #define fn(p, xxx) foo(__VA_ARGS__)

but when I try to make it actually variadic like:

+ #define fn(p, ...) foo(__VA_ARGS__)

it gives me error. Is there a way to escape? Even tried to use a fresh
identifier vararg = "..."; but cocci made them unique with numbering
"...0" and "...1" etc.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: Weirdness in parsing cpp macros

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]



On Wed, 20 Mar 2024, Jani Nikula wrote:

> On Wed, 20 Mar 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Mar 20, 2024 at 02:24:08PM +0100, Julia Lawall wrote:
> >>
> >>
> >> On Wed, 20 Mar 2024, Ville Syrjälä wrote:
> >>
> >> > Hi Julia et al,
> >> >
> >> > In Linux drm/i915 driver (drivers/gpu/drm/i915/display/intel_pps.[ch])
> >> > we have a magic macro like this:
> >> >
> >> > #define with_intel_pps_lock(dp, wf) \
> >>
> >> Did you try declaring:
> >>
> >> iterator name with_intel_pps_lock;
> >>
> >> up with the metavariables?
> >
> > Nope, didn't know about that one.
> >
> > Seems to work fine with that. Thanks.
>
> Okay, I have another one wrt macros. :)
>
> I'm trying to add a completely new variadic macro, but it fails at
> "...". I've tried all sorts of things, but can't seem to be able to add
> a literal "...".
>
> I've tested that my cocci patch works with x's:
>
> + #define fn(p, xxx) foo(__VA_ARGS__)
>
> but when I try to make it actually variadic like:
>
> + #define fn(p, ...) foo(__VA_ARGS__)
>
> it gives me error. Is there a way to escape? Even tried to use a fresh
> identifier vararg = "..."; but cocci made them unique with numbering
> "...0" and "...1" etc.

Put 6 dots.  It's silly, but ... is a Coccinelle thing, so we had to use
something else.

julia

Re: Weirdness in parsing cpp macros

[Jani Nikula]

On Wed, 20 Mar 2024, Julia Lawall <julia.lawall@inria.fr> wrote:
> On Wed, 20 Mar 2024, Jani Nikula wrote:
>> Okay, I have another one wrt macros. :)
>>
>> I'm trying to add a completely new variadic macro, but it fails at
>> "...". I've tried all sorts of things, but can't seem to be able to add
>> a literal "...".
>>
>> I've tested that my cocci patch works with x's:
>>
>> + #define fn(p, xxx) foo(__VA_ARGS__)
>>
>> but when I try to make it actually variadic like:
>>
>> + #define fn(p, ...) foo(__VA_ARGS__)
>>
>> it gives me error. Is there a way to escape? Even tried to use a fresh
>> identifier vararg = "..."; but cocci made them unique with numbering
>> "...0" and "...1" etc.
>
> Put 6 dots.  It's silly, but ... is a Coccinelle thing, so we had to use
> something else.

I've tried, but it doesn't seem to work in the + side:

plus: parse error: 
  File "/tmp/tmp.clvvc812Qe", line 20, column 2, charpos = 254
  around = '#define fn(',
  whole content = + #define fn(p, ......) __fn(__to_intel_display(p), __VA_ARGS__)

BR,
Jani.


-- 
Jani Nikula, Intel

Re: Weirdness in parsing cpp macros

[Julia Lawall]

On Wed, 20 Mar 2024, Jani Nikula wrote:

> On Wed, 20 Mar 2024, Julia Lawall <julia.lawall@inria.fr> wrote:
> > On Wed, 20 Mar 2024, Jani Nikula wrote:
> >> Okay, I have another one wrt macros. :)
> >>
> >> I'm trying to add a completely new variadic macro, but it fails at
> >> "...". I've tried all sorts of things, but can't seem to be able to add
> >> a literal "...".
> >>
> >> I've tested that my cocci patch works with x's:
> >>
> >> + #define fn(p, xxx) foo(__VA_ARGS__)
> >>
> >> but when I try to make it actually variadic like:
> >>
> >> + #define fn(p, ...) foo(__VA_ARGS__)
> >>
> >> it gives me error. Is there a way to escape? Even tried to use a fresh
> >> identifier vararg = "..."; but cocci made them unique with numbering
> >> "...0" and "...1" etc.
> >
> > Put 6 dots.  It's silly, but ... is a Coccinelle thing, so we had to use
> > something else.
>
> I've tried, but it doesn't seem to work in the + side:
>
> plus: parse error:
>   File "/tmp/tmp.clvvc812Qe", line 20, column 2, charpos = 254
>   around = '#define fn(',
>   whole content = + #define fn(p, ......) __fn(__to_intel_display(p), __VA_ARGS__)

OK, maybe it doesn't work for macros.  I will take a look.

julia

Re: Weirdness in parsing cpp macros

[Julia Lawall]

On Wed, 20 Mar 2024, Jani Nikula wrote:

> On Wed, 20 Mar 2024, Julia Lawall <julia.lawall@inria.fr> wrote:
> > On Wed, 20 Mar 2024, Jani Nikula wrote:
> >> Okay, I have another one wrt macros. :)
> >>
> >> I'm trying to add a completely new variadic macro, but it fails at
> >> "...". I've tried all sorts of things, but can't seem to be able to add
> >> a literal "...".
> >>
> >> I've tested that my cocci patch works with x's:
> >>
> >> + #define fn(p, xxx) foo(__VA_ARGS__)
> >>
> >> but when I try to make it actually variadic like:
> >>
> >> + #define fn(p, ...) foo(__VA_ARGS__)
> >>
> >> it gives me error. Is there a way to escape? Even tried to use a fresh
> >> identifier vararg = "..."; but cocci made them unique with numbering
> >> "...0" and "...1" etc.
> >
> > Put 6 dots.  It's silly, but ... is a Coccinelle thing, so we had to use
> > something else.
>
> I've tried, but it doesn't seem to work in the + side:
>
> plus: parse error:
>   File "/tmp/tmp.clvvc812Qe", line 20, column 2, charpos = 254
>   around = '#define fn(',
>   whole content = + #define fn(p, ......) __fn(__to_intel_display(p), __VA_ARGS__)

A patch is below.  I haven't tested it.  It's working its way through the
CI at the moment, but I'll be offline for the rest of the day, so I don't
think I will be able to make it public today.  But let me know if it
works.

In particular, I don't think it would work for matching or removals, but
it should work for additions.

julia

---

commit 8a637290a87ca86ca75a216ef1a7ac07c6a1fd57
Author: Julia Lawall <Julia.Lawall@inria.fr>
Date:   Thu Mar 21 09:16:07 2024 +0100

    allow ...... as a #define param in SmPL

diff --git a/parsing_cocci/parser_cocci_menhir.mly b/parsing_cocci/parser_cocci_menhir.mly
index afba9e8f..4f9be9a6 100644
--- a/parsing_cocci/parser_cocci_menhir.mly
+++ b/parsing_cocci/parser_cocci_menhir.mly
@@ -1614,6 +1614,12 @@ defineop:

 dparam: mident { Ast0_cocci.wrap(Ast0_cocci.DParam $1) }
 | TMetaDParamList { Parse_aux.meta_dparam_list $1 }
+| TVAEllipsis
+    { let id = ("...",$1) in
+      Ast0_cocci.wrap
+	(Ast0_cocci.DParam
+	   (Ast0_cocci.wrap
+	      (Ast0_cocci.Id(Parse_aux.id2mcode id)))) }

 define_param_list_option:
     empty_list_start(dparam,TEllipsis)

✗ Fi.CI.BUILD: failure for Weirdness in parsing cpp macros

[Patchwork]

== Series Details ==

Series: Weirdness in parsing cpp macros
URL   : https://patchwork.freedesktop.org/series/131409/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/131409/revisions/1/mbox/ not applied
Applying: Weirdness in parsing cpp macros
error: sha1 information is lacking or useless (parsing_cocci/parser_cocci_menhir.mly).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Weirdness in parsing cpp macros
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

Re: Weirdness in parsing cpp macros

[Jani Nikula]

On Thu, 21 Mar 2024, Julia Lawall <julia.lawall@inria.fr> wrote:
> On Wed, 20 Mar 2024, Jani Nikula wrote:
>
>> On Wed, 20 Mar 2024, Julia Lawall <julia.lawall@inria.fr> wrote:
>> > On Wed, 20 Mar 2024, Jani Nikula wrote:
>> >> Okay, I have another one wrt macros. :)
>> >>
>> >> I'm trying to add a completely new variadic macro, but it fails at
>> >> "...". I've tried all sorts of things, but can't seem to be able to add
>> >> a literal "...".
>> >>
>> >> I've tested that my cocci patch works with x's:
>> >>
>> >> + #define fn(p, xxx) foo(__VA_ARGS__)
>> >>
>> >> but when I try to make it actually variadic like:
>> >>
>> >> + #define fn(p, ...) foo(__VA_ARGS__)
>> >>
>> >> it gives me error. Is there a way to escape? Even tried to use a fresh
>> >> identifier vararg = "..."; but cocci made them unique with numbering
>> >> "...0" and "...1" etc.
>> >
>> > Put 6 dots.  It's silly, but ... is a Coccinelle thing, so we had to use
>> > something else.
>>
>> I've tried, but it doesn't seem to work in the + side:
>>
>> plus: parse error:
>>   File "/tmp/tmp.clvvc812Qe", line 20, column 2, charpos = 254
>>   around = '#define fn(',
>>   whole content = + #define fn(p, ......) __fn(__to_intel_display(p), __VA_ARGS__)
>
> A patch is below.  I haven't tested it.  It's working its way through the
> CI at the moment, but I'll be offline for the rest of the day, so I don't
> think I will be able to make it public today.  But let me know if it
> works.

Thanks for looking into it!

I'm afraid setting up an environment to try cocci patches is a bit too
involved for me right now.

BR,
Jani.


>
> In particular, I don't think it would work for matching or removals, but
> it should work for additions.
>
> julia
>
> ---
>
> commit 8a637290a87ca86ca75a216ef1a7ac07c6a1fd57
> Author: Julia Lawall <Julia.Lawall@inria.fr>
> Date:   Thu Mar 21 09:16:07 2024 +0100
>
>     allow ...... as a #define param in SmPL
>
> diff --git a/parsing_cocci/parser_cocci_menhir.mly b/parsing_cocci/parser_cocci_menhir.mly
> index afba9e8f..4f9be9a6 100644
> --- a/parsing_cocci/parser_cocci_menhir.mly
> +++ b/parsing_cocci/parser_cocci_menhir.mly
> @@ -1614,6 +1614,12 @@ defineop:
>
>  dparam: mident { Ast0_cocci.wrap(Ast0_cocci.DParam $1) }
>  | TMetaDParamList { Parse_aux.meta_dparam_list $1 }
> +| TVAEllipsis
> +    { let id = ("...",$1) in
> +      Ast0_cocci.wrap
> +	(Ast0_cocci.DParam
> +	   (Ast0_cocci.wrap
> +	      (Ast0_cocci.Id(Parse_aux.id2mcode id)))) }
>
>  define_param_list_option:
>      empty_list_start(dparam,TEllipsis)

-- 
Jani Nikula, Intel