Fixing misuse of the standard math library (performance audit)

[pavel-kirienko]

Please refer there for context: PX4/PX4-Autopilot#6829 (comment).

The root of the problem is an improper use of the math library. The templated classes were implicitly making assumptions about the scalar type. For example, in many places the library used to make references to ::sinf or ::sqrt, essentially assuming that the scalar type is float or double, respectively.

Normally, this problem would be really easy to fix; consider the following two of the most obvious approaches:

• Replace all references to the C standard library with alternative references to the C++ standard library by adding std:: where necessary. This approach is pretty straightorward, but lacks in two ways:
  • A lot of code modifications.
  • No proof against re-occurrence of the same problem in the future. This one could be solved by means of advanced static analysis though, but it would be slightly outside of the scope right now.
• Shadow the C math functions with C++ alternatives using the using directive. This approach requires minimal changes to the codebase and is absolutely resilient to re-occurrence of the problem in the future: in order to break things again, a developer would have to explicitly specify the desired scope of the referred definition, e.g. ::sin, which people normally don't do.

N.B.: high integrity coding standards, such as MISRA C++ or HIC++, consistently prohibit the use of the C standard library. This specific case is a good anecdote in support of that recommendation.

The second solution from the above have been implemented here and tested against the unit tests (they all pass). Sadly, my attempt to test it with NuttX was unsuccessful due to the sad state of its C++ standard library. It turned out that instead of providing correct overloads in adherence to the C++ specification, it simply imports the C definitions into the std namespace. Consider fabs:

  using ::fabs;

That's it. This is what should have been there according to the standard:

float       fabs(float);
double      fabs(double);
long double fabs(long double);

The problem I described in the PX4 performance audit thread cannot be fixed right now without the loss of generality in the Matrix library. There are two solutions:

• Partially fix the NuttX C++ standard library. This is not something people do overnight.
• Implement the missing math functions manually in the Matrix library. Here's an idea how we can do that:

inline float       fabs(float x)       { return ::fabsf(x); }
inline double      fabs(double x)      { return ::fabs(x); }
inline long double fabs(long double x) { return ::fabsl(x); }

And so on. The list of missing functions is not large, I believe we only need about 10 or so.

[coveralls]

Coverage remained the same at 100.0% when pulling 14e9465 on performance_audit into cf92495 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 8ef2527 on performance_audit into cf92495 on master.

[pavel-kirienko]

I have applied the second workaround described above, now it works well with NuttX.

[pavel-kirienko]

Is there a way to detect NuttX in general instead of the PX4 flavor of it?

[jgoppert]

@davids5

[davids5]

@pavel-kirienko

For the record no there is none. So far, a change to add NUTTX to the generated config.h will not be accepted upstream. So this has to be set in the build by the flags, and we do that with __PX4_NUTTX.

[coveralls]

Coverage remained the same at 100.0% when pulling 41fe2cd on performance_audit into cf92495 on master.

[dagar]

Thanks for the thorough explanation Pavel. What about situations where the return is assigned to a float, but the input is a double. Wouldn't it be better to explicitly call the float version?

[pavel-kirienko]

Not always. Use of double suggests that a higher precision is needed than what float can provide, so converting to float before the computation may have adverse effects on numerical precision.

[jgoppert]

Looks good to me.

[jgoppert]

@davids5

[jgoppert]

Just waiting on feedback from @davids5 , then I think we are good to merge.

[davids5]

@jgoppert LTGM

[dagar]

@pavel-kirienko do you know any tools that would help us work towards MISRA C++ or HIC++? I haven't looked recently, but the MISRA options I've seen so far have been quite expensive.

[pavel-kirienko]

@dagar there is SciTools Understand, it's free for open source, but it tends to produce heaps of false positives. A bit unfriendly but usable.