On Wed, Jan 22, 2020 at 08:08:32PM +0300, Alexander Monakov wrote:
>
>
> On Wed, 22 Jan 2020, Stefan Schulze Frielinghaus wrote:
>
> > Hi David,
> >
> > In function `tree_cmp` an invariant [1] is assumed which does not
> > necessarily
> > exist. In case both input trees are finally compared via `strcmp`, then
> >
> > tree_cmp (t1, t2) == -tree_cmp (t2, t1)
> >
> > does not hold in general, since function `strcmp (x, y)` guarantees only
> > that a
> > negative integer is returned in case x<y or a positive integer in case x>y.
> > Currently this breaks s390x where, for example, for certain inputs x,y
> > `tree_cmp (x, y) == 1` and `tree_cmp (y, x) == -2` hold. The attached patch
> > normalizes the output from `strcmp` to -1, 0, or 1 while using an auxiliary
> > function `sign` (stolen from the Hacker's Delight book ;-)).
> >
> > Bootstrapped and tested on s390x. Any thoughts?
>
> It's more appropriate to fix the assert rather than the comparator, like
>
> gcc_assert (sign (reversed) == -sign (result));
>
> But qsort_chk already checks that, and more, so why is the assert there?
> Shouldn't it be simply removed?
Yeah. Note there is also return DECL_UID (t1) - DECL_UID (t2); that also
doesn't guarantee -1/0/1 return values, so the patch as posted isn't
sufficient.
Jakub
