> -----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