Re: [PATCH 1/3] New make target gen-libm-inputs

[Joseph Myers <joseph at codesourcery dot com>]

On Mon, 14 Dec 2015, Siddhesh Poyarekar wrote:

> On Fri, Dec 11, 2015 at 01:00:25PM +0000, Joseph Myers wrote:
> > I don't think adding thousands of tests like that is a good idea (unless 
> > function implementations genuinely have thousands of branches).  Rather, 
> > you should identify a minimal set of test inputs that actually improve 
> > branch coverage or increase observed ulps, and only add those inputs to 
> > auto-libm-test-in.
> 
> OK, then that will have to be a completely different workflow.  What
> do you think about this: add systemtap probes to each branch point in
> the libm code and then have a driver link against this libc and
> identify an input for each branch.  These probe points get built in
> only for the gen-libm-inputs target and should get invalidated on the
> default target.
> 
> The downside I see is that the code is slightly harder to read due to
> all of these probe points, but it shouldn't be that bad.

I don't like the idea of adding such probe points, especially in cases 
where the branches currently use ? : instead of "if".

I'd imagine something using -fprofile-arcs -ftest-coverage, or scripting a 
debugger tracing what instructions are executed / paths are followed 
inside s_sin.c, so that no source code changes are needed.  Find what 
instructions / paths are executed inside the existing tests for sin for 
double in auto-libm-test-in.  For each benchmark input for sin, if it adds 
new paths then add it to auto-libm-test-in, update the list of 
already-seen paths (to avoid adding multiple new tests for the same paths) 
and continue checking further benchmark inputs.

A generic system for testing coverage of libm tests without needing source 
code changes is much more generally useful, in that covering as much of 
glibc as possible at the source and binary level in the testsuite is 
desirable for every area of glibc.  If you can identify untested code, 
that's useful information for people wanting to improve test coverage.  
(The other part of this - given a large set of possible extra test inputs, 
find a minimal subset to add to get as much extra test coverage as the 
whole set would provide - is more specialized, in that in most cases such 
a set of candidate test inputs won't be available, though it could be used 
with random test generation to improve test coverage of libm functions 
more generally.)

> > I'm also wary of a makefile rule like this because it's likely that MPFR 
> > or MPC versions newer than the system ones are required (for example, as 
> > soon as MPC 1.1 comes out with the performance improvements for mpc_sin 
> > that went in two years ago but still haven't appeared in a release, I 
> > intend to move the remaining csin tests to auto-libm-test-in, which will 
> > mean regeneration may take over 10 minutes when using older MPC versions).
> 
> That is why this rule is optional, i.e. at the descretion of
> developers only.  That said, I could add a configure option to specify
> which MPC and MPFR you want to build against.  Would that be
> acceptable?

I don't object to such a rule if it's only using auto-libm-test-in, not 
using benchmark inputs.

> > >      FUNC_mpfr_ff_f ("pow", mpfr_pow, false),
> > > +    FUNC_mpfr_f_f ("rint", mpfr_erf, false),
> > >      FUNC_mpfr_f_f ("sin", mpfr_sin, false),
> > >      FUNC ("sincos", ARGS1 (type_fp), RET2 (type_fp, type_fp), false, false,
> > >  	  false, CALC (mpfr_f_11, mpfr_sin_cos)),
> > 
> > No.  If you want to test extra functions, add the support required to do 
> > so properly, not hacking things up to accept test inputs whose outputs are 
> > both meaningless and unused.
> 
> Sorry, I don't even remember how I came up with that bit.

I presume you were trying to avoid errors arising from functions that have 
benchmarks, but don't use auto-libm-test-* at present.

-- 
Joseph S. Myers
joseph@codesourcery.com