Re: [lmi] feholdexcept()

[Greg Chicares]

On 2017-01-08 23:53, Vadim Zeitlin wrote:
> 
>  I'm very sorry, I completely misunderstood how feholdexcept() worked and,
> worse, didn't actually check that it worked the way I thought it did. After
> looking closer into it, feholdexcept() only seems to prevent floating point
> exceptions from being turned into "real" asynchronous exceptions. This is
> rather confusing because there is nothing in the standard allowing to
> enable turning FP exceptions into real exceptions in the first place -- but
> the idea is, apparently, that the caller might do it in some non-portable
> way (e.g. GNU libc feenableexcept() or MSVC _controlfp() or just, indeed,
> direct control register manipulation using inline assembler) and the
> standard at least provides a way to for libraries to protect themselves
> against this by using feholdexcept() on entry to the library function and
> feupdateenv() when leaving it in order to avoid unexpected exceptions being
> thrown.

Umm...okay, I guess...

>  However -- and, again, I'm very sorry for not understanding nor testing it
> before -- feholdexcept() doesn't actually affect setting the bits of the
> control word at all. So, as can be seen by running this simple program:

[...extensively snipped...]

> int main(int argc, char const* argv[])
> {
[...]
>     float x = argc / 193.0f;
> 
> which outputs
> 
> ---------------------------------- >8 --------------------------------------
> Initially:      00000000
> After div:      00000020
> ---------------------------------- >8 --------------------------------------

What 'argv' did you use? (I guess anything other than an exact multiple
of 193.)

$grep 20 /usr/share/mingw-w64/include/fenv.h 
#define FE_INEXACT      0x20

Just to rule out any gcc defect...does that program behave the same way
with msvc? Even though this is C99 stuff, AIUI they use Plauger's C++
standard library, so dinkumware might have tested these functions just
because they're imported into <cfenv>.

> They will still be set irrespectively of whether feholdexcept() was called
> or not.

But that's okay. The flags in the status word are often raised,
especially FE_INEXACT. Masking them in the control word doesn't keep
them from being raised; it just prevents them from causing interrupts.

What we're concerned about is a rogue DLL changing the control word's
interrupt mask. That's what could do real harm, so that's what we
want to test.

>  And this means that my changes were completely wrong because we can't
> check that 0 == std::fetestexcept(FE_ALL_EXCEPT) in fenv_is_valid() as this
> check fails all the time and, in fact, I've implemented a nice DoS attack
> against myself in this way, because I can't even close lmi currently as it
> just keeps spawning message box after message box... This is a problem in
> its own right, I think we should have some limit on the number of message
> boxes shown (per minute?)

I'm not sure that's a good idea. Suppose we've used up the present minute's
quota of messageboxes with non-urgent diagnostics, and then a really urgent
one (like "out of memory") comes along...and is deferred, potentially for
fifty-nine seconds.

> but in the meanwhile we need to just remove this
> check for fetestexcept() and, probably, feholdexcept() call as well as it's
> useless. Here is the trivial patch doing just this:

I'm not yet convinced about *all* of this:

> ---------------------------------- >8 --------------------------------------
> diff --git a/fenv_lmi.cpp b/fenv_lmi.cpp
> index 351400d..db5e9af 100644
> --- a/fenv_lmi.cpp
> +++ b/fenv_lmi.cpp
> @@ -65,8 +65,6 @@ void fenv_initialize()
>  #if defined LMI_X87
>      x87_control_word(default_x87_control_word());
>  #else  // !defined LMI_X87
> -    std::fenv_t saved_env;
> -    LMI_ASSERT(0 == std::feholdexcept(&saved_env));
>      LMI_ASSERT(0 == std::fesetround(FE_TONEAREST));

Here, the intention is to ensure that all exceptions are masked. AFAICT
from reading the standard:
  http://lists.nongnu.org/archive/html/lmi/2017-01/msg00007.html
feholdexcept() really does do that--it...
  "installs a non-stop (continue on floating-point exceptions) mode"
and footnote 189 says we can use that to...
  "hide spurious floating-point exceptions from their callers"
That's exactly what we want to do--in your example above, we want to
hide the div-by-193 --> FE_INEXACT exception.

Of course, I've only proved this, but haven't actually tested it; but
it would be really weird if gcc's implementation failed to conform
to these clear requirements.

> @@ -164,7 +162,7 @@ bool fenv_is_valid()
>  #if defined LMI_X87
>      return default_x87_control_word() == x87_control_word();
>  #else  // !defined LMI_X87
> -    return FE_TONEAREST == std::fegetround() && 0 == 
> std::fetestexcept(FE_ALL_EXCEPT);
> +    return FE_TONEAREST == std::fegetround();
>  #endif // !defined LMI_X87

Okay, that's what we misunderstood: fetestexcept() queries the *status*
word, not the *control* word. It informs us that dividing by 193 was an
inexact operation, but we don't care. What we really want to ask is
whether the control word is still set to mask all exceptions.

*Maybe* we could test that by calling fegetenv(), obtaining an fenv_t
that "represents the entire floating-point environment" [7.6/3]. However
(as I think we recently discussed), we don't necessarily know whether
"entire" means the limited entirety contemplated by the standard, or
the plenary entirety of the actual hardware; and, because fenv_t is
utterly opaque, we don't know whether it has the Comparable nature.
(If it does, then it's only in a limited sense: cygwin's FE_DFL_ENV
is zero, so we can't compare to *FE_DFL_ENV.)

Perhaps we should unit-test this. E.g., using asm to talk directly
to the hardware where required:
 - mask all exceptions
 - call fegetenv() and store what it gives us
 - use asm to un-mask exceptions
 - call fegetenv() again
 - attempt to compare the "before" and "after" fenv_t objects.
If we can prove that this works for a particular compiler on a particular
platform, then we can write production code with standard functions only,
confining asm to a unit test that proves the production code is valid.

>  Finally, if you're wondering why/where does the original FP exception
> happen in the first place, it is in grow_lf70() function in wx/hashmap.h
> header which is used by wxHashMap<> which is itself used quite extensively
> by wxWidgets and happens to be called during each event loop iteration,
> which explains the infinite flood of message boxes. I don't know why does
> this function use division instead of multiplication, except that it has
> always been like this, ever since wxHashMap was added 15 years ago. I can
> (and probably will) modify it, but it doesn't change the fact that checking
> fetestexcept() result is still wrong as it's clearly very easy to do
> something resulting in a FP exception if a simple division like above can
> do it (or a simple multiplication: multiplying 193 by 0.85 still raises
> precision loss flag), and we don't want to show a message box (let alone
> tons of them) when this happens.

I habitually strive to replace division by a constant with multiplication
by the constant's reciprocal wherever that seems advantageous; but maybe
that's not actually helpful, because a good optimizing compiler would do
that already if it is actually advantageous. Thus, I'm not sure you
should change grow_lf70() at all. And even if you do, (1/193) is not an
exact binary number, so it's likely to use about as many mantissa bits as
the hardware offers for its approximation, and multiplying it by any number
except perhaps a very small integer is probably going to raise FE_INEXACT.