Re: [Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Andrew Dutcher]

Also- I'm having issues applying the new patch:

@@ -5768,7 +5774,9 @@ int floatx80_lt(floatx80 a, floatx80 b,
float_status *status)
 *----------------------------------------------------------------------------*/
 int floatx80_unordered(floatx80 a, floatx80 b, float_status *status)
 {
-    if (    (    ( extractFloatx80Exp( a ) == 0x7FFF )
+    if (    floatx80_invalid_encoding( a )
+         || floatx80_invalid_encoding( b )
+         || (    ( extractFloatx80Exp( a ) == 0x7FFF )
               && (uint64_t) ( extractFloatx80Frac( a )<<1 ) )
          || (    ( extractFloatx80Exp( b ) == 0x7FFF )
               && (uint64_t) ( extractFloatx80Frac( b )<<1 ) )

When I do this, the style checker complains about the spaces after the
open parens and before the close parens. I now have to change this
entire stanza to be styled correctly, since I'm replacing the original
first line...

On Tue, Aug 16, 2016 at 3:59 PM, Andrew Dutcher
<address@hidden> wrote:
> I explicitly left the check off the comparison operations because I
> misread the NaN check as something equivalent to the check I would be
> adding. I'll add it shortly.
>
> With regards to adding int32_indefinite, etc constants, I think I'll
> leave it as is -- I'd prefer to have *what* happens clear (return this
> number), then have *why* it happens be clear (return the integer
> indefinite value).
>
> And, finally, yes, I know what C++ and C-style comments are :P I just
> think every argument I've ever read in favor of only using the latter
> has been complete nonsense. Regardless! Guidelines are guidelines.
>
> Thanks,
> - Andrew
>
> On Tue, Aug 16, 2016 at 6:34 AM, Peter Maydell <address@hidden> wrote:
>> On 15 August 2016 at 23:27, Andrew Dutcher <address@hidden> wrote:
>>> All operations that take a floatx80 as an operand need to have their
>>> inputs checked for malformed encodings. In all of these cases, use the
>>> function floatx80_invalid_encoding to perform the check. If an invalid
>>> operand is found, raise an invalid operation exception, and then return
>>> either NaN (for fp-typed results) or the integer indefinite value (the
>>> minimum representable signed integer value, for int-typed results).
>>>
>>> Signed-off-by: Andrew Dutcher <address@hidden>
>>> ---
>>>
>>> Version 4: Remove comments, since apparently it's still 1998. If anyone 
>>> wants
>>> to know what the value is for, they can check git blame.
>>
>> The code style gripe is not for having comments, it's for
>> using the "//" style comment rather than "/* ... */". Yeah,
>> we have some odd style requirements; at least we have a script
>> which will detect them automatically. (By the way, you can run
>> ./scripts/checkpatch.pl on your patch before sending it if you
>> like; you don't have to wait for the patch robot to read your
>> email :-))
>>
>> If you liked you could  define a constant for the 32-bit and
>> 64-bit indefinite values rather than using 1 << 31 &c directly;
>> 'return int32_indefinite;' is sufficiently self-documenting
>> not to need a comment.
>>
>> I don't mind whether you do that, or not; your choice.
>>
>> The code changes you have here are good, but you've forgotten
>> one piece: the comparison ops also need to handle the invalid
>> encodings.
>>
>> floatx80_compare_internal() needs to raise float_flag_invalid
>> and return float_relation_unordered if either of its inputs are
>> invalid encodings.
>>
>> There are also separate single-comparison functions:
>> floatx80_eq(), floatx80_le(), floatx80_lt(), floatx80_unordered(),
>> floatx80_eq_quiet(), floatx80_le_quiet(), floatx80_lt_quiet(),
>> floatx80_unordered_quiet(). The i386 guest doesn't use them but
>> I think for consistency we should treat invalid encodings like
>> NaNs there: raise float_flag_invalid and return 0.
>>
>> thanks
>> -- PMM