Possible shift by negative amount in modules arm_lms_norm_q31 and am_lms_norm_q15

[elagil]

This raises the original issue from https://github.com/ARM-software/CMSIS_5/issues/1567, which is now out of scope there.

In brief: "Compiling current CMSIS for Host platfom (AMD64) while gcc's static code analysis feature is active, results in serious looking errors."

[elagil]

More specifically, the compiler warning is about https://github.com/ARM-software/CMSIS-DSP/blob/6ae4fa55482ea91b837ca9113070671bbc4b406e/Source/FilteringFunctions/arm_lms_norm_q31.c#LL189, where a negative shift value cannot be ruled out.

[christophe0606]

@elagil I don't think it can happen. CLZ return 32 only for a zero value. arm_recip_q31 is used with energy + DELTA_Q31 and the DELTA_Q31 is here so that we never divide by zero.

The only risk may be that energy overflow. But in that case, the algorithm will not work. The energy would overflow only if the signal is diverging from the reference.

[elagil]

If energy is guaranteed not to reach negative values, that is true. The warning describes the case where energy + DELTA_Q31 == 1.

Would you consider adding an assert(energy >= 0)?

[christophe0606]

From the user point of view, if energy saturates and becomes negative, it means the filter has already strongly diverged and so the results are meaningless. In this context, if the algorithm is no more right it is not really a problem.

But if we want to give enough information to the compiler to remove some warnings, then perhaps the simplest is to saturate the energy.

Before the >>32 and cast to q31_t, we could check if the int64 value is negative and in this case, return 0x7FFFFFFF for the int32 result.

For this operation that may saturate: ((((q63_t) in * in) << 1) + (energy << 32))

[elagil]

I am not sure, which line of the code you are referencing.

In any case, I agree that mitigating compiler warnings by catching this undefined case is a good idea.

[christophe0606]

@elagil I have made more tests with a relatively long filter (150 taps). In q15, there is an energy overflow and adding saturation is really making a difference. The algorithm is working with the saturation but is not without in the example I tested.

In q31, when energy is saturating, the signal part of the algorithm is also saturating. q31 is very sensitive to saturation and as explained in the documentation, the signal must often be scaled down. So I have not observed any difference with or without saturation. I have added the saturation of energy to help the compiler only.

[elagil]

Thanks for your efforts! That sounds good to me.