Re: [PATCH RFC] coccinelle: flag constants being passed for jiffies

[Julia Lawall <julia.lawall@lip6.fr>]

On Wed, 27 May 2015, Nicholas Mc Guire wrote:

> schedule_timeout_* takes a jiffies value as timeout - passing in a constant
> makes the timeout HZ dependent which is wrong. Checking for contstants only
> yields many false positives so they are filtered for digits only. A numeric
> value of 1 is though commonly in use for "shortest possible delay" so those
> cases are treated as false positives as well and not reported.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> The cases reported all look like they are actual API inconsistencies but
> not reporting does not mean there is no bug with respect to jiffies. This
> still can miss some values e.g. like in:
>  drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
>  drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);
> 
> For schedule_timeout_*() it reports 23 cases - all of which look like 
> they are correct findings. 
> 
> Further the list of functions taking jiffies as arguments is much longer
> but they can be added once the basic script is sound - which it probably
> is not at this point.
> 
>  scripts/coccinelle/api/timeout_HZ_dependent.cocci |   62 +++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci
> 
> diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> new file mode 100644
> index 0000000..6e98da1
> --- /dev/null
> +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> @@ -0,0 +1,62 @@
> +/* check for hardcoded timeout values which are thus HZ dependent
> + * only report findings if the value is digits and != 1 as hardcoded
> + * 1 seems to be in use for short delays.
> + *
> + * No patch mode for this as converting the value from C to
> + * msecs_to_jiffies(C) could be changing the effective timeout by
> + * more than a factor of 10 so this always needs manual inspection
> + *
> + * Options:  --no-includes --include-headers
> + */
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@ep depends on !patch && (org || report)@
> +identifier f;
> +constant C;
> +position p1,p2;
> +@@
> +
> +f@p1(...) {

Do you need the function for anything?  This would be more efficient with 
just the calls.


> + <+...
> +(
> +* schedule_timeout@p2(C)

If you put in the argument list \(i\|1\|C@p2\) then you can get rid of the 
python code (i would be an identifier metavariable).

What kind of false positives do you get for the macro case?  Would it be 
sufficient to do:

@r@
expression e != 1;
identifier i,
@@

#define i e

@@
identifier r.i,
@@

*schedule_timeout(i)

(and all the other cases?)

julia

> +|
> +* schedule_timeout_interruptible@p2(C)
> +|
> +* schedule_timeout_killable@p2(C)
> +|
> +* schedule_timeout_uninterruptible@p2(C)
> +)
> + ...+>
> +}
> +
> +@script:python depends on org@
> +p1 << ep.p1;
> +p2 << ep.p2;
> +timeout << ep.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg="WARNING: timeout is HZ dependent"
> +      coccilib.org.print_todo(p2[0], msg)
> +
> +@script:python depends on report@
> +p1 << ep.p1;
> +p2 << ep.p2;
> +timeout << ep.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg="WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p2[0], msg)
> -- 
> 1.7.10.4
> 
>