Fix arithmetics overflow warnings with VS2019 #1877

obones · 2021-11-16T13:23:35Z

This fixes warnings of this kind, seen via #1866:

Warning C26451 Arithmetic overflow: Using operator '' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '' to avoid overflow (io.2).

merbanan · 2021-11-16T13:28:21Z

src/baseband.c

@@ -116,7 +116,7 @@ float magnitude_true_cs16(int16_t const *iq_buf, uint16_t *y_buf, uint32_t len)
    for (i = 0; i < len; i++) {
        int32_t x = iq_buf[2 * i];
        int32_t y = iq_buf[2 * i + 1];
-        y_buf[i]  = (int)sqrt(x * x + y * y) >> 1; // max 46341, scaled 23170, fs 16384
+        y_buf[i]  = (int)sqrt((double)x * x + (double)y * y) >> 1; // max 46341, scaled 23170, fs 16384


The code is fixed point by design.

Yes, but sqrt takes a double which is why there is the warning.

I think that sqrtf() should work and is slightly better type wise. If it is better accuracy wise I don't know.

I guess the point is that x and y are limited to [-128,127] because of iq_buf being bytes, but the compiler is not figuring it out? Is there in fact an overflow possibility, as determined by a human reviewing? Or is it a false positive warning. The position I have seen elsewhere, and tend to agree with, is that code should be fixed for valid warnings, and not modified much if any for invalid warnings, except that restructuring it slightly to be easier to reason about, withtout hurting the generated code is ok.

merbanan · 2021-11-16T13:29:04Z

src/baseband.c

@@ -85,7 +85,7 @@ float magnitude_true_cu8(uint8_t const *iq_buf, uint16_t *y_buf, uint32_t len)
    for (i = 0; i < len; i++) {
        int16_t x = iq_buf[2 * i] - 128;
        int16_t y = iq_buf[2 * i + 1] - 128;
-        y_buf[i]  = (uint16_t)(sqrt(x * x + y * y) * 128.0); // max 181, scaled 23170, fs 16384
+        y_buf[i]  = (uint16_t)(sqrt((double)x * x + (double)y * y) * 128.0); // max 181, scaled 23170, fs 16384


This should be fixed point but I guess there is a reason for the 128.0.

I believe the reason is more about sqrt taking a double instead of a float

This is just a test function, still turning int muls into double muls isn't great.

And same here with sqrtf().

zuckschwerdt · 2021-11-16T13:54:48Z

src/pulse_analyzer.c

-        device.gap_limit   = to_us * (hist_gaps.bins[1].max + 1);                        // Set limit above next lower gap
-        device.reset_limit = to_us * (hist_gaps.bins[hist_gaps.bins_count - 1].max + 1); // Set limit above biggest gap
+        device.gap_limit   = to_us * (hist_gaps.bins[1].max + 1.0);                        // Set limit above next lower gap
+        device.reset_limit = to_us * (hist_gaps.bins[hist_gaps.bins_count - 1].max + 1.0); // Set limit above biggest gap


Not great, it was int addition before, now it's double addition.

Well, yes, the warning is about doing a 4 byte addition that then gets promoted to an 8 byte type for the multiplication, potentially leading to an overflow.
Using the floating point constant forces double computation which is not great, I agree. But how do I get rid of the warning then?

This is non performance critical code. I'm fine with this change here but maybe it should be a cast? gap_limit is (float) while to_us is (double).

obones · 2023-01-09T13:19:59Z

I have just rebased this branch on the latest master, please let me know if this is still of interest

zuckschwerdt · 2023-01-09T13:36:03Z

Yes, these clean-ups are of interest. I really need to find quiet time to contemplate the effects (e.g. changing some double calcs to float only).

…utation

…at and not on int16_t that could overflow

gdt · 2023-10-03T20:05:20Z

src/baseband.c

@@ -126,7 +126,7 @@ float magnitude_true_cs16(int16_t const *iq_buf, uint16_t *y_buf, uint32_t len)

 // Fixed-point arithmetic on Q0.15
 #define F_SCALE 15
-#define S_CONST (1 << F_SCALE)
+#define S_CONST ((int)(1 << F_SCALE))


This surprises me. What type is 1<< F_SCALE without a cast. unsigned int?

merbanan reviewed Nov 16, 2021

View reviewed changes

zuckschwerdt reviewed Nov 16, 2021

View reviewed changes

zuckschwerdt force-pushed the master branch 2 times, most recently from 8e31ece to e42ad33 Compare December 26, 2021 11:10

zuckschwerdt force-pushed the master branch from d8293a7 to 63514de Compare March 3, 2022 13:20

zuckschwerdt force-pushed the master branch from 9fafe69 to 15637a2 Compare September 7, 2022 13:14

obones force-pushed the warnings_arithmetics_overflow branch from 4e3d436 to 3eb7966 Compare January 9, 2023 09:38

obones force-pushed the warnings_arithmetics_overflow branch from 3eb7966 to 16d39f2 Compare January 11, 2023 16:47

zuckschwerdt force-pushed the master branch from 5b2a938 to 3603446 Compare January 22, 2023 14:25

zuckschwerdt force-pushed the master branch from 976371b to fe3df68 Compare March 30, 2023 09:19

zuckschwerdt force-pushed the master branch 2 times, most recently from 93031a1 to ed794ec Compare August 14, 2023 11:47

obones added 3 commits October 3, 2023 10:27

minor: use an explicit cast to make sure no "float" intermediate comp…

a58c73a

…utation

minor: use double constants to avoid intermediate "float"

6d71f79

minor: use parenthesis to make sure the multiplication is done on flo…

1285116

…at and not on int16_t that could overflow

obones force-pushed the warnings_arithmetics_overflow branch from 16d39f2 to 1285116 Compare October 3, 2023 08:39

gdt reviewed Oct 3, 2023

View reviewed changes

zuckschwerdt force-pushed the master branch from 7b88ef5 to cf4c74d Compare December 24, 2023 15:31

zuckschwerdt force-pushed the master branch from 1613179 to 25a0a97 Compare March 12, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix arithmetics overflow warnings with VS2019 #1877

Fix arithmetics overflow warnings with VS2019 #1877

obones commented Nov 16, 2021

merbanan Nov 16, 2021

obones Nov 16, 2021

merbanan Nov 16, 2021

gdt Oct 3, 2023

merbanan Nov 16, 2021

obones Nov 16, 2021

zuckschwerdt Nov 16, 2021

merbanan Nov 16, 2021

zuckschwerdt Nov 16, 2021

obones Nov 16, 2021

merbanan Nov 16, 2021

obones commented Jan 9, 2023

zuckschwerdt commented Jan 9, 2023

gdt Oct 3, 2023

Fix arithmetics overflow warnings with VS2019 #1877

Are you sure you want to change the base?

Fix arithmetics overflow warnings with VS2019 #1877

Conversation

obones commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obones commented Jan 9, 2023

zuckschwerdt commented Jan 9, 2023

Choose a reason for hiding this comment