On Fri, 26 Jul 2024 at 01:16, Xingtao Yao (Fujitsu)
<yaoxt.f...@fujitsu.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Peter Maydell <peter.mayd...@linaro.org>
> > Sent: Thursday, July 25, 2024 11:14 PM
> > To: Yao, Xingtao/姚 幸涛 <yaoxt.f...@fujitsu.com>
> > Cc: Philippe Mathieu-Daudé <phi...@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é <phi...@linaro.org>
> > > > Sent: Monday, July 22, 2024 2:43 PM
> > > > To: Yao, Xingtao/姚 幸涛 <yaoxt.f...@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
