Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (3af006e) to head (d8b4bf4).

❗ Current head d8b4bf4 differs from pull request most recent head 3613ad9

Please upload reports for the commit 3613ad9 to get more accurate results.

@@           Coverage Diff           @@
##           master   #14456   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20864    20864           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Starting from C99 FLT_EVAL_METHOD should be left to the compiler/libc to define, according to the appropriate floating point types' representation for the platform. Choosing floating point types representations should be done via compiler options rather than with #defines.

As I understand it, the maybe-tricky part here is that MicroPython libm provides a bunch of optimised math routines that rely on a particular binary representation of floats. So if FLT_EVAL_METHOD != 0 then probably float_t != float and possibly double_t != double and therefore MicroPython's libm may produce garbage results on that compiler.

To keep a check for this, I think it might be enough to insert this into both libm headers where the define previously was:

// These lines verify that FLT_EVAL_METHOD==0, MicroPython's libm requires this.
// If compilation fails here then check the host compiler's FLT_EVAL_METHOD.
typedef float float_t;
typedef double double_t;

Redundant (compatible) typedef declarations are allowed, so provided math.h makes matching definitions then this should compile. If a particular platform is configured differently, math.h should have defined float_t or double_t to something else so a compiler error should appear at this point. I think it's preferable to (maybe) compiling and producing garbage results

That said, I don't know if MicroPython supports any build configurations where this would fail.

@projectgus, I've looked a bit into it as well.

On RISC-V I believe FLT_EVAL_METHOD may change depending on the combination of architecture and abi being passed to GCC via -march and -mabi. If my understanding is correct (and please do not quote me :)), if GCC is asked to build for a CPU implementing a combination of "D", "Q", or "Zfh"/"Zfhmin" extensions (double, quad, half precision) but not the "F" extension (single precision) then MicroPython's libm would not work as either floats or doubles will be aliased to something else. Same goes for libm_dbl on a CPU that only implements "F", "Q", or "Zfh"/"Zfhmin" but not "D", doubles will be aliased to floats.

That alone makes your point valid, I guess.

I haven't looked much into how Arm handles this, but for example what happens if you use libm_dbl targetting a Cortex-M4F (whose FPU only supports single-precision floats) passing -mfloat-abi=hard? No idea whether the compiler will still use soft-float code when using doubles or will just alias doubles to floats.

That said, I'll update the patch following your suggestion, thanks!

@agatti These are good questions that I don't know the answers to! At minimum, I think these changes won't make anything not work that previously would have worked...