Re: minor bug in control_loop.h

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEOn0gFAd3OQG8ow6EtFwrk3lBwykFAmZiIt8ACgkQtFwrk3lB
wynZThAAhKkc06wcoFUhJW9HkiurLbdzhMad94WaRtmB1xzO/Bnp0SXsv71nvb/+
od+4LB3pIg5wRyYd18l3KshDaz7NJQbIHPLo2Z/m2E3leLVoV4kn873IIvMlgLt+
lqOl1chunYkcoOOmbmmBl/qnqQXxBdG42IrpvQIdJWJWx45HkTSOOvYj1X/QAmpX
bpAuVohaMv7yFDvBmrImJHkByH8O89e65GghOIS28ICnN/RRh1lRQoVXzbes7nbZ
erQgu81jblez/PCHyR/afBpY++PfV+/dNZx3luf7nEOQblIRr0zqxVEeY2Gr/YMi
wo84VrpvlvMoM+Qjx+SBtqUOUMuUFbtrUZP6+4fh/+Fwt6EOjEL6ULMG60PUFqqC
N3AT2hGEAbIqjwsOxtH/hLaPP0C8yOqHoX4dKyYY8FEEK4RUd8j0ANsbrIcMCaqO
VYc19E8g3ognvg0U5Mk/MaiRV10SvtJY+kGWzc5+dNnbWC4iMPrLO72SJ3MmIgR2
GtKBR+CP9em1shqLWMYRbERXiVd4w7vmbOFelEuOI4eeeE4vxvQDrG5S87trbRLm
raPGzkyR0GOiiYLPAeDBe8q+G3gP+icO48kJhMiSB0BqY7/kM2iPXlr2oSxe4vwi
ft6vkC7LbNK2oY57sGYvTTTJxReeSpDpiHJJTmT3HJdG2uI6u9o=
=D5nQ
-----END PGP SIGNATURE-----
Hi Eugene,

I believe there is this bug in other places (including other places in
control_loop) too. Basically any time that an input float is used to
compute an error which then ends up indexing some array (as could happen
in a closed loop that indexes into filterbank), if the input is NaN,
then NaN will propagate through the float calculations and give an
absurd value when cast to int to obtain the index. Some day someone will
go and fix all these (though the best fix is not obvious, since
performance is also a concern).

Are you sure that in the code snippet you've shared replacing int by
uint8_t fixes the problem? I haven't looked at the standard, but I
imagine that casting a NaN float to uint8_t is UB. Maybe this doesn't
cause a segfault because even though there is UB, in this particular
build the index ends up having some value between 0 and 255, which is
fine to index into tanh_lut_table without causing segfault (but the
result would be wrong/unpredictable!), whereas with an int index the int
can have any large (positive or negative) value that causes a segfault
when indexing.

Best,
Daniel.

On 06/06/2024 20:33, Eugene Grayver wrote:
> Hi,
>
> Found a minor (but annoying to track down!) bug in
> include/gnuradio/blocks/control_loop.h.
>
> If the input is NaN, then neither of the if statements evaluates to
> true, and the program segfaults.
>
> My company policy requires jumping through some hoops to make a direct
> github contribution.
>
> I suggest a patch:
>
> instead of 'int index', make it 'uint8_t index' and that will solve the
> segfault.
>
> static inline float tanhf_lut(float x)
> {
>     if (x > 2)
>         return 1;
>     else if (x <= -2)
>         return -1;
>     else {
>         int index = 128 + 64 * x;
>         return tanh_lut_table[index];
>     }
> }
>
> Eugene.