On Fri, 13 May 2011, Jim Cromie wrote:
> thanks Julia
>
> On Fri, May 13, 2011 at 2:39 PM, Julia Lawall <[email protected]> wrote:
> > On Fri, 13 May 2011, Jim Cromie wrote:
> >
> >> hi folks,
>
> >> 0 - is there a way to completely disable a rule with compile-syntax errors,
> >> such that it doesnt have to be edited out ? /* ... */ doesnt work.
> >> is there an __END__ ala perl or could there be ?
> >
> > Sorry about this. Originally we had the idea that // comments would be
> > SmPL comments and /* */ comments would be added code. But there is no
> > reason for this distinction. We can just consider an unannotated comment
> > to be a SmPL comment, and either kind of comment annotated to be + to be
> > added code. Indeed, this was already supported for //. I have fixed
> > the treatment of /* */ to allow it to appear without a + in front of it.
> > In the meantime, you can use //.
> >
>
> ok, good to know.
> Im using 1.00-rc1 - fwiw
>
>
>
> >> 1 - these 2 rules apply, but are simplistic, and wrong in some cases
> >> (they disregard the if-then-else that should be removed)
> >>
> >> My question with them is around the dev_t.
> >> 1st 'type's it, 2nd doesnt, both then use 'dev_t'
> >> where a keyword {expression, identifier, etc} would go,
> >> this doesnt blow up - whats the real interpretation ??
> >>
> >> @ register_chrdev_region @ // depends on fs_h @
> >> type dev_t;
> >
> > This is declaring dev_t to be a metavariable representing a type. You
> > could then for example use this metavariable in some variable declarations
> > in the SmPL code.
> >
> >> //identifier devid;
> >> dev_t devid;
> >
> > Now that dev_t is a metavariable, this will allow devid to have any type
> > (or any type compatible with the metavariable value if it had a reason to
> > have another one, but you don't use dev_t, so that is not your case).
> >
> > The declaration you wanted was not type, but typedef. But SmPL should be
> > able to figure out that dev_t should be a type, so you don't need the
> > typedef in this case either. Here are some examples. In the second
> > example, the typedef is needed, because the parser can't figure out that
>
> > it should be a type itself. Once something has been declared using
> > typedef, it remains considered that way for the entire semantic patch, not
> > just for the current rule.
>
> OOH - thats not obvious.
> I didnt think I wanted 'type dev_t' anyway, but I was surprised it worked.
>
> >
> > @@
> > dev_t c;
> > @@
> >
> > -foo(c);
> >
> > @@
> > typedef xxx_t;
> > @@
> >
> > - (xxx_t)y;
> >
>
> >> @ alloc_chrdev_region @ // depends on fs_h @
> >>
> >> // type dev_t;
> >> dev_t devid;
>
> So, given Ive removed the 'type dev_t;' metavar decl from the file,
> this 'dev_t devid' line declares a metavar - devid, which must be
> of the type dev_t ?
>
> is this because dev_t is known to be a typedef,
> and thus declaring devid as a metavar of that type is allowed ?
> and/or cuz any literal type is acceptable ?
Anything is acceptable.
> ie, would
> struct device DEV_VAR also work ?
Yes. But spatch need to have enough information to know the connection
between dev_t and struct device. This typically requires it being able to
find header files. You would need -all_includes (try to find all header
files explicitly mentioned in the file) or -recursive_includes (try to
find all header files included by included header files). These will slow
down a lot the application of your semantic patch. So it is better to use
the typedef if that is what is used in the code. Or if you are worried
that either might be, you can explicitly list both types in {}:
{dev_t,struct device} x;
You can also consider whether the type information is needed at all. If
you are writing eg:
x = foo();
and foo is a function that already returns a value of a specific type,
then there is no need to specify the type of x. If you can avoid relying
on header files, you will get a result faster, and the result will
possibly be mor complete, because coccinelle can't always find the header
file, if it is in some other directory that was not specified with a -I
option.
> >> 2- this rule wont apply - and is where Im mostly looking atm.
> >>
> >> @ for_rtc_dev @ // ./drivers/rtc/rtc-dev.c
> >>
> >> // typedef dev_t; // "meta: parse error: around = 'dev_t'
> >
> > Probably it already figured out that dev_t should be a typedef.
> >
> >> identifier err;
> >> identifier rtc_devt;
> >> expression maj, ct, nm;
> >> @@
> >> // dev_t rtc_devt; // < -- VARY THIS LINE
> >> ... // this doesnt matter
> >> // err = alloc_chrdev_region(&rtc_devt, 0, RTC_DEV_MAX, "rtc");
> >> - err = alloc_chrdev_region(&rtc_devt, maj, ct, nm);
> >> + err = register_chrdev_ids(&rtc_devt, maj, ct, nm);
> >> if (err < 0) { ... }
> >>
>
>
> >>
> >> linux-2.6.git]$ spatch -sp_file ../chrdev.cocci -partial_match
> >> ./drivers/rtc/rtc-dev.c
> >>
>
> >> ... (other rules elided) ...
> >>
> >> HANDLING: ./drivers/rtc/rtc-dev.c
> >> Empty list of bindings, I will restart from old env
> >> Empty list of bindings, I will restart from old env
> >> Empty list of bindings, I will restart from old env
> >> Empty list of bindings, I will restart from old env
> >> Empty list of bindings, I will restart from old env
> >> partial matches
> >> 7[]{!wit 7[err --> id err]
> >> {wit 7[rtc_devt --> id rtc_devt]
> >> {wit 7[maj --> 0]
> >> {wit 7[ct --> RTC_DEV_MAX]
> >> {wit 7[nm --> "rtc"]
> >> {wit 7[_v --> for_rtc_dev:err = alloc_chrdev_region(
> >> &for_rtc_dev:rtc_devt, for_rtc_dev:maj,
> >> for_rtc_dev:ct, for_rtc_dev:nm);]{}}}}}}}
> >>
> >>
> >> Empty list of bindings, I will restart from old env
> >>
> >>
> >> If I remove the leading // on the <-- VARY THIS LINE above,
> >> I get no partial match, only 1 more empty-list-of-bindings.
> >>
> >> This rule has 5 metavars, all are mentioned in the partial-matching output,
> >> whats missing ?
> >
> > I have the impression that it found the call to alloc_chrdev_region, but
> > not the subsequent if (err < 0) test. For that you have written { ... },
> > but in the code there are no braces. I usually put a statement
> > metavariable in this context. That will match a single statement by
> > itself as well as anything with braces.
> >
> > There are isomorphisms for eg { ... S } => S, but nothing for braces with
> > nothing specified inside, so that would mean that the braces really have
> > to be there.
> >
> >> I have 6 rules, and 6 empty-bindings, which seems to make sense.
> >>
> >>
> >>
> >> 3- this rule does work, at least in one case.
> >> If I can generalize it properly, I might be finished...
> >>
> >>
> >>
> >> @ for_fuse_cuse @ // this works
> >> identifier rc, devt;
> >> expression arg1, arg2; // doesnt work as ident - lh_expr would if it
> >> existed.
> >> expression MIN, CT, devname;
> >> expression warn_on_this_expr;
> >> identifier warn_on_this_ident;
> >> @@
> >>
> >> // devt = MKDEV(arg->dev_major, arg->dev_minor);
> >> devt = MKDEV(arg1, arg2);
> >> ... when != devt // no harm to purpose, may help other apps?
> >> - if (!MAJOR(devt))
> >> //- rc = alloc_chrdev_region(&devt, MINOR(devt), 1,
> >> devinfo.name);
> >> - rc = alloc_chrdev_region(&devt, MIN, CT, devname);
> >> - else
> >> //- rc = register_chrdev_region(devt, 1, devinfo.name);
> >> - rc = register_chrdev_region(devt, CT, devname);
> >> + rc = register_chrdev_ids(&devt, CT, devname);
> >> if (rc) {
> >> ...
> >> }
> >
> > Could you send an example of something you hope to match?
>
> I wanted a rule to pick up this chunk.
> my 1st 2 rules have altered it 1st.
>
> diff --git a/drivers/char/pc8736x_gpio.c b/drivers/char/pc8736x_gpio.c
> index b304ec0..c7e432e 100644
> --- a/drivers/char/pc8736x_gpio.c
> +++ b/drivers/char/pc8736x_gpio.c
> @@ -304,9 +304,9 @@ static int __init pc8736x_gpio_init(void)
>
> if (major) {
> devid = MKDEV(major, 0);
> - rc = register_chrdev_region(devid, PC8736X_GPIO_CT, DEVNAME);
> + rc = register_chrdev_ids(&devid, PC8736X_GPIO_CT, DEVNAME);
> } else {
> - rc = alloc_chrdev_region(&devid, 0, PC8736X_GPIO_CT, DEVNAME);
> + rc = register_chrdev_ids(&devid, PC8736X_GPIO_CT, DEVNAME);
> major = MAJOR(devid);
> }
This time your rule doesn't have braces by the code does.
> That is, unless theres an isomorphism that recognizes
> if (expr)
> {S+}
> else
> {S+}
>
> and collapses it down to just {S+}
No there isn't a rule like that. In your rule, you should put eg
if (...) {
...
- old
+ new
...
}
else {
...
-old
+new
...
}
The isomorphism { ... S ... } => S will take care of the case where there
are no braces.
> but relying on that would be dumb,
> theres too much likelyhood that its really S1 else S2
>
>
> So, Ive split my rules into separate files, to better see what each is doing,
>
>
>
> @ for_pc8736x_gpio @
> expression CT, DEVNAME;
> expression MIN;
> dev_t devid;
> identfifier major, rc;
> @@
>
> - if (major) {
> - devid = MKDEV(major, MIN);
> - rc = register_chrdev_region(devid, CT, DEVNAME);
> - } else {
> - rc = alloc_chrdev_region(&devid, MIN, CT, DEVNAME);
> - major = MAJOR(devid);
> - }
>
> + rc = register_chrdev_ids(&devid, CT, devname);
>
>
> This is oversimplified - it wont catch other forms with variations.
> But it doesnt catch intended code either.
> A pure literal version worked.
I'm puzzled by this. I'll have to look into it.
julia_______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)
