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 ?
ie, would
struct device DEV_VAR also work ?
>> expression minor; // ident didnt pick up constants
>
> Normal. If you want constants, you can say constant minor;. That will
> match eg 0, and also match identifiers written entirely in upper case. If
> you really want all possible identifier and all possible constants, you
> can say:
ack - I should have trimmed that comment :-)
>> 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);
}
That is, unless theres an isomorphism that recognizes
if (expr)
{S+}
else
{S+}
and collapses it down to just {S+}
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.
>
> julia
>
_______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)
