Bug 491667

Summary:	std::fmin(val, NaN) should returns NaN on arm.
Product:	[Developer tools] valgrind	Reporter:	meirav.grimberg
Component:	memcheck	Assignee:	Paul Floyd <pjfloyd>
Status:	CONFIRMED ---
Severity:	normal	CC:	mark, pjfloyd
Priority:	NOR
Version First Reported In:	unspecified
Target Milestone:	---
Platform:	Other
OS:	Other
Latest Commit:		Version Fixed/Implemented In:
Sentry Crash Report:

Description meirav.grimberg 2024-08-13 13:15:11 UTC

SUMMARY

fmin(NaN,-1) returns NaN on linux arm.
According to fmin documentation:
https://en.cppreference.com/w/cpp/numeric/math/fmin
Error handling
This function is not subject to any of the error conditions specified in math_errhandling.

If the implementation supports IEEE floating-point arithmetic (IEC 60559),

* If one of the two arguments is NaN, the value of the other argument is returned.
* Only if both arguments are NaN, NaN is returned.

STEPS TO REPRODUCE
program test.cpp

``
#include <cmath>
#include <iostream>

int main()
{
    std::cout << "fmin(NaN,-1) = " << std::fmin(NAN, -1) << '\n';
}

``
 g++ test.cpp


OBSERVED RESULT

valgrind ./a.out 
==9822== Memcheck, a memory error detector
==9822== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9822== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==9822== Command: ./a.out
==9822== 
fmin(NaN,-1) = nan

EXPECTED RESULT

./a.out 
fmin(NaN,-1) = -1

SOFTWARE/OS VERSIONS
Was tested on dockers.
arch: armV8
1. linux jammy. valgrind version 18.1
2. linux apline. valgrind version 23

ADDITIONAL INFORMATION

Comment 1 Mark Wielaard 2024-08-14 11:27:16 UTC

Confirmed on fedora 40 aarch64 with valgrind 3.23.0 and current git trunk VALGRIND_3_23_0-94-g033baf8f1

Dump of assembler code for function __fmin:
=> 0x0000fffff7f42880 <+0>:	paciasp
   0x0000fffff7f42884 <+4>:	stp	x29, x30, [sp, #-16]!
   0x0000fffff7f42888 <+8>:	fminnm	d0, d0, d1
   0x0000fffff7f4288c <+12>:	mov	x29, sp
   0x0000fffff7f42890 <+16>:	ldp	x29, x30, [sp], #16
   0x0000fffff7f42894 <+20>:	autiasp
   0x0000fffff7f42898 <+24>:	ret
End of assembler dump.

Comment 2 Mark Wielaard 2024-08-14 11:30:33 UTC

Note that VEX/priv/guest_arm64_toIR.c contains the following comment:

      /* ------- 0x,0111: FMINNM d_d, s_s ------- (FIXME KLUDGED) */
[...]
         case BITS4(0,1,1,1): nm = "fminnm"; iop = mkVecMINF(ty+2); break; //!!

Comment 3 Mark Wielaard 2024-08-14 11:33:41 UTC

And the following comment at the top:

/* KNOWN LIMITATIONS 2014-Nov-16

   * Correctness: FMAXNM, FMINNM are implemented the same as FMAX/FMIN.

     Also FP comparison "unordered" .. is implemented as normal FP
     comparison.

     Both should be fixed.  They behave incorrectly in the presence of
     NaNs.

     FMULX is treated the same as FMUL.  That's also not correct.

   * Floating multiply-add (etc) insns.  Are split into a multiply and 
     an add, and so suffer double rounding and hence sometimes the
     least significant mantissa bit is incorrect.  Fix: use the IR
     multiply-add IROps instead.           

   * FRINTX might be need updating to set the inexact computation FPSR flag
         
   * Ditto FCVTXN.  No idea what "round to odd" means.  This implementation
     just rounds to nearest. 
*/