[Cocci] Itterating matches

[Cocci] Itterating matches

[Ira Weiny]

I have been looking for a way to help automate my kmap/mem*/kunmap
transformation.[1]  I'm very new to coccinelle and I'm having some trouble.

I have tried to build up a script[2][3] to do this while also learning coccinelle
but I'm not getting a couple of points.

1) How do I get matches to iterate?
	1a) What is the difference between '<+... ...+>' and '<... ...>'?
2) Why can't I get the type declaration (type VP;) matched correctly.  It works
   some times but not always.  It also matches a lot of random declarations?
	2a) and for some reason in the more advanced script[3] it completely
	    breaks.  Which is very confusing because fundamentally it does not
            seem to be any different from [2]?

Any advice is appreciated.

Thanks,
Ira

[1] https://lore.kernel.org/lkml/20201124141244.GE17322@twin.jikos.cz/

[2]
<script>
  1 @ rule0 @
  2 type VP;
  3 identifier ptr;
  4 identifier page;
  5 @@
  6 
  7 -VP ptr;
  8 <...
  9 (
 10 -ptr = kmap(page);
 11 |
 12 -ptr = kmap_atomic(page);
 13 )
 14 <+...
 15 (
 16 -memset(...);
 17 +memIra();
 18 |
 19 -memcpy(...);
 20 +memIra();
 21 )
 22 ...+>
 23 (
 24 -kunmap(page);
 25 |
 26 -kunmap_atomic(ptr);
 27 )
 28 ...>
</script>

[3]
<script>
  1 @ rule0 @
  2 type VP;
  3 identifier page;
  4 identifier ptr;
  5 expression V;
  6 expression L;
  7 expression T;
  8 expression F;
  9 expression B;
 10 expression Off;
 11 @@
 12 
 13 -VP ptr;
 14 <...
 15 (
 16 -ptr = kmap(page);
 17 |
 18 -ptr = kmap_atomic(page);
 19 )
 20 <...
 21 (
 22 -memset(ptr, 0, L); 
 23 +zero_user(page, 0, L);
 24 |
 25 -memset(ptr + Off, 0, L);
 26 +zero_user(page, Off, L);
 27 |
 28 -memset(ptr, V, L);
 29 +memset_page(page, V, 0, L);
 30 |
 31 -memset(ptr + Off, V, L);
 32 +memset_page(page, V, Off, L);
 33 |
 34 -memcpy(ptr + Off, F, B);
 35 +memcpy_to_page(page, Off, F, B);
 36 |
 37 -memcpy(ptr, F, B);
 38 +memcpy_to_page(page, 0, F, B);
 39 |
 40 -memcpy(T, ptr + Off, B);
 41 +memcpy_from_page(T, page, Off, B);
 42 |
 43 -memcpy(T, ptr, B);
 44 +memcpy_from_page(T, page, 0, B);
 45 )
 46 ...>
 47 (
 48 -kunmap(page);
 49 |
 50 -kunmap_atomic(ptr);
 51 )
 52 ...>
</script>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Itterating matches

[Julia Lawall]

On Fri, 27 Nov 2020, Ira Weiny wrote:

> I have been looking for a way to help automate my kmap/mem*/kunmap
> transformation.[1]  I'm very new to coccinelle and I'm having some trouble.
>
> I have tried to build up a script[2][3] to do this while also learning coccinelle
> but I'm not getting a couple of points.
>
> 1) How do I get matches to iterate?

What do you mean by iterate?

> 	1a) What is the difference between '<+... ...+>' and '<... ...>'?

<+... P ...+> requires P to appear one or more times
<... P ...>  allows P to occur 0 or more times

> 2) Why can't I get the type declaration (type VP;) matched correctly.  It works
>    some times but not always.  It also matches a lot of random declarations?
> 	2a) and for some reason in the more advanced script[3] it completely
> 	    breaks.  Which is very confusing because fundamentally it does not
>             seem to be any different from [2]?

I haven't tried running the scripts to see why the declaration breaks.
but I think that the declaration is not what you want.  ptr could be used
in some other case.  It would be better to have one rule that makes the
rest of the changes in the code, and the another rule that removes the ptr
declaration when there is no remaining use of the variable:

@r1@
identifier ptr;
...
@@

remove kmaps etc

@@
identifier r1.ptr;
type T;
@@

- T ptr;
  ... when != ptr

This will allow you to get rid of lines 8 and 28 in [2].

julia


> Any advice is appreciated.
>
> Thanks,
> Ira
>
> [1] https://lore.kernel.org/lkml/20201124141244.GE17322@twin.jikos.cz/
>
> [2]
> <script>
>   1 @ rule0 @
>   2 type VP;
>   3 identifier ptr;
>   4 identifier page;
>   5 @@
>   6
>   7 -VP ptr;
>   8 <...
>   9 (
>  10 -ptr = kmap(page);
>  11 |
>  12 -ptr = kmap_atomic(page);
>  13 )
>  14 <+...
>  15 (
>  16 -memset(...);
>  17 +memIra();
>  18 |
>  19 -memcpy(...);
>  20 +memIra();
>  21 )
>  22 ...+>
>  23 (
>  24 -kunmap(page);
>  25 |
>  26 -kunmap_atomic(ptr);
>  27 )
>  28 ...>
> </script>
>
> [3]
> <script>
>   1 @ rule0 @
>   2 type VP;
>   3 identifier page;
>   4 identifier ptr;
>   5 expression V;
>   6 expression L;
>   7 expression T;
>   8 expression F;
>   9 expression B;
>  10 expression Off;
>  11 @@
>  12
>  13 -VP ptr;
>  14 <...
>  15 (
>  16 -ptr = kmap(page);
>  17 |
>  18 -ptr = kmap_atomic(page);
>  19 )
>  20 <...
>  21 (
>  22 -memset(ptr, 0, L);
>  23 +zero_user(page, 0, L);
>  24 |
>  25 -memset(ptr + Off, 0, L);
>  26 +zero_user(page, Off, L);
>  27 |
>  28 -memset(ptr, V, L);
>  29 +memset_page(page, V, 0, L);
>  30 |
>  31 -memset(ptr + Off, V, L);
>  32 +memset_page(page, V, Off, L);
>  33 |
>  34 -memcpy(ptr + Off, F, B);
>  35 +memcpy_to_page(page, Off, F, B);
>  36 |
>  37 -memcpy(ptr, F, B);
>  38 +memcpy_to_page(page, 0, F, B);
>  39 |
>  40 -memcpy(T, ptr + Off, B);
>  41 +memcpy_from_page(T, page, Off, B);
>  42 |
>  43 -memcpy(T, ptr, B);
>  44 +memcpy_from_page(T, page, 0, B);
>  45 )
>  46 ...>
>  47 (
>  48 -kunmap(page);
>  49 |
>  50 -kunmap_atomic(ptr);
>  51 )
>  52 ...>
> </script>
> _______________________________________________
> 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] Itterating matches

[Ira Weiny]

On Sat, Nov 28, 2020 at 08:55:16AM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 27 Nov 2020, Ira Weiny wrote:
> 
> > I have been looking for a way to help automate my kmap/mem*/kunmap
> > transformation.[1]  I'm very new to coccinelle and I'm having some trouble.
> >
> > I have tried to build up a script[2][3] to do this while also learning coccinelle
> > but I'm not getting a couple of points.
> >
> > 1) How do I get matches to iterate?
> 
> What do you mean by iterate?
> 
> > 	1a) What is the difference between '<+... ...+>' and '<... ...>'?
> 
> <+... P ...+> requires P to appear one or more times
> <... P ...>  allows P to occur 0 or more times

Great that is what I thought but I was getting confused.  See the examples
below for details.

> 
> > 2) Why can't I get the type declaration (type VP;) matched correctly.  It works
> >    some times but not always.  It also matches a lot of random declarations?
> > 	2a) and for some reason in the more advanced script[3] it completely
> > 	    breaks.  Which is very confusing because fundamentally it does not
> >             seem to be any different from [2]?
> 
> I haven't tried running the scripts to see why the declaration breaks.
> but I think that the declaration is not what you want.  ptr could be used
> in some other case.  It would be better to have one rule that makes the
> rest of the changes in the code, and the another rule that removes the ptr
> declaration when there is no remaining use of the variable:
> 
> @r1@
> identifier ptr;
> ...
> @@
> 
> remove kmaps etc
> 
> @@
> identifier r1.ptr;
> type T;
> @@
> 
> - T ptr;
>   ... when != ptr
> 
> This will allow you to get rid of lines 8 and 28 in [2].

Ok this helps a lot.   Thank you!

That said it does not seem to work.  I have a better example now.

https://elixir.bootlin.com/linux/v5.10-rc6/source/fs/btrfs/compression.c

fs/btrfs/compression.c:

 571                 if (page->index == end_index) {
 572                         char *userpage;
 573                         size_t zero_offset = offset_in_page(isize);
 574 
 575                         if (zero_offset) {
 576                                 int zeros;
 577                                 zeros = PAGE_SIZE - zero_offset;
 578                                 userpage = kmap_atomic(page);
 579                                 memset(userpage + zero_offset, 0, zeros);
 580                                 flush_dcache_page(page);
 581                                 kunmap_atomic(userpage);
 582                         }
 583                 }

<script>
@ r1 @
identifier page;
identifier ptr;
expression L;
expression Off;
@@

-ptr = kmap_atomic(page);
...
-memset(ptr + Off, 0, L);
+zero_user(page, Off, L);
...
-kunmap_atomic(ptr);


@@
identifier r1.ptr;
type VP;
@@

-VP ptr;
        ... when != ptr;
</script>

Results in:

--- fs/btrfs/compression.c
+++ /tmp/cocci-output-1442863-cb5dd7-compression.c
@@ -575,10 +575,8 @@ static noinline int add_ra_bio_pages(str
                        if (zero_offset) {
                                int zeros;
                                zeros = PAGE_SIZE - zero_offset;
-                               userpage = kmap_atomic(page);
-                               memset(userpage + zero_offset, 0, zeros);
+                               zero_user(page, zero_offset, zeros);
                                flush_dcache_page(page);
-                               kunmap_atomic(userpage);
                        }
                }


Question: Is there something about userpage being declared in the outer block
which is causing the second rule to fail?


Back to the 'iteration issue'.  I've realized that I understood perfectly what
the iteration dot notations meant...  My bug was elsewhere.

For others who may be following along at home, I'll walk through this because
it brings up another issue with the removal of the pointer declaration which
I don't understand.

That same file (fs/btrfs/compression.c) has a second instance I'm trying to
match; except with memcpy().

1242         char *kaddr;
...
1267         /* copy bytes from the working buffer into the pages */
1268         while (working_bytes > 0) {
1269                 bytes = min_t(unsigned long, bvec.bv_len,
1270                                 PAGE_SIZE - (buf_offset % PAGE_SIZE));
1271                 bytes = min(bytes, working_bytes);
1272 
1273                 kaddr = kmap_atomic(bvec.bv_page);
1274                 memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
1275                 kunmap_atomic(kaddr);
...


Here I added <... ...> (and even tried <+... ...+>) around the
k[un]map_atomic() in my script to try and catch the second instance along with
the memcpy() as an alternative match; thusly.

<script>
@ r1 @
identifier page;
identifier ptr;
expression L;
expression F;
expression B;
expression Off;
@@

<...
-ptr = kmap_atomic(page);
...
(
-memset(ptr + Off, 0, L);
+zero_user(page, Off, L);
|
-memcpy(ptr + Off, F, B);
+memcpy_to_page(page, Off, F, B);
)
...
-kunmap_atomic(ptr);
...>


@@
identifier r1.ptr;
type VP;
@@

-VP ptr;
        ... when != ptr;
</script>

Which results in:

--- fs/btrfs/compression.c
+++ /tmp/cocci-output-1443266-8381d0-compression.c
@@ -575,10 +575,8 @@ static noinline int add_ra_bio_pages(str
                        if (zero_offset) {
                                int zeros;
                                zeros = PAGE_SIZE - zero_offset;
-                               userpage = kmap_atomic(page);
-                               memset(userpage + zero_offset, 0, zeros);
+                               zero_user(page, zero_offset, zeros);
                                flush_dcache_page(page);
-                               kunmap_atomic(userpage);
                        }
                }
 
:-/

Finally, after thinking on this more I realized this was _not_ because the
_iteration_ was wrong.  Rather the second kmap_atomic uses 'bvec.bv_page' which
matches as an expression not an identifier.  The correct script is therefore:

<script>
10:59:30 > cat kmap-test.cocci 
@ r1 @
expression page;                <============ 'expression'
identifier ptr;
expression L;
expression F;
expression B;
expression Off;
@@

-ptr = kmap_atomic(page);
...
(
-memset(ptr + Off, 0, L);
+zero_user(page, Off, L);
|
-memcpy(ptr + Off, F, B);
+memcpy_to_page(page, Off, F, B);
)
...
-kunmap_atomic(ptr);


@@
identifier r1.ptr;
type VP;
@@

-VP ptr;
        ... when != ptr;
</script>

which results in...

--- fs/btrfs/compression.c
+++ /tmp/cocci-output-1443375-182695-compression.c
@@ -145,7 +145,6 @@ static int check_compressed_csum(struct
        const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
        struct page *page;
        unsigned long i;
-       char *kaddr;
        u8 csum[BTRFS_CSUM_SIZE];
        struct compressed_bio *cb = bio->bi_private;
        u8 *cb_sum = cb->sums;
@@ -575,10 +574,8 @@ static noinline int add_ra_bio_pages(str
                        if (zero_offset) {
                                int zeros;
                                zeros = PAGE_SIZE - zero_offset;
-                               userpage = kmap_atomic(page);
-                               memset(userpage + zero_offset, 0, zeros);
+                               zero_user(page, zero_offset, zeros);
                                flush_dcache_page(page);
-                               kunmap_atomic(userpage);
                        }
                }
 
@@ -1239,7 +1236,6 @@ int btrfs_decompress_buf2page(const char
        unsigned long prev_start_byte;
        unsigned long working_bytes = total_out - buf_start;
        unsigned long bytes;
-       char *kaddr;
        struct bio_vec bvec = bio_iter_iovec(bio, bio->bi_iter);
 
        /*
@@ -1270,9 +1266,8 @@ int btrfs_decompress_buf2page(const char
                                PAGE_SIZE - (buf_offset % PAGE_SIZE));
                bytes = min(bytes, working_bytes);
 
-               kaddr = kmap_atomic(bvec.bv_page);
-               memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
-               kunmap_atomic(kaddr);
+               memcpy_to_page(bvec.bv_page, bvec.bv_offset, buf + buf_offset,
+                              bytes);
                flush_dcache_page(bvec.bv_page);
 
                buf_offset += bytes;


Yay!!!  The 'iteration' is not even needed...  :-D  So sorry for bothering you
on that point.

However, I still have an issue with the second rule you specified.  It appears
to be too aggressive with removing kaddr; removing it from an entirely
unrelated function.  Why?  Is there a way to limit the scope of the second rule
to blocks and parent blocks?

And the final questoin is: Why is kaddr correctly removed from
btrfs_decompress_buf2page() while the previous match 'userpage' is not?  It
seems like exactly the same pattern...

So obviously don't fully understand.

Thanks so much for your help though!  :-D
Ira

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

Re: [Cocci] Itterating matches

[Julia Lawall]

> That said it does not seem to work.  I have a better example now.
>
> https://elixir.bootlin.com/linux/v5.10-rc6/source/fs/btrfs/compression.c
>
> fs/btrfs/compression.c:
>
>  571                 if (page->index == end_index) {
>  572                         char *userpage;
>  573                         size_t zero_offset = offset_in_page(isize);
>  574
>  575                         if (zero_offset) {
>  576                                 int zeros;
>  577                                 zeros = PAGE_SIZE - zero_offset;
>  578                                 userpage = kmap_atomic(page);
>  579                                 memset(userpage + zero_offset, 0, zeros);
>  580                                 flush_dcache_page(page);
>  581                                 kunmap_atomic(userpage);
>  582                         }
>  583                 }
>
> <script>
> @ r1 @
> identifier page;
> identifier ptr;
> expression L;
> expression Off;
> @@
>
> -ptr = kmap_atomic(page);
> ...
> -memset(ptr + Off, 0, L);
> +zero_user(page, Off, L);
> ...
> -kunmap_atomic(ptr);
>
>
> @@
> identifier r1.ptr;
> type VP;
> @@
>
> -VP ptr;
>         ... when != ptr;
> </script>
>
> Results in:
>
> --- fs/btrfs/compression.c
> +++ /tmp/cocci-output-1442863-cb5dd7-compression.c
> @@ -575,10 +575,8 @@ static noinline int add_ra_bio_pages(str
>                         if (zero_offset) {
>                                 int zeros;
>                                 zeros = PAGE_SIZE - zero_offset;
> -                               userpage = kmap_atomic(page);
> -                               memset(userpage + zero_offset, 0, zeros);
> +                               zero_user(page, zero_offset, zeros);
>                                 flush_dcache_page(page);
> -                               kunmap_atomic(userpage);
>                         }
>                 }
>
>
> Question: Is there something about userpage being declared in the outer block
> which is causing the second rule to fail?

The problem is the while loop. The ... when != ptr goes to the end of the
function.  It thus goes around the loop and reaches the use of userpage
again.  You can do:

type VP, VP1;

- VP ptr;
  ... when != ptr;
? VP1 ptr;

Then it will check that there is no use of ptr until reaching a new
declaration of ptr, if any (the ? ).

>
>
> Back to the 'iteration issue'.  I've realized that I understood perfectly what
> the iteration dot notations meant...  My bug was elsewhere.
>
> For others who may be following along at home, I'll walk through this because
> it brings up another issue with the removal of the pointer declaration which
> I don't understand.
>
> That same file (fs/btrfs/compression.c) has a second instance I'm trying to
> match; except with memcpy().
>
> 1242         char *kaddr;
> ...
> 1267         /* copy bytes from the working buffer into the pages */
> 1268         while (working_bytes > 0) {
> 1269                 bytes = min_t(unsigned long, bvec.bv_len,
> 1270                                 PAGE_SIZE - (buf_offset % PAGE_SIZE));
> 1271                 bytes = min(bytes, working_bytes);
> 1272
> 1273                 kaddr = kmap_atomic(bvec.bv_page);
> 1274                 memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
> 1275                 kunmap_atomic(kaddr);
> ...
>
>
> Here I added <... ...> (and even tried <+... ...+>) around the
> k[un]map_atomic() in my script to try and catch the second instance along with
> the memcpy() as an alternative match; thusly.
>
> <script>
> @ r1 @
> identifier page;
> identifier ptr;
> expression L;
> expression F;
> expression B;
> expression Off;
> @@
>
> <...
> -ptr = kmap_atomic(page);
> ...
> (
> -memset(ptr + Off, 0, L);
> +zero_user(page, Off, L);
> |
> -memcpy(ptr + Off, F, B);
> +memcpy_to_page(page, Off, F, B);
> )
> ...
> -kunmap_atomic(ptr);
> ...>

I don't see the need for the outer <... ...>.  It looks like the pattern
inside the <... ...> should already match lines 1273-1275 in the code.


>
> @@
> identifier r1.ptr;
> type VP;
> @@
>
> -VP ptr;
>         ... when != ptr;
> </script>
>
> Which results in:
>
> --- fs/btrfs/compression.c
> +++ /tmp/cocci-output-1443266-8381d0-compression.c
> @@ -575,10 +575,8 @@ static noinline int add_ra_bio_pages(str
>                         if (zero_offset) {
>                                 int zeros;
>                                 zeros = PAGE_SIZE - zero_offset;
> -                               userpage = kmap_atomic(page);
> -                               memset(userpage + zero_offset, 0, zeros);
> +                               zero_user(page, zero_offset, zeros);
>                                 flush_dcache_page(page);
> -                               kunmap_atomic(userpage);
>                         }
>                 }
>
> :-/
>
> Finally, after thinking on this more I realized this was _not_ because the
> _iteration_ was wrong.  Rather the second kmap_atomic uses 'bvec.bv_page' which
> matches as an expression not an identifier.  The correct script is therefore:
>
> <script>
> 10:59:30 > cat kmap-test.cocci
> @ r1 @
> expression page;                <============ 'expression'
> identifier ptr;
> expression L;
> expression F;
> expression B;
> expression Off;
> @@
>
> -ptr = kmap_atomic(page);
> ...
> (
> -memset(ptr + Off, 0, L);
> +zero_user(page, Off, L);
> |
> -memcpy(ptr + Off, F, B);
> +memcpy_to_page(page, Off, F, B);
> )
> ...
> -kunmap_atomic(ptr);
>
>
> @@
> identifier r1.ptr;
> type VP;
> @@
>
> -VP ptr;
>         ... when != ptr;
> </script>
>
> which results in...
>
> --- fs/btrfs/compression.c
> +++ /tmp/cocci-output-1443375-182695-compression.c
> @@ -145,7 +145,6 @@ static int check_compressed_csum(struct
>         const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
>         struct page *page;
>         unsigned long i;
> -       char *kaddr;
>         u8 csum[BTRFS_CSUM_SIZE];
>         struct compressed_bio *cb = bio->bi_private;
>         u8 *cb_sum = cb->sums;
> @@ -575,10 +574,8 @@ static noinline int add_ra_bio_pages(str
>                         if (zero_offset) {
>                                 int zeros;
>                                 zeros = PAGE_SIZE - zero_offset;
> -                               userpage = kmap_atomic(page);
> -                               memset(userpage + zero_offset, 0, zeros);
> +                               zero_user(page, zero_offset, zeros);
>                                 flush_dcache_page(page);
> -                               kunmap_atomic(userpage);
>                         }
>                 }
>
> @@ -1239,7 +1236,6 @@ int btrfs_decompress_buf2page(const char
>         unsigned long prev_start_byte;
>         unsigned long working_bytes = total_out - buf_start;
>         unsigned long bytes;
> -       char *kaddr;
>         struct bio_vec bvec = bio_iter_iovec(bio, bio->bi_iter);
>
>         /*
> @@ -1270,9 +1266,8 @@ int btrfs_decompress_buf2page(const char
>                                 PAGE_SIZE - (buf_offset % PAGE_SIZE));
>                 bytes = min(bytes, working_bytes);
>
> -               kaddr = kmap_atomic(bvec.bv_page);
> -               memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
> -               kunmap_atomic(kaddr);
> +               memcpy_to_page(bvec.bv_page, bvec.bv_offset, buf + buf_offset,
> +                              bytes);
>                 flush_dcache_page(bvec.bv_page);
>
>                 buf_offset += bytes;
>
>
> Yay!!!  The 'iteration' is not even needed...  :-D  So sorry for bothering you
> on that point.
>
> However, I still have an issue with the second rule you specified.  It appears
> to be too aggressive with removing kaddr; removing it from an entirely
> unrelated function.  Why?  Is there a way to limit the scope of the second rule
> to blocks and parent blocks?

Indeed the rule does nothing to check that the function is the same.  I
kind of assumed that good code would not contain unused variables, and if
the code does contain unused variables you would be happy to be rid of
them.  However, it is possible with some scripting hacks to find the name
of the function that one rule matches and then use that information to
filter the matches of another rule.  But again, I'm not sure it is worth
the trouble.

julia


>
> And the final questoin is: Why is kaddr correctly removed from
> btrfs_decompress_buf2page() while the previous match 'userpage' is not?  It
> seems like exactly the same pattern...
>
> So obviously don't fully understand.
>
> Thanks so much for your help though!  :-D
> Ira
>
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Itterating matches

[Ira Weiny]

On Mon, Nov 30, 2020 at 08:59:42PM +0100, Julia Lawall wrote:

[snip]

> >
> >
> > Question: Is there something about userpage being declared in the outer block
> > which is causing the second rule to fail?
> 
> The problem is the while loop. The ... when != ptr goes to the end of the
> function.  It thus goes around the loop and reaches the use of userpage
> again.  You can do:
> 
> type VP, VP1;
> 
> - VP ptr;
>   ... when != ptr;
> ? VP1 ptr;
> 
> Then it will check that there is no use of ptr until reaching a new
> declaration of ptr, if any (the ? ).

Thanks!

[snip]

> >
> >
> > Yay!!!  The 'iteration' is not even needed...  :-D  So sorry for bothering you
> > on that point.
> >
> > However, I still have an issue with the second rule you specified.  It appears
> > to be too aggressive with removing kaddr; removing it from an entirely
> > unrelated function.  Why?  Is there a way to limit the scope of the second rule
> > to blocks and parent blocks?
> 
> Indeed the rule does nothing to check that the function is the same.  I
> kind of assumed that good code would not contain unused variables, and if
> the code does contain unused variables you would be happy to be rid of
> them.

But the kaddr in check_compressed_csum() is used?

 148         char *kaddr;
 149         u8 csum[BTRFS_CSUM_SIZE];
 150         struct compressed_bio *cb = bio->bi_private;
 151         u8 *cb_sum = cb->sums;
 152 
 153         if (inode->flags & BTRFS_INODE_NODATASUM)
 154                 return 0;
 155 
 156         shash->tfm = fs_info->csum_shash;
 157 
 158         for (i = 0; i < cb->nr_pages; i++) {
 159                 page = cb->compressed_pages[i];
 160 
 161                 kaddr = kmap_atomic(page);
 162                 crypto_shash_digest(shash, kaddr, PAGE_SIZE, csum);
 163                 kunmap_atomic(kaddr);

It correctly leaves this kmap_atomic alone for me.

> However, it is possible with some scripting hacks to find the name
> of the function that one rule matches and then use that information to
> filter the matches of another rule.  But again, I'm not sure it is worth
> the trouble.

Nope...  I can deal with it.  :-D  This is so much better than what I had.

Thanks again for your help,
Ira

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

Re: [Cocci] Itterating matches

[Julia Lawall]

> But the kaddr in check_compressed_csum() is used?
>
>  148         char *kaddr;
>  149         u8 csum[BTRFS_CSUM_SIZE];
>  150         struct compressed_bio *cb = bio->bi_private;
>  151         u8 *cb_sum = cb->sums;
>  152
>  153         if (inode->flags & BTRFS_INODE_NODATASUM)
>  154                 return 0;
>  155
>  156         shash->tfm = fs_info->csum_shash;
>  157
>  158         for (i = 0; i < cb->nr_pages; i++) {
>  159                 page = cb->compressed_pages[i];
>  160
>  161                 kaddr = kmap_atomic(page);
>  162                 crypto_shash_digest(shash, kaddr, PAGE_SIZE, csum);
>  163                 kunmap_atomic(kaddr);
>
> It correctly leaves this kmap_atomic alone for me.

OK, that's strange.  I will take a look.

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

Re: [Cocci] Automating a kmap/mem*/kunmap transformation

[Ira Weiny]

On Sun, Nov 29, 2020 at 11:17:14AM +0100, Markus Elfring wrote:
> > 1) How do I get matches to iterate?
> 
> How do you think about possibilities for “iteration”?
> 
> 
> >	1a) What is the difference between '<+... ...+>' and '<... ...>'?
> 
> I suggest to take another look at the section “Dot variants” in the available
> software documentation.
> https://coccinelle.gitlabpages.inria.fr/website/docs/main_grammar004.html#sec7
> 
> Would you like to distinguish if you are looking for source code parts
> which can be optional?

Yes, not every file will have the pattern I'm looking for and as I explained in
my other thread the second instance of that pattern was not being matched and I
could not understand why.  In that case either '+' or no '+' should (and as I
know now does) work.  But I thought there was some secret to them I was
missing...  Sorry...

> 
> 
> > 2) Why can't I get the type declaration (type VP;) matched correctly.  It works
> >    some times but not always.  It also matches a lot of random declarations?
> 
> I imagine that it will help to point a few source code places out
> where you find the shown transformation approach improvable.
> 
> 
> > 2a) and for some reason in the more advanced script[3] it completely breaks.
> 
> How does such an undesirable failure look like?
> 
> I wonder a bit more about the detail why you would like to enclose
> the SmPL code by the construct “<... … ...>” here.

Because of the second instance of my pattern not matching I thought I needed to
specify that the pattern could appear 0 or more times.  But I now realize that
the pattern specification was wrong.

> I find that the change specification “-VP ptr;” would mean then that
> all variable declarations should be deleted for selected source files.


> 
> 
> Would you become interested in any more code alternatives for scripts
> in the semantic patch language?
> 
> Examples:
> A)
> (
> -ptr = kmap(page);
> |
> -ptr = kmap_atomic(page);
> )
> 
> 
> This SmPL disjunction for a deletion approach can be transformed
> like the following.
> 
> -ptr = \( kmap \| kmap_atomic \) (page);

Thanks yes that helps.

> 
> 
> B)
> (
> -memset(...);
> +memIra();
> |
> -memcpy(...);
> +memIra();
> )
> 
> 
> This SmPL disjunction for a replacement approach can eventually be transformed
> like the following.
> 
> -\( memset \| memcpy \) (...)
> +memIra()
>  ;

This makes sense but it is not my final goal and the final goal requires more
manipulation of the parameters to memset/memcpy.

But I am curious to learn more so I'll keep these alternatives in mind.

Thanks!
Ira

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