Re: [lmi] Unit test for C99 round()

[Greg Chicares]

On 2008-06-05 12:28Z, Vadim Zeitlin wrote:
> 
>  Thanks, this did help to narrow down the problem:

Okay, thanks: now I see what I did wrong.

Let's walk through the lmi 'DefectLog' template as though we were
going to fill it out, but skip over what's trivial here, like:
  Symptom: [observable behavior, with steps to reproduce]
because this isn't a process for the sake of having a process.
Stating something mindless like:
  Symptom: Unit-test failure. To reproduce: run unit test.
is just busywork in this case, and what we really want to do is
figure out what's gone wrong. You've already covered a lot of it:

*Cause: As you explain, the proximate cause is...

> that set_hardware_rounding_mode() in round_to_test.cpp() also tests for
> __STDC_IEC_559__ before LMI_X86 and so uses the standard fesetround()
> instead of LMI fenv_rounding(). So far so good, but fesetround() must be
> passed FE_TONEAREST (whose value is the same as fe_tonearest in enum
> e_ieee754_rounding defined in fenv_lmi_x86.hpp) or one of FE_DOWNWARD,
> FE_UPWARD or FE_TOWARDZEO whose values are not at all the same. So setting
> hardware rounding mode to anything but "to nearest" fails.

fesetround() takes implementation-defined arguments, while lmi's
fenv_rounding()'s arguments are actually hardware constants. The
two can't reliably, if at all, be made the same, so we need to
have the right set of values for the context we're using. Indeed
we already have that:

    enum e_ieee754_rounding
        {fe_tonearest  = FE_TONEAREST
        ,fe_downward   = FE_DOWNWARD
        ,fe_upward     = FE_UPWARD
        ,fe_towardzero = FE_TOWARDZERO
        };

but we suppressed it in the 20080602T1804Z change. AFAICT, we
broke something that was correct when "fixing" something else.

You also observe:

> And the code
> doesn't even notice it because it doesn't check the return value of
> fesetround().

Furthermore, I wrote this code probably years ago, but I never
tested the __STDC_IEC_559__ part. This week, you're testing that
for the first time. I think the ultimate cause is that I wrote
something that I couldn't test, and conditionalized it in an
inconsistent way...and now we're breaking one facet of it while
fixing another, because we're not stepping back and looking at
the entire problem.

Now we come to a meaty question:

*Genesis: [in which revision was the defect introduced?]
       * in brand-new code? refactoring? redesign?
       * while fixing a defect? the same defect?

Apparently I introduced it 20080602T1804Z:
  http://cvs.sv.gnu.org/viewvc/lmi/lmi/round_to_test.cpp?r1=1.17&r2=1.18
while fixing a defect...which is either the same defect, or a
closely related one--and it's even in the same file.

*Similar defects elsewhere: Behold...
  $grep __STDC_IEC_559__ *.?pp
Yick. Even without looking at the surrounding context, we've got:
  #if defined __STDC_IEC_559__ || defined __MINGW32__
apparently with some rationale given; but, inconsistently,
  #ifdef __STDC_IEC_559__
with no __MINGW32__ test; and, curiously,
  #if 0 && defined __STDC_IEC_559__
which suggests that I knew there was a problem, but thought it'd
be a really good idea to mask it by inserting '0 && '...in some
places, but not in others.

Moreover, 'round_test.cpp' is cloned from 'round_to_test.cpp', so
it probably inherits these problems. Or, if the 20080602T1804Z
change didn't affect that file, then maybe that file has the
problem that the 20080602T1804Z change was meant to address?

*Critique of the past: When I committed the 20080602T1804Z change
I knew I was doing the wrong thing, and satisfied myself with the
usual excuses. This is just a unit test, after all (but other
places that looked wrong are production code...and the reason for
fixing a unit test is not to "make the test pass", but rather to
ensure that production code is right). It's only a problem on a
platform not used today in commercial production (but that could
change tomorrow--or, rather, we might try to change it tomorrow;
then we'd have to re-find the problem that was swept under the
rug here). I have other things to do that are more pressing, and
committing this was "expedient" (except it wasn't: it took some
effort to debug today's problem and find that we'd stepped into
a trap of our own making, soon after we set it).

Watchwords for the future: When changing the code, leave it in
a demonstrably-correct state--or at least mark known defects.
When writing speculative code whose correctness cannot be
demonstrated, mask it out with consistent conditionals. Don't
try to fix one symptom at a time--find the root cause instead.

>  So the simplest way to fix the test is:
[snip patch]

*Repairs completed:

Patch applied. Probably a mistake, but then again it might stop
the bleeding so that we can step back and figure out the right
thing to do.

*Repairs postponed:

> To be complete I'd also add a test of fesetround() return value but then I
> can't really test it as long as I'm only testing LMI under x86. Maybe we
> should have a separate define LMI_X86_FP which would be defined by default
> if LMI_X86 is but could be manually undefined to force the use of standard
> FP functions even under x86 at least in order to test them, what do you
> think?

I don't have a compiler that conforms fully to C99 Annex F (and
therefore defines __STDC_IEC_559__). If you have one, and want to
write and validate __STDC_IEC_559__ support for every place in
lmi that mentions or should mention that macro, then of course
that would be welcome.

In that case, I don't see how a separate LMI_X86_FP macro would
be useful. We'd just write

  #ifdef __STDC_IEC_559__
      // Pure C99 code.
  #elif defined LMI_X86
      // Platform-specific alternative if C99 unsupported.
  #else ...

and everything would just work, right? Or maybe you intend such a
macro to be only temporary scaffolding?

Otherwise, here's an idea we can fall back on, which should leave
the system in a consistent and correct state. First, revert the
20080602T1804Z and 20080606T2049Z changes; then run this command
to use '(0 && defined __STDC_IEC_559__)' uniformly as the only
__STDC_IEC_559__ conditional:

$for z in `grep -l '^#.*__STDC_IEC_559__' *.?pp`; \
do sed -i -e'/^#...[^0].*__STDC_IEC_559__/\
{s/ifdef/if defined/;s/defined __STDC_IEC_559__/\
(0 \&\& defined __STDC_IEC_559__)/}' $z; \
done

and then document it in one place, e.g., 'fenv_lmi.hpp':

+// SOMEDAY !! Revisit suppressed __STDC_IEC_559__ support. See:
+//   http://lists.nongnu.org/archive/html/lmi/2008-06/[this email's URL]
 #if (0 && defined __STDC_IEC_559__) || defined __MINGW32__

and, finally, test it to make sure it works for all the platforms
we're testing. IOW, roll the system back to the point where the
original mistake was made, and unmake the mistake.

Perhaps we should do that first, no matter what else we attempt.
The purpose is to get everything stabilized and working before
trying anything new. Then we can remove all occurrences of
'|| defined __MINGW32__', to make all the complete conditionals
uniform--because I think that nonuniformity was a mistake, too.
And then we can optionally do a text replacement (I'll write
your macro here, because I think I've just reinvented it--which
seems to show we're thinking in much the same way):
  s/(0 && defined __STDC_IEC_559__)/defined LMI_X86_FP/
and develop and test a pure-C99 alternative, safely. (And when
that's done, the macro becomes simply __STDC_IEC_559__.)