Re: [lmi] [PATCH] Better support for non-x87 platforms

[Greg Chicares]

On 2017-01-03 15:14, Vadim Zeitlin wrote:
> 
>  As promised, here are the changes which allow to set rounding mode when
> not using x87 and so make the "round" and "round_to" tests pass in 64 bit
> builds: https://github.com/vadz/lmi/pull/49
> 
>  I left a few comments there concerning some perhaps slightly less obvious
> points, but globally the changes are pretty straightforward: we keep using
> the same code as now, with as few changes as possible, when using x87 and
> use the standard C++11 functions otherwise. I've checked that all tests now
> pass under Linux in x86-64 build and that they still pass in the standard
> 32 bit MinGW build. I also tested the interactive floating point
> environment test in the GUI in the latter (it wouldn't do anything, and so
> doesn't even exist, in the former).

But doesn't the menuitem still exist, because 'menus.xrc' is unchanged?
It still specifies:
    <object class="wxMenuItem" name="test_floating_point_environment">
        <label>Test _floating point environment</label>
    </object>
and what is the effect of selecting that menuitem in an x86_64 build?
(I really should build it so that I can see for myself, but I haven't
gotten around to that yet.) (Also see the last comment below.)

I was at first surprised that you don't use __STDC_IEC_559__ anywhere,
but apparently that macro is for C only and is neither mentioned in
the C++ standard nor incorporated therein by reference:
  
http://stackoverflow.com/questions/23802294/why-does-the-c-standard-not-mention-stdc-iec-559
so I'll expunge all mention of it and of the FENV_ACCESS pragma.

Let me address some of the comments at
  https://github.com/vadz/lmi/pull/49
here:

* fenv_lmi.cpp
| Dropping support for Borland could simplify the preprocessor checks
| in this file a bit, and I think it should be done, but I decided
| against doing it in this patch to keep the changes well separate.

Given this:
  https://en.wikipedia.org/wiki/C%2B%2BBuilder#10_Seattle
  "updates the C++ compiler suite to CLANG 3.3"
it's safe to assume we'll never support any borland compiler again
except to the extent we use clang directly. I've gradually been
removing this compiler-specific code:
  $git log -S"__BORLANDC__"
However, for commit da7e969c5502c7bb1bd9b7626d05db31fa97592a IIRC,
it really was handy to have an old conditional code alternative
that I could clone for a different purpose; that's the real reason
I have for keeping any of it.

    if(default_x87_control_word() != x87_control_word())
        {
        oss
            << "The floating-point control word was unexpectedly '"
            << std::hex << std::internal << std::showbase << std::setfill('0')
            << std::setw(6) << x87_control_word()
            << "'.\n"
            ;
        }

| The only real change to the existing code is the addition of an
| extra \n here. It simplifies the code of this function slightly
| and I think an extra blank line makes the text of the warning more
| readable, but please let me know if you disagree.

Actually, I found this the toughest section to review: the other changes
are mostly mechanical, but this one is more complicated. At first, it's
pretty shocking that fenv_is_valid(), formerly a lightweight function
that simply compared two 16-bit words, now constructs a std::string and
passes it to a function that constructs a std::ostringstream. I suppose
you're doing that to avoid repeating the tests that <cfenv> requires,
though we could just as well test this in fenv_is_valid():
  fenv_rounding() != fe_tonearest || 0 != fetestexcept(FE_ALL_EXCEPT)
and call the reporting function if that fails. But what really bothers
me is that we have to go to such trouble to test so few bits. I thought
of doing something like this:

    // Set the rounding bits and interrupt mask the way we want,
    // then save them somewhere:
    std::fenv_t desired_settings;
    std::fegetenv(&desired_settings);
    // and later compare the whole environment:
    std::fenv_t current_settings;
    std::fegetenv(&current_settings);
    assert(desired_settings == current_settings);

but type fenv_t is so opaque that it's not even equality-comparable.
I could compare the values bit by bit, but that's too much work, and
I'm not sure it's even guaranteed to do the right thing. And we
mustn't even think of doing this:

    assert(*FE_DFL_ENV == current_settings);
           ^ segfault!

because at least MinGW-w64 and Cygwin's newlib define FE_DFL_ENV to
equal zero. It's certainly a pointer, but it cannot be dereferenced.
The C99 standard says [7.6/3] that fenv_t "represents the entire
floating-point environment", which, for x87, must include the
precision bits--and it's the only "entire" representation, but it's
utterly opaque, so there's no standard way to detect whether the
precision bits have changed. This is incompatible with the spirit
of C. What were these guys thinking? Why not, for instance, require
fenv_t to be an implementation-defined struct?

Anyway, I guess it's okay to add another '\n' here in the middle of
a string that already has about half a dozen newlines.

| +    int const enabled_fp_exceptions = fetestexcept(FE_ALL_EXCEPT);
|
| I hesitated for quite some before adding this code because I don't
| think this changes the results of any computations.

I think this is clearly necessary. We recently reviewed how other
projects monitor the FP control word for harmful changes, and I
remember seeing reports that some DLLs actually unmask exceptions.

* fenv_lmi.hpp
| -/// The precision functions similarly resemble GNU/Linux functions
| -/// fe[gs]etprecision().
|
| I removed the mention of these functions because they don't seem
| to actually exist.

In commit da7e969c5502c7bb1bd9b7626d05db31fa97592a I clarified one
old reference like that:

-/// Precision-control values used by 80x87 hardware. The enumerators
-/// are lowercase versions of the GNU/Linux <fenvwm.h> macros,
-/// although the constant-expressions may differ.
+/// IEEE 754 precision-control values used by x87 hardware.
+///
+/// Cf. the cognate macros in WG14 N751/J11.
+///
+/// The enumerators are prefixed lowercase versions of those cognates,

Perhaps I'll retain the comment but make it refer to that proposal.

[...defining fenv_precision() iff LMI_X87 is defined...]

| Another alternative could be to provide these functions but always
| return the fixed fe_dblprec value from the accessor and throw from
| the setter.

I suspect that alternative is preferable. Otherwise, we have to write
LMI_X87 conditionals each time these functions are called. But first
I'd better look into 'math_functors_test.cpp', because I'm not sure
how this code:

void sample_results()
{
    fenv_initialize();
    fenv_precision(fe_ldblprec);

can even compile with x86_64.

* skeleton.cpp

| +#if defined LMI_X87
   void Skeleton::UponTestFloatingPointEnvironment(wxCommandEvent&)
| It didn't seem useful to neither keep a test doing nothing nor
| creating a (necessarily) completely different and artificial
| version of this test for non-x87.

I think we had concluded that we should keep Skeleton::UponTimer(),
and make it test for changes in the SSE as well as the x87 environment.
Even though the precision can't change, the interrupt mask can, and we
found other software that runs a similar test for exactly that reason.
But we can defer that until after I get this patchset committed.