This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] soft-fp: Fix overwritten issue for division in op-4.h


Joseph Myers <joseph@codesourcery.com> 於 2018年10月15日 週一 下午9:01寫道:
>
> On Mon, 15 Oct 2018, Zong Li wrote:
>
> > diff --git a/soft-fp/op-4.h b/soft-fp/op-4.h
> > index 01b87d0..a407168 100644
> > --- a/soft-fp/op-4.h
> > +++ b/soft-fp/op-4.h
> > @@ -517,10 +517,15 @@
> >             R##_f[_FP_DIV_MEAT_4_udiv_i] = -1;                        \
> >             if (!_FP_DIV_MEAT_4_udiv_i)                               \
> >               break;                                                  \
> > -           __FP_FRAC_SUB_4 (X##_f[3], X##_f[2], X##_f[1], X##_f[0],  \
> > +           UWtype __FP_FRAC_tmp;                                     \
> > +           __FP_FRAC_SUB_4 (X##_f[2], X##_f[1], X##_f[0], __FP_FRAC_tmp,     \
> >                              Y##_f[2], Y##_f[1], Y##_f[0], 0,         \
> >                              X##_f[2], X##_f[1], X##_f[0],            \
> >                              _FP_DIV_MEAT_4_udiv_n_f[_FP_DIV_MEAT_4_udiv_i]); \
> > +           X##_f[3] = X##_f[2];                                      \
> > +           X##_f[2] = X##_f[1];                                      \
> > +           X##_f[1] = X##_f[0];                                      \
> > +           X##_f[0] = __FP_FRAC_tmp;                                 \
>
> This approach, using the X array in a shifted form to store the result
> with a single temporary, is not a clean solution.
>
> First, we should work out what the interface *should* be to all the
> FRAC_ADD and FRAC_SUB macros, regarding permitted overlap between the
> output and the inputs.  This requires examining all those macros and their
> uses to determine what consistent or nearly consistent patterns there are
> in that interface.
>
> Given an appropriate choice of interface, all the definitions that do not
> fit that choice, and all the uses that do not fit that choice, then need
> to be fixed.  If that choice says the overlap above it not OK, a whole
> four-word temporary should be defined using _FP_FRAC_DECL_4 for the
> result, and then copied with _FP_FRAC_COPY_4.  The commit message for any
> proposed change needs to describe the conclusions regarding the interface
> for permitted overlaps, and the analysis done to determine what needs to
> change.
>
> I suspect that the intended interface is that *exact* overlap is permitted
> between the output and the *second* input, but not other overlap.  In
> which case at least the definitions of __FP_FRAC_SUB_3 and __FP_FRAC_SUB_4
> look buggy and need changing (they reference y1 and y2 after r1 and r2
> have been set), as well as the user you're changing above (which should
> use a full temporary).
>

I spend some time to check all the FRAC_ADD and FRAC_SUB use cases.

In FRAC_ADD_3 and FRAC_ADD_4(R, X, Y), it reference the x[N] after
r[N] have been set. If the x and r are the same address, the
expression r[N] < x[N] is always false(it always be equal), then the
carry bit is always zero. In glibc, all FRAC_ADD users aren't specify
the r and x with the same address, but there are some users specify
the r and y with the same address, and they are still work because
y[N] not be used again in FRAC_ADD.

In FRAC_SUB_3 and FRAC_SUB_4, it is more strict that it reference both
the x[N] and y[N] after r[N] have been set. If one of the x and y is
the same address with r, the result of the calculation is wrong
because the value of the original x and y are overwritten. In glibc,
there are two places use FRAC_SUB and occurs the overlap. The first is
_FP_DIV_MEAT_N_loop in op-common.h, it uses the source
_FP_DIV_MEAT_N_loop_u as the destination. This macro
only be used when N is one(_FP_DIV_MEAT_1_loop) and then the
_FP_FRAC_SUB_##wc extend to _FP_FRAC_SUB_1 in this macro. so it also
work because _FP_FRAC_SUB_1 has no overlap problem in its
implementation. The second places is I had modified in this patch, and
I would use the full temporary replace single temporary for next
version.

In FRAC_ADD_1, FRAC_ADD_2, FRAC_SUB_1 and FRAC_SUB_2, there don't
refer the source after destination have been set, so they have no
problem.

In conclusion, I inclines to keep the current implementation of
FRAC_SUB_3, FRAC_SUB_4, FRAC_ADD_3 and FRAC_ADD_4 instead of using the
temporary variables like FRAC_ADD_2 and FRAC_SUB_2, and fix the
_FP_DIV_MEAT_N_loop together in next version. Because using temporary
variables have to additional move the result from temporary to real
destination in the most situation. The macros user should be
responsible for ensuing no overlap
between sources and destination. I want to know whether anyone has
some suggestion, please let me know, thanks.

I test the macros by hand and glibc testsutie, and it seems that the
overlap is unreasonable. The overlap will cause the wrong result and
failed on glibc testsuite.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]