Re: [Cocci] Matching function prototypes and casts

[SF Markus Elfring]

>> (
>> *setup_timer(>_timer@tl, \((_func)\| \) \(&\| \) _callback, \((_data)\| 
>> \) E);
>> |
>> *E->_timer@tl.function = \((_func)\| \) \(&\| \) _callback;
>> )
> 
> No, it doesn't work, because \( \| \) matches an expression and there is
> no empty expression.

Will the Coccinelle software become able to generate the desired combination
of casts and address-of operators anyhow automatically?

Can the usage of optional items be expressed in a more convenient way here?

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

Re: [Cocci] Matching function prototypes and casts

[SF Markus Elfring]

> I had --no-includes in my .cocci.

Would you like to transform also any header files by the mentioned approach?


> More insane corner cases:

Are you looking for further clarification there?

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

Re: [Cocci] Matching function prototypes and casts

[Julia Lawall]

On Fri, 25 Aug 2017, SF Markus Elfring wrote:

> >> You try to search for variations of a function call and an assignment to
> >> a data structure member. It seems that they differ then by the usage of a 
> >> cast
> >> and the operator “&”.
> >> How do you think about to express such search criteria in a succinct way 
> >> with
> >> the help of the semantic patch language?
> >>
> >> I imagine that nested disjunctions or the SmPL operator “?” could be used
> >> for your source code transformation approach.
> >
> > I don't know what you have in mind,
>
> Would the following design suggestion be usable?
>
>
> @check_timer_function_usage@
> expression _callback, E;
> struct timer_list tl;
> identifier _timer;
> type _func, _data;
> @@
>
> (
> *setup_timer(>_timer@tl, \((_func)\| \) \(&\| \) _callback, \((_data)\| \) 
> E);
> |
> *E->_timer@tl.function = \((_func)\| \) \(&\| \) _callback;
> )

No, it doesn't work, because \( \| \) matches an expression and there is
no empty expression.

julia

>
>
> I wonder that “E” should be passed twice to the shown function call.
>
>
> > but ? only works on top-level terms (statements, loop headers, etc), not 
> > nested expressions.
>
> I assume that this functionality can occasionally be used more often.
>
> Regards,
> Markus
>___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Matching function prototypes and casts

[SF Markus Elfring]

>> You try to search for variations of a function call and an assignment to
>> a data structure member. It seems that they differ then by the usage of a 
>> cast
>> and the operator “&”.
>> How do you think about to express such search criteria in a succinct way with
>> the help of the semantic patch language?
>>
>> I imagine that nested disjunctions or the SmPL operator “?” could be used
>> for your source code transformation approach.
> 
> I don't know what you have in mind,

Would the following design suggestion be usable?


@check_timer_function_usage@
expression _callback, E;
struct timer_list tl;
identifier _timer;
type _func, _data;
@@

(
*setup_timer(>_timer@tl, \((_func)\| \) \(&\| \) _callback, \((_data)\| \) 
E);
|
*E->_timer@tl.function = \((_func)\| \) \(&\| \) _callback;
)


I wonder that “E” should be passed twice to the shown function call.


> but ? only works on top-level terms (statements, loop headers, etc), not 
> nested expressions.

I assume that this functionality can occasionally be used more often.

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

Re: [Cocci] Matching function prototypes and casts

[Julia Lawall]

On Fri, 25 Aug 2017, SF Markus Elfring wrote:

> > (
> > -setup_timer(&_E->_timer@_e, _callback, _E);
> > |
> …
> > -_E->_timer@_e.function = (_cast_func)&_callback;
> > )
>
> You try to search for variations of a function call and an assignment to
> a data structure member. It seems that they differ then by the usage of a cast
> and the operator “&”.
> How do you think about to express such search criteria in a succinct way with
> the help of the semantic patch language?
>
> I imagine that nested disjunctions or the SmPL operator “?” could be used
> for your source code transformation approach.

I don't know what you have in mind, but ? only works on top-level terms
(statements, loop headers, etc), not nested expressions.

julia

>
> Regards,
> Markus
> ___
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Matching function prototypes and casts

[SF Markus Elfring]

> (
> -setup_timer(&_E->_timer@_e, _callback, _E);
> |
…
> -_E->_timer@_e.function = (_cast_func)&_callback;
> )

You try to search for variations of a function call and an assignment to
a data structure member. It seems that they differ then by the usage of a cast
and the operator “&”.
How do you think about to express such search criteria in a succinct way with
the help of the semantic patch language?

I imagine that nested disjunctions or the SmPL operator “?” could be used
for your source code transformation approach.

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

Re: [Cocci] Matching function prototypes and casts

[Julia Lawall]

> > For me it works.  Do you have the latest version of Coccinelle from
> > github?  I used the option --all-includes.
>
> Ah-ha! Thank you. :) I had --no-includes in my .cocci. :)

>From a performance point of view, you might want to do what you can with
--no-includes first.  It will then only be doing the --all-includes on
files that still contain init_timer.

> More insane corner cases:
>
> I have this function:
>
> static void
> hfcmulti_dbusy_timer(struct hfc_multi *hc)
> {
> }
>
> And this rule (which is using the working change_timer_function_usage
> rule to find identifiers):
>
> // callback(struct something *handle)
> @change_callback_handle_arg
>  depends on patch &&
> change_timer_function_usage &&
> !change_callback_handle_cast@
> identifier change_timer_function_usage._callback;
> identifier change_timer_function_usage._timer;
> type _handletype;
> identifier _handle;
> @@
>
> -static void _callback(_handletype *_handle)
> +static void _callback(struct timer_list *t)
>  {
> +   _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
> ...
>  }
>
> It correctly produces:
>
> -static void
> -hfcmulti_dbusy_timer(struct hfc_multi *hc)
> +static void hfcmulti_dbusy_timer(struct timer_list *t)
>  {
> +   struct hfc_multi *hc = TIMER_CONTAINER(hc, t, timer);
>  }
>
> But since this was an empty function originally, I don't want to add
> just the variable declaration. I tried various ()-like things, but
> they didn't work:
>
> -static void _callback(_handletype *_handle)
> +static void _callback(struct timer_list *t)
>  {
> (
> +   _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
> ...
> |
> )
>  }
>
> etc...

Really what you care about is whether _handle was used.  I think that you
will need two rules:

@exists@
@@

-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t) {
+  _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
  ...
  _handle
  ... when any
}

and

-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t) { ...
}

to clean up the rest.

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

Re: [Cocci] Matching function prototypes and casts

[Kees Cook]

On Wed, Aug 23, 2017 at 2:19 PM, Julia Lawall  wrote:
>
>
> On Wed, 23 Aug 2017, Kees Cook wrote:
>
>> I think I'm getting closer. Here are some specific examples that don't
>> seem to work:
>>
>> ---match_callback.cocci---
>> virtual patch
>>
>> @match_timer_function_usage
>>  depends on patch@
>> expression _E;
>> struct timer_list _e;
>> identifier _timer;
>> identifier _callback;
>> type _cast_func, _cast_data;
>> @@
>>
>> (
>> -setup_timer(&_E->_timer@_e, _callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, &_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, _callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)_callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, (_cast_data)_E);
>> |
>> -_E->_timer@_e.function = _callback;
>> |
>> -_E->_timer@_e.function = &_callback;
>> |
>> -_E->_timer@_e.function = (_cast_func)_callback;
>> |
>> -_E->_timer@_e.function = (_cast_func)&_callback;
>> )
>> ---EOF---
>>
>> Doesn't match drivers/ide/ide-probe.c which has:
>>
>> setup_timer(>timer, _timer_expiry, (unsigned long)hwif);
>>
>> Even this doesn't:
>>
>> (
>> -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
>> )
>>
>> Unless I remove the "@_e" part...? Am I using that wrong?
>
> For me it works.  Do you have the latest version of Coccinelle from
> github?  I used the option --all-includes.

Ah-ha! Thank you. :) I had --no-includes in my .cocci. :)

More insane corner cases:

I have this function:

static void
hfcmulti_dbusy_timer(struct hfc_multi *hc)
{
}

And this rule (which is using the working change_timer_function_usage
rule to find identifiers):

// callback(struct something *handle)
@change_callback_handle_arg
 depends on patch &&
change_timer_function_usage &&
!change_callback_handle_cast@
identifier change_timer_function_usage._callback;
identifier change_timer_function_usage._timer;
type _handletype;
identifier _handle;
@@

-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t)
 {
+   _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
...
 }

It correctly produces:

-static void
-hfcmulti_dbusy_timer(struct hfc_multi *hc)
+static void hfcmulti_dbusy_timer(struct timer_list *t)
 {
+   struct hfc_multi *hc = TIMER_CONTAINER(hc, t, timer);
 }

But since this was an empty function originally, I don't want to add
just the variable declaration. I tried various ()-like things, but
they didn't work:

-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t)
 {
(
+   _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
...
|
)
 }

etc...

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Matching function prototypes and casts

[Kees Cook]

I think I'm getting closer. Here are some specific examples that don't
seem to work:

---match_callback.cocci---
virtual patch

@match_timer_function_usage
 depends on patch@
expression _E;
struct timer_list _e;
identifier _timer;
identifier _callback;
type _cast_func, _cast_data;
@@

(
-setup_timer(&_E->_timer@_e, _callback, _E);
|
-setup_timer(&_E->_timer@_e, &_callback, _E);
|
-setup_timer(&_E->_timer@_e, _callback, (_cast_data)_E);
|
-setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
|
-setup_timer(&_E->_timer@_e, (_cast_func)_callback, _E);
|
-setup_timer(&_E->_timer@_e, (_cast_func)&_callback, _E);
|
-setup_timer(&_E->_timer@_e, (_cast_func)_callback, (_cast_data)_E);
|
-setup_timer(&_E->_timer@_e, (_cast_func)&_callback, (_cast_data)_E);
|
-_E->_timer@_e.function = _callback;
|
-_E->_timer@_e.function = &_callback;
|
-_E->_timer@_e.function = (_cast_func)_callback;
|
-_E->_timer@_e.function = (_cast_func)&_callback;
)
---EOF---

Doesn't match drivers/ide/ide-probe.c which has:

setup_timer(>timer, _timer_expiry, (unsigned long)hwif);

Even this doesn't:

(
-setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
)

Unless I remove the "@_e" part...? Am I using that wrong?

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

Re: [Cocci] Matching function prototypes and casts

[Julia Lawall]

On Tue, 22 Aug 2017, Kees Cook wrote:

> Hi,
>
> I'm working on a large type conversion for struct timer_list, and
> it'll require changing all of the callback prototypes and changing
> logic at their start. For example, if I start with this:
>
>
> void primary_callback(unsigned long arg);
> void alternate_callback(struct some_handle *handle);
> ...
>
> void primary_callback(unsigned long arg)
> {
> struct some_handle *handle = (struct some_handle *)arg;
> ...
> }
>
> void alternate_callback(struct some_handle *handle)
> {
> ...
> }
>
> ...
> setup_timer(>timer_field, some_callback, (unsigned long)ptr);
> ...
> ptr->timer_field.function = (void (*)(unsigned long)alternate_callback;
>
>
> I'd like to end up with:
>
>
> void primary_callback(struct timer_list *t);
> void alternate_callback(struct timer_list *t);
> ...
>
> void primary_callback(struct timer_list *t)
> {
> struct some_handle *handle = container_of(t, struct some_handle,
> timer_field);
> ...
> }
>
> void alternate_callback(struct timer_list *t)
> {
> struct some_handle *handle = container_of(t, struct some_handle,
> timer_field);
> ...
> }
>
> ...
> setup_time(>timer_field, some_callback, >timer_field);
> ...
> ptr->timer_field.function = alternate_callback;
>
>
> I'm having trouble matching the callback arg type, and I'm having
> trouble identifying the callbacks. I think I can do things like this,
> but it's not quite working:
>
> @match_setup_timer@
> identifier _p;
> struct timer_list _timer_field;
> identifier _callback;
> type _cast;
> @@
>
> (
>  setup_timer(&_p->_timer_field, _setup_callback, _p);
> |
>  setup_timer(&_p->_timer_field, (_cast)_setup_callback, _p);

The code has setup_callback but the metavariable is just callback.

> )
>
> @match_function_assignment@
> identifier _p;
> struct timer_list _timer_field;

This is declaring a metavariable that matches an expression of type struct
timer_list.  You are trying to match a field name, so you just want an
identifier metavariable.  If you want to be sure that _p->_timer_field has
type struct timer_list, then you can make an expression metavariable of
that type, ie:

struct timer_list e;

and then put @e after _timer_field.  But maybe you don't even need
_p->_timer_field and can just use e?

> identifier _callback;
> type _cast;
> @@
>
> (
>  _p->_timer_field.function = _callback;
> |
>  _p->_timer_field.function = (_cast)_callback;
> )
>
> // callback(unsigned long arg)
> @change_callback_handle_cast
> depends on match_setup_timer || match_function_assignment@
> identifier ???._callback;
> struct timer_list ???._timer_field;
> type _origtype;
> identifier _origarg;
> type _handletype;
> identifier _handle;
> @@
>
> -void _callback(_origtype _origarg)
> +void _callback(struct timer_list *t)
>  {
> -_handletype *_handle = (_handletype *)_origarg;
> +_handletype *_handle = container_of(t, _handletype, _timer_field);
>
> // callback(struct something *handle)
> @change_callback_handle_arg
> depends on match_setup_timer || match_function_assignment@
> identifier ???._callback;

Is the concern here that you want to get the metavariable binding from two
different rules?

It might be possible to just have two metavariables inherited from
different places, and then have a disjunction in the function name
position.

Alternatively, maybe those two rules could be merged.

julia

> struct timer_list ???._timer_field;
> type _handletype;
> identifier _handle;
> @@
>
> -void _callback(_handletype *_handle)
> +void _callback(struct timer_list *t)
>  {
> +_origtype *_origarg = container_of(t, _origtype, _timer_field);
>
>
> Namely, I don't seem to be matching casts or function arg types, and I
> sometimes seem to be missing cases of struct timer_list. Also, I can't
> figure out how to have the change_callback_* rules depend on either
> matching match_setup_timer or match_function_assignment and having
> access to the matched fields...
>
> What would be the best way to deal with these?
>
> Thanks!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci