| Message ID | 20240722040742.11513-1-yaoxt.fnst@fujitsu.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | make range overlap check more readable | expand |
Hi Yao, On 22/7/24 06:07, Yao Xingtao via wrote: > Currently, some components still open-coding the range overlap check. > Sometimes this check may be fail because some patterns are missed. How did you catch all these use cases? > To avoid the above problems and improve the readability of the code, > it is better to use the ranges_overlap() to do this check. > > Yao Xingtao (13): > range: Make ranges_overlap() return bool > arm/boot: make range overlap check more readable > core/loader: make range overlap check more readable > cxl/mailbox: make range overlap check more readable > display/sm501: make range overlap check more readable > aspeed_smc: make range overlap check more readable > qtest/fuzz: make range overlap check more readable > sparc/ldst_helper: make range overlap check more readable > system/memory_mapping: make range overlap check more readable > block/vhdx: make range overlap check more readable > crypto/block-luks: make range overlap check more readable > dump: make range overlap check more readable > block/qcow2-cluster: make range overlap check more readable
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Sent: Monday, July 22, 2024 2:43 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > Subject: Re: [PATCH 00/13] make range overlap check more readable > > Hi Yao, > > On 22/7/24 06:07, Yao Xingtao via wrote: > > Currently, some components still open-coding the range overlap check. > > Sometimes this check may be fail because some patterns are missed. > > How did you catch all these use cases? I used the Coccinelle to match these use cases, the pattern is below range_overlap.cocci: // use ranges_overlap() instead of open-coding the overlap check @@ expression E1, E2, E3, E4; @@ ( - E2 <= E3 || E1 >= E4 + !ranges_overlap(E1, E2, E3, E4) | - (E2 <= E3) || (E1 >= E4) + !ranges_overlap(E1, E2, E3, E4) | - E1 < E4 && E2 > E3 + ranges_overlap(E1, E2, E3, E4) | - (E1 < E4) && (E2 > E3) + ranges_overlap(E1, E2, E3, E4) | - (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) + ranges_overlap(E1, E2, E3, E4) | - ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) + ranges_overlap(E1, E2, E3, E4) ) then execute the command: # spatch --macro-file scripts/cocci-macro-file.h --sp-file range_overlap.cocci --keep-comments --in-place --use-gitgrep --dir . but some of the matched cases are not valid and need to be manually judged. there may be cases that have not been matched yet. > > > To avoid the above problems and improve the readability of the code, > > it is better to use the ranges_overlap() to do this check. > > > > Yao Xingtao (13): > > range: Make ranges_overlap() return bool > > arm/boot: make range overlap check more readable > > core/loader: make range overlap check more readable > > cxl/mailbox: make range overlap check more readable > > display/sm501: make range overlap check more readable > > aspeed_smc: make range overlap check more readable > > qtest/fuzz: make range overlap check more readable > > sparc/ldst_helper: make range overlap check more readable > > system/memory_mapping: make range overlap check more readable > > block/vhdx: make range overlap check more readable > > crypto/block-luks: make range overlap check more readable > > dump: make range overlap check more readable > > block/qcow2-cluster: make range overlap check more readable
On 22/7/24 08:59, Xingtao Yao (Fujitsu) wrote: > > >> -----Original Message----- >> From: Philippe Mathieu-Daudé <philmd@linaro.org> >> Sent: Monday, July 22, 2024 2:43 PM >> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org >> Subject: Re: [PATCH 00/13] make range overlap check more readable >> >> Hi Yao, >> >> On 22/7/24 06:07, Yao Xingtao via wrote: >>> Currently, some components still open-coding the range overlap check. >>> Sometimes this check may be fail because some patterns are missed. >> >> How did you catch all these use cases? > I used the Coccinelle to match these use cases, the pattern is below > range_overlap.cocci: > > // use ranges_overlap() instead of open-coding the overlap check > @@ > expression E1, E2, E3, E4; > @@ > ( > - E2 <= E3 || E1 >= E4 > + !ranges_overlap(E1, E2, E3, E4) > | > > - (E2 <= E3) || (E1 >= E4) > + !ranges_overlap(E1, E2, E3, E4) > | > > - E1 < E4 && E2 > E3 > + ranges_overlap(E1, E2, E3, E4) > | > > - (E1 < E4) && (E2 > E3) > + ranges_overlap(E1, E2, E3, E4) > | > > - (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) > + ranges_overlap(E1, E2, E3, E4) > > | > - ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) > + ranges_overlap(E1, E2, E3, E4) > ) Please add to scripts/coccinelle/range.cocci. > > then execute the command: > # spatch --macro-file scripts/cocci-macro-file.h --sp-file range_overlap.cocci --keep-comments --in-place --use-gitgrep --dir . > > but some of the matched cases are not valid and need to be > manually judged. > > there may be cases that have not been matched yet.
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Sent: Monday, July 22, 2024 3:37 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > Subject: Re: [PATCH 00/13] make range overlap check more readable > > On 22/7/24 08:59, Xingtao Yao (Fujitsu) wrote: > > > > > >> -----Original Message----- > >> From: Philippe Mathieu-Daudé <philmd@linaro.org> > >> Sent: Monday, July 22, 2024 2:43 PM > >> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > >> Subject: Re: [PATCH 00/13] make range overlap check more readable > >> > >> Hi Yao, > >> > >> On 22/7/24 06:07, Yao Xingtao via wrote: > >>> Currently, some components still open-coding the range overlap check. > >>> Sometimes this check may be fail because some patterns are missed. > >> > >> How did you catch all these use cases? > > I used the Coccinelle to match these use cases, the pattern is below > > range_overlap.cocci: > > > > // use ranges_overlap() instead of open-coding the overlap check > > @@ > > expression E1, E2, E3, E4; > > @@ > > ( > > - E2 <= E3 || E1 >= E4 > > + !ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E2 <= E3) || (E1 >= E4) > > + !ranges_overlap(E1, E2, E3, E4) > > | > > > > - E1 < E4 && E2 > E3 > > + ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E1 < E4) && (E2 > E3) > > + ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) > > + ranges_overlap(E1, E2, E3, E4) > > > > | > > - ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) > > + ranges_overlap(E1, E2, E3, E4) > > ) > > Please add to scripts/coccinelle/range.cocci. OK, I will add this file in next revision. > > > > > then execute the command: > > # spatch --macro-file scripts/cocci-macro-file.h --sp-file range_overlap.cocci > --keep-comments --in-place --use-gitgrep --dir . > > > > but some of the matched cases are not valid and need to be > > manually judged. > > > > there may be cases that have not been matched yet.
On Mon, 22 Jul 2024 at 08:00, Xingtao Yao (Fujitsu) via <qemu-devel@nongnu.org> wrote: > > > > > -----Original Message----- > > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > Sent: Monday, July 22, 2024 2:43 PM > > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > > Subject: Re: [PATCH 00/13] make range overlap check more readable > > > > Hi Yao, > > > > On 22/7/24 06:07, Yao Xingtao via wrote: > > > Currently, some components still open-coding the range overlap check. > > > Sometimes this check may be fail because some patterns are missed. > > > > How did you catch all these use cases? > I used the Coccinelle to match these use cases, the pattern is below > range_overlap.cocci: > > // use ranges_overlap() instead of open-coding the overlap check > @@ > expression E1, E2, E3, E4; > @@ > ( > - E2 <= E3 || E1 >= E4 > + !ranges_overlap(E1, E2, E3, E4) > | Maybe I'm misunderstanding the coccinelle patch here, but I don't see how it produces the results in the patchset. ranges_overlap() takes arguments (start1, len1, start2, len2), but an expression like "E2 <= E3 || E1 >= E4" is working with start,end pairs to indicate the ranges. And looking at e.g. patch 9: - if (cur->phys_addr >= begin + length || - cur->phys_addr + cur->length <= begin) { + if (!ranges_overlap(cur->phys_addr, cur->length, begin, length)) { the kind of if() check you get for start, length pairs has an addition in it, which I don't see in any of these coccinelle script fragments. thanks -- PMM
> -----Original Message----- > From: Peter Maydell <peter.maydell@linaro.org> > Sent: Thursday, July 25, 2024 11:14 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>; qemu-devel@nongnu.org > Subject: Re: [PATCH 00/13] make range overlap check more readable > > On Mon, 22 Jul 2024 at 08:00, Xingtao Yao (Fujitsu) via > <qemu-devel@nongnu.org> wrote: > > > > > > > > > -----Original Message----- > > > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > > Sent: Monday, July 22, 2024 2:43 PM > > > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > > > Subject: Re: [PATCH 00/13] make range overlap check more readable > > > > > > Hi Yao, > > > > > > On 22/7/24 06:07, Yao Xingtao via wrote: > > > > Currently, some components still open-coding the range overlap check. > > > > Sometimes this check may be fail because some patterns are missed. > > > > > > How did you catch all these use cases? > > I used the Coccinelle to match these use cases, the pattern is below > > range_overlap.cocci: > > > > // use ranges_overlap() instead of open-coding the overlap check > > @@ > > expression E1, E2, E3, E4; > > @@ > > ( > > - E2 <= E3 || E1 >= E4 > > + !ranges_overlap(E1, E2, E3, E4) > > | > > Maybe I'm misunderstanding the coccinelle patch here, but > I don't see how it produces the results in the patchset. > ranges_overlap() takes arguments (start1, len1, start2, len2), > but an expression like "E2 <= E3 || E1 >= E4" is working > with start,end pairs to indicate the ranges. And looking > at e.g. patch 9: > > - if (cur->phys_addr >= begin + length || > - cur->phys_addr + cur->length <= begin) { > + if (!ranges_overlap(cur->phys_addr, cur->length, begin, length)) { > > the kind of if() check you get for start, length pairs > has an addition in it, which I don't see in any of these > coccinelle script fragments. I understand your confusion, but it is difficult to match the region overlap check because it has many variations, like below: case1: start >= old_start +old_len || start + len <= old_start case2: start >= old_end || end <= old_start case3: cur->phys_addr >= begin + length || cur->phys_addr + cur->length <= begin case4: new->base >= old.base +old.size || new->base + new->size <= old.base ...... and sometimes the length or end may be also an expression, I can not find a way to handle all the variants. So I decided to use the above pattern to find out the region overlap checks as much as possible, then manually drop the cases that does not meet the requirements, and then manually modify the cases that meets the requirements. > thanks > -- PMM
On Fri, 26 Jul 2024 at 01:16, Xingtao Yao (Fujitsu) <yaoxt.fnst@fujitsu.com> wrote: > > > > > -----Original Message----- > > From: Peter Maydell <peter.maydell@linaro.org> > > Sent: Thursday, July 25, 2024 11:14 PM > > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>; qemu-devel@nongnu.org > > Subject: Re: [PATCH 00/13] make range overlap check more readable > > > > On Mon, 22 Jul 2024 at 08:00, Xingtao Yao (Fujitsu) via > > <qemu-devel@nongnu.org> wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > Sent: Monday, July 22, 2024 2:43 PM > > > > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; qemu-devel@nongnu.org > > > > Subject: Re: [PATCH 00/13] make range overlap check more readable > > > > > > > > Hi Yao, > > > > > > > > On 22/7/24 06:07, Yao Xingtao via wrote: > > > > > Currently, some components still open-coding the range overlap check. > > > > > Sometimes this check may be fail because some patterns are missed. > > > > > > > > How did you catch all these use cases? > > > I used the Coccinelle to match these use cases, the pattern is below > > > range_overlap.cocci: > > > > > > // use ranges_overlap() instead of open-coding the overlap check > > > @@ > > > expression E1, E2, E3, E4; > > > @@ > > > ( > > > - E2 <= E3 || E1 >= E4 > > > + !ranges_overlap(E1, E2, E3, E4) > > > | > > > > Maybe I'm misunderstanding the coccinelle patch here, but > > I don't see how it produces the results in the patchset. > > ranges_overlap() takes arguments (start1, len1, start2, len2), > > but an expression like "E2 <= E3 || E1 >= E4" is working > > with start,end pairs to indicate the ranges. And looking > > at e.g. patch 9: > > > > - if (cur->phys_addr >= begin + length || > > - cur->phys_addr + cur->length <= begin) { > > + if (!ranges_overlap(cur->phys_addr, cur->length, begin, length)) { > > > > the kind of if() check you get for start, length pairs > > has an addition in it, which I don't see in any of these > > coccinelle script fragments. > I understand your confusion, but it is difficult to match the region overlap check because > it has many variations, like below: > case1: > start >= old_start +old_len || start + len <= old_start > > case2: > start >= old_end || end <= old_start > > case3: > cur->phys_addr >= begin + length || cur->phys_addr + cur->length <= begin > > case4: > new->base >= old.base +old.size || new->base + new->size <= old.base > ...... > and sometimes the length or end may be also an expression, I can not find a way to > handle all the variants. > > So I decided to use the above pattern to find out the region overlap checks as much as possible, > then manually drop the cases that does not meet the requirements, and then manually modify > the cases that meets the requirements. Ah, I see -- you just used coccinelle as a more flexible grep, effectively. That's fine -- it just means that when we review the series we need to review each patch for e.g. off-by-one errors rather than being able to do that only on the coccinelle patch. thanks -- PMM
Hi Yao, On 22/7/24 09:40, Xingtao Yao (Fujitsu) wrote: >> Please add to scripts/coccinelle/range.cocci. > OK, I will add this file in next revision. Please Cc me on v3 (I'm dropping v2 from my queue). Thanks, Phil.