On Tue, Sep 29, 2020 at 6:50 AM Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
>
> On 29/09/20 18:00, H.J. Lu wrote:
> > Here is the updated patch with TUNABLE_SET_BOUNDS_IF_VALID.
> >
> > OK for master?
>
> I don't think TUNABLE_SET_BOUNDS_IF_VALID is doing what it intends to do.
>
> > +#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __maxp, __minp, __type)         \
>
> minp and maxp are switched around.

Fixed.

> > +({                                                                         \
> > +  __type min = __minp ? *((__type *) __minp) : (__cur)->type.min;          \
> > +  __type max = __maxp ? *((__type *) __maxp) : (__cur)->type.max;          \
> > +  if (__minp)                                                                      \
> > +    {                                                                              \
> > +      if (__maxp)                                                          \
> > +     {                                                                     \
> > +       if (max > min)                                                      \
> > +         {                                                                 \
> > +           (__cur)->type.min = min;                                        \
> > +           (__cur)->type.max = max;                                        \
> > +         }                                                                 \
>
> When both minp and maxp are specified, it checks if they're sane with
> respect to each other but it should also check whether the range they
> describe is a *subset* of (__cur)->type.min and (__cur)->type.max.
>
> > +     }                                                                     \
> > +      else if (max > min)                                                  \
> > +     (__cur)->type.min = min;                                              \
>
> You also need to make sure that (__cur)->type.min < min to ensure a more
> restrictive range.
>
> > +    }                                                                              \
> > +  else if (max > min)                                                              \
> > +    (__cur)->type.max = min;                                               \
>
> I did not understand this one.
>
> Basically, this is what it should look like:
>
>     if (__minp != NULL
>         && *__minp <= *__maxp

__maxp may be NULL.

>         && *__minp >= (__cur)->type.min
>         && *__minp <= (__cur)->type.max)
>       (__cur)->type.min = *__minp;
>
>     if (__maxp != NULL
>         && *__minp <= *_maxp

__minp may be NULL.

>         && *__maxp >= (__cur)->type.min
>         && *__maxp <= (__cur)->type.max)
>       (__cur)->type.max = *maxp;
>
> You could collapse some of these conditions if we assume that maxp and
> minp are always either NULL or not NULL together.
>

I don't think we should make such assumptions.  Here is the
updated patch with the check for the subset of (__cur)->type.min
and (__cur)->type.max.


-- 
H.J.