| Message ID | 20181205142305.15361-4-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: use offset_in_page and PAGE_ALIGNED | expand |
On Wed, 5 Dec 2018, Johannes Thumshirn wrote: > Constructs like 'var & (PAGE_SIZE - 1)' or 'var & ~PAGE_MASK' can be > replaced by the offset_in_page() macro instead of open-coding it. > > Add a coccinelle semantic patch to ease detection and conversion of these. > > This unfortunately doesn't account for the case when we want PAGE_ALIGNED() > instead of offset_in_page() yet. > > Cc: Julia Lawall <julia.lawall@lip6.fr> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > scripts/coccinelle/api/offset_in_page.cocci | 81 +++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 scripts/coccinelle/api/offset_in_page.cocci > > diff --git a/scripts/coccinelle/api/offset_in_page.cocci b/scripts/coccinelle/api/offset_in_page.cocci > new file mode 100644 > index 000000000000..ea5b3a8e0390 > --- /dev/null > +++ b/scripts/coccinelle/api/offset_in_page.cocci > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/// > +/// Use offset_in_page macro on address instead of explicit computation. > +/// > +// Confidence: High > +// Keywords: offset_in_page > +// Comment: Based on vma_pages.cocci > + > +virtual context > +virtual patch > +virtual org > +virtual report > + > + > +//---------------------------------------------------------- > +// For context mode > +//---------------------------------------------------------- > + > +@r_context depends on context && !patch && !org && !report@ > +expression E; > +@@ > + > +( > +* E & ~PAGE_MASK > +| > +* E & (PAGE_SIZE - 1) > +) > + > + > +//---------------------------------------------------------- > +// For patch mode > +//---------------------------------------------------------- > + > +@r_patch depends on !context && patch && !org && !report@ > +expression E; > +type T; > +@@ > + > +( > +- E & ~PAGE_MASK > ++ offset_in_page(E) > +| > +- E & (PAGE_SIZE - 1) > ++ offset_in_page(E) The two lines above should be subsumed by the two lines below. When there is a type metavariable that has no other dependencies, an isomorphism will consider that it is either present or absent. Why not include the cast case for the context and org cases? Masahiro will ultimately commit this. I have added him to CC. Thanks for the contribution. julia > +| > +- E & ((T)PAGE_SIZE - 1) > ++ offset_in_page(E) > +) > + > +//---------------------------------------------------------- > +// For org mode > +//---------------------------------------------------------- > + > +@r_org depends on !context && !patch && (org || report)@ > +expression E; > +position p; > +@@ > + > + ( > + * E@p & ~PAGE_MASK > + | > + * E@p & (PAGE_SIZE - 1) > + ) > + > +@script:python depends on report@ > +p << r_org.p; > +x << r_org.E; > +@@ > + > +msg="WARNING: Consider using offset_in_page helper on %s" % (x) > +coccilib.report.print_report(p[0], msg) > + > +@script:python depends on org@ > +p << r_org.p; > +x << r_org.E; > +@@ > + > +msg="WARNING: Consider using offset_in_page helper on %s" % (x) > +msg_safe=msg.replace("[","@(").replace("]",")") > +coccilib.org.print_todo(p[0], msg_safe) > + > -- > 2.16.4 > >
On 05/12/2018 15:46, Julia Lawall wrote: [...] >> +@r_patch depends on !context && patch && !org && !report@ >> +expression E; >> +type T; >> +@@ >> + >> +( >> +- E & ~PAGE_MASK >> ++ offset_in_page(E) >> +| >> +- E & (PAGE_SIZE - 1) >> ++ offset_in_page(E) > > The two lines above should be subsumed by the two lines below. When there > is a type metavariable that has no other dependencies, an isomorphism will > consider that it is either present or absent. Oh OK, I'm sorry I'm not really into cocinelle so I guessed it might take some iterations. Do you have an example for this? > Why not include the cast case for the context and org cases? This is an oversight actually as I couldn't test it due to a bug in openSUSE's coccinelle rpm. Thanks, Johannes
On Thu, 6 Dec 2018, Johannes Thumshirn wrote: > On 05/12/2018 15:46, Julia Lawall wrote: > [...] > >> +@r_patch depends on !context && patch && !org && !report@ > >> +expression E; > >> +type T; > >> +@@ > >> + > >> +( > >> +- E & ~PAGE_MASK > >> ++ offset_in_page(E) > >> +| > >> +- E & (PAGE_SIZE - 1) > >> ++ offset_in_page(E) > > > > The two lines above should be subsumed by the two lines below. When there > > is a type metavariable that has no other dependencies, an isomorphism will > > consider that it is either present or absent. > > Oh OK, I'm sorry I'm not really into cocinelle so I guessed it might > take some iterations. > > Do you have an example for this? Expanation 1: Coccinelle as a file standard.iso that shows the isomorphisms (rewrite rules) that may be applied to semantic patches. One of the rules is: Expression @ not_ptr1 @ expression *X; @@ !X => X == NULL So if you have a pointer typed expression X and you write a transformation on !X, it will also apply to occurrences of X == NULL in the source code. In this way, you don't have to write so many variants. Likewise there is an isomorphism: Expression @ drop_cast @ expression E; pure type T; @@ (T)E => E That is, if you have a semantic patch with (T)X, then it will also apply to code that matches just X, without the cast. The word pure means that this isomorphism metavariable has to match a semantic patch term that is a metavariable and this metavariable can't be used elsewhere. If you wrote - (char)x Then you would probably not want that to apply without the (char) cast. But if you have just - (T)x for some randome unbound metavariable T, then perhaps you don't case about the cast to T. If you actually do, then you can put disable drop_cast in the header of your rule. Explanation 2: To see what your semantic patch is really doing, you can run spatch --parse-cocci sp.cocci Here is what I get for your patch rule, with some annotations added: @@ expression E; type T; @@ ( -E >>> offset_in_page(E) -& -~-PAGE_MASK | -~ >>> offset_in_page(E) -PAGE_MASK -& -E | // the following come from // - E & (PAGE_SIZE - 1) // + offset_in_page(E) -E // 1 >>> offset_in_page(E) -& -(-PAGE_SIZE -- -1-) | -E // 2 >>> offset_in_page(E) -& -PAGE_SIZE -- -1 | -( // 3 >>> offset_in_page(E) -PAGE_SIZE -- -1-) -& -E | -PAGE_SIZE // 4 >>> offset_in_page(E) -- -1 -& -E | // the following come from: // - E & ((T)PAGE_SIZE - 1) // + offset_in_page(E) -E >>> offset_in_page(E) -& -(-(-T -)-PAGE_SIZE -- -1-) | -E // same as 1 >>> offset_in_page(E) -& -(-PAGE_SIZE -- -1-) | -E >>> offset_in_page(E) -& -(-T -)-PAGE_SIZE -- -1 | -E // same as 2 >>> offset_in_page(E) -& -PAGE_SIZE -- -1 | -( >>> offset_in_page(E) -(-T -)-PAGE_SIZE -- -1-) -& -E | -( // same as 3 >>> offset_in_page(E) -PAGE_SIZE -- -1-) -& -E | -( >>> offset_in_page(E) -T -)-PAGE_SIZE -- -1 -& -E | -PAGE_SIZE // same as 4 >>> offset_in_page(E) -- -1 -& -E ) So all the transformation generated by - E & (PAGE_SIZE - 1) + offset_in_page(E) are also generated by - E & ((T)PAGE_SIZE - 1) + offset_in_page(E) I hope that is helpful. julia
diff --git a/scripts/coccinelle/api/offset_in_page.cocci b/scripts/coccinelle/api/offset_in_page.cocci new file mode 100644 index 000000000000..ea5b3a8e0390 --- /dev/null +++ b/scripts/coccinelle/api/offset_in_page.cocci @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0 +/// +/// Use offset_in_page macro on address instead of explicit computation. +/// +// Confidence: High +// Keywords: offset_in_page +// Comment: Based on vma_pages.cocci + +virtual context +virtual patch +virtual org +virtual report + + +//---------------------------------------------------------- +// For context mode +//---------------------------------------------------------- + +@r_context depends on context && !patch && !org && !report@ +expression E; +@@ + +( +* E & ~PAGE_MASK +| +* E & (PAGE_SIZE - 1) +) + + +//---------------------------------------------------------- +// For patch mode +//---------------------------------------------------------- + +@r_patch depends on !context && patch && !org && !report@ +expression E; +type T; +@@ + +( +- E & ~PAGE_MASK ++ offset_in_page(E) +| +- E & (PAGE_SIZE - 1) ++ offset_in_page(E) +| +- E & ((T)PAGE_SIZE - 1) ++ offset_in_page(E) +) + +//---------------------------------------------------------- +// For org mode +//---------------------------------------------------------- + +@r_org depends on !context && !patch && (org || report)@ +expression E; +position p; +@@ + + ( + * E@p & ~PAGE_MASK + | + * E@p & (PAGE_SIZE - 1) + ) + +@script:python depends on report@ +p << r_org.p; +x << r_org.E; +@@ + +msg="WARNING: Consider using offset_in_page helper on %s" % (x) +coccilib.report.print_report(p[0], msg) + +@script:python depends on org@ +p << r_org.p; +x << r_org.E; +@@ + +msg="WARNING: Consider using offset_in_page helper on %s" % (x) +msg_safe=msg.replace("[","@(").replace("]",")") +coccilib.org.print_todo(p[0], msg_safe) +
Constructs like 'var & (PAGE_SIZE - 1)' or 'var & ~PAGE_MASK' can be replaced by the offset_in_page() macro instead of open-coding it. Add a coccinelle semantic patch to ease detection and conversion of these. This unfortunately doesn't account for the case when we want PAGE_ALIGNED() instead of offset_in_page() yet. Cc: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- scripts/coccinelle/api/offset_in_page.cocci | 81 +++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 scripts/coccinelle/api/offset_in_page.cocci