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

[Jason King]

I've updated the bug description with the updated information.  The
improved test case can be see at:
http://www.illumos.org/attachments/151/test.c

The new webrev is available at http://cr.illumos.org/view/hool5vqu/

Testing was successful on x86 & sparc (32 & 64bit on each).


On Tue, Jan 25, 2011 at 8:34 PM, Jason King <jason.brian.king at gmail.com> wrote:
> 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
>>
>>
>>
>