[illumos-Developer] Review for 166 CR6901979 error in xdr_float.c not fixed

[Jason King]

Doing a little more digging and and as result, more extensive testing,
it actually appears the xdr_double and xdr_float functions have been
busted on x86 at last as far back as Solaris 9 (and for amd64 as long
as the platform has been available).

Essentially, IEEE 754 standard requires the same byte ordering on all
platforms (this is the OTW format used by XDR).  As such the in memory
representation of double and float are identical on x86, amd64, sparc,
and sparcv9.  However the suggested workaround is to use
XDR_{GET,PUT}INT32 which will convert (if necessary) to big endian,
and thus is incorrect.

The fix is to use XDR_{GET,PUT}BYTES instead which will just do a straight copy.

I have an updated webrev and have tested on x86, amd64, sparc, and
sparcv9 now.  All platforms pass the (new improved) test case.

Since the website is having issues, once it is back up I will upload
the new webrev + attach the test case to the ticket.

On Sat, Jan 22, 2011 at 4:09 PM, Garrett D'Amore <garrett at damore.org> wrote:
> Generally I like the cleanup.  I'd like confirmation of correct
> execution on all the platforms that are relevant to us:
>
> sparcv8
> sparcv9
> i386
> amd64
>
> Once we are certain that the code works as it should across all of these
> platforms, I think you should move ahead with an RTI -- assuming nobody
> spots anything that I've missed.
>
>        - Garrett
>
> On Sat, 2011-01-22 at 12:59 -0600, Jason King wrote:
>> Webrev is at http://cr.illumos.org/view/ltia0hr8/
>>
>> This does a couple of things:
>>   1. Removes the code for a VAX as it seems incredibly unlikely
>> Illumos will be ported to such an old CPU.  I also suspect there was
>> probably some bugs in the code anyway.
>>   2. Changes the selection of the XDR float conversion routines based
>> on feature (i.e. IEEE 794) instead of platform.  Currently there is an
>> IEEE 794 implementation and a (slower) platform independent one.  When
>> the platform independent code is chosen, a compiler warning is
>> generated to assist porting.
>>   3. Separates the conversion routines into separate functions by
>> implementation.  The old code had lots of #ifdefs scattered throughout
>> the code, making it harder to read.   Instead a few #ifdefs at the
>> very top of the file pick the implementation to use.
>>
>> Having said all that it appears the actual conversion bug was somehow
>> fixed accidentally in the past.  I tried writing a straight float
>> value:
>>
>>     FILE *f = fopen("test", "w+);
>>     float a = 0.0F;
>>     fwrite(&a, sizeof (a), 1, f);
>>
>> Then reading it back via xdr_float, and it came back with '0.0' (both
>> when compiled as 32 & 64 bit).  I've attached the test case (it's very
>> small) if anyone can spot any issues with the test.   This is largely
>> based on the test case documented in the original Sun/Oracle bug.
>>
>> Regardless, I think there is value in the elimination of dead code and
>> improved clarity.  The proposed change produces identical results and
>> nightly compiles clean.
>> _______________________________________________
>> Developer mailing list
>> Developer at lists.illumos.org
>> http://lists.illumos.org/m/listinfo/developer
>
>
>