GCC v11 32F4: compiler warning depends on optimisation level

[Nominal Animal]

It is interesting to note that given int32_t x = -1, (int32_t)((((uint32_t)x) >> 1) | (1 << 31)) == -1.  For GCC, that expression is equivalent to (x >> 1) == -1 (which is true).

That is, GCC only supports two's complement integer types where all possible patterns are ordinary values, and shifting signed values right copies the most significant AKA sign bit, and the entire bit pattern is shifted.  That is, for int32_t x, shifting right by one bit is
    x >> 1 == (int32_t)(((uint32_t)x) >> 1) | (0x80000000 & (uint32_t)x))
and larger shifts are this repeated.  In particular,
    x >> 31 == (x < 0) ? -1 : 0

This also means that (x >> 1) != (x/2) for negative values.

It is also notable that this is implementation defined behaviour (see C99-C23 6.5.7p5), not undefined behaviour, so a build-time test suffices to verify the behaviour for portable code.


In the case where you want to ensure zeros are shifted in, use a cast,
  ((uint32_t)x) >> n
which is guaranteed by the C standard to shift in zeroes, because ((uint32_t)x) >> n == ((uint32_t)x) / (1 << n) per standard definition.

Then, in your source/build documentation, add a section Expected implementation-defined behaviour, and there describe this behaviour; for example,

• Compiler implementation is such that given int32_t x = -1, (x >> 1) == -1.

Not only is this useful for porting, it is useful to check this list first when you encounter an unexpected bug (that is, a bug that is not in the code you added or modified last).

[peter-h]

In the case where you want to ensure zeros are shifted in, use a cast:
  ((uint32_t)x) >> n

Will

int32_t x;
x =  ((uint32_t)x) >> n;

be equivalent? It is surely a stupid question but some of the above got me worried. IOW does a cast back to int change any bits (for any value of x)? I don't think I have ever written code which casts a uint to an int, where the uint may be big enough to have the top bit set. I would regard that as really useless and dodgy.

I've just documented all this as you suggest.

In the meantime I've had more fun with CPPcheck:

while ( (response[idy]!='=') && (idy<50) ) {  ... }
style: Defensive programming: The variable 'idy' is used as an array index before it is checked that is within limits. This can mean that the array might be accessed out of bounds.
It works but would blow up if the array happened to be at the very end of physical RAM

But I believe this warning is wrong:

How can they be sure len is not 0. Inbuf could contain any garbage.

And I wonder if anyone has an idea re printsize() further back...

[Nominal Animal]

Given
    int32_t  x;
then
    x = ((uint32_t)x) >> 5;
is exactly equivalent to
    x >>= 5;
    x &= 0x07FFFFFF;
based on standard C, no implementation-defined behaviour or differences between compilers, as long as they follow standard C (currently C99, C11, C17, or C2x/C23).

(Remember, exact-width integer types uintN_t and intN_t always use two's complement representation for negative values with all possible bit patterns corresponding to ordinary values, no padding, no trap representations.)

does a cast back to int change any bits (for any value of x)?

In the right shift case (by a positive, nonzero number of bits) that is not relevant, because the result is always positive, because the sign bit is the most significant bit and a zero will be shifted in when using the unsigned type cast.  That is, if the exact-size (uint_N) cast value is shifted right by at least one bit, then the result is guaranteed to fit and be within the range of the corresponding intN_t type, and the standard says that in that case that will be the resulting value.

The standard leaves a cast from unsigned to signed integer types implementation defined only when the value cannot be represented in the signed type.  GCC will never change the bit pattern (see here), and will simply re-interpret the sign bit as-is.  This is another implementation-defined behaviour one can expect, and note in the documentation for portable code.  For example, (int32_t)(0x80000008u) == -2147483640.

I did some digging for Clang, but I couldn't find a similar implementation behaviour list, but I believe it behaves the exact same way.  It is actually quite possible that future C standards end up codifying this, because I personally have never used a C99-or-later compliant compiler that behaved differently.

[golden_labels]

That implementation-defined behavior for right shifts may be tracked down to early versions of C before standardization. It’s even mentioned in “The C programming language” (2.9): “Right shifting a signed quantity will fill with sign bits ("arithmetic shift") on some machines such as the PDP-11, and with 0-bits ("logical shift") on others.” The associated getbits example evades the issue entirely by just assuming the value is positive. A similar note may be found in the Ritchie’s memo, where it’s indicated that “the use of arithmetic rather than logical shift does not survive transportation between machines.”

Both versions still assumed a more or less specific integer representation. In particular the 1974 version assumes 16-bit, 2’s complement integers.

[Nominal Animal]

That implementation-defined behavior for right shifts may be tracked down to early versions of C before standardization. It’s even mentioned in “The C programming language” (2.9): “Right shifting a signed quantity will fill with sign bits ("arithmetic shift") on some machines such as the PDP-11, and with 0-bits ("logical shift") on others.” The associated getbits example evades the issue entirely by just assuming the value is positive. A similar note may be found in the Ritchie’s memo, where it’s indicated that “the use of arithmetic rather than logical shift does not survive transportation between machines.”

Both versions still assumed a more or less specific integer representation. In particular the 1974 version assumes 16-bit, 2’s complement integers.

(To nitpick, only right shifts of negative values is implementation-defined.  Positive values in signed integer types follow logical right-shift rules, being equivalent to division by a power of two.  I know golden_labels knows this well; this is just a reminder for others reading this thread that might not be aware of this.)

For exact-size unsigned integer types (uintN_t) right-shift is always the logical one, equivalent to division by a power of two.  So, if casting to/from the corresponding signed type (intN_t) simply reinterprets the N-bit pattern –– the types being required by the standard to use two's complement format, with no padding bits or trap representations, all possible storage patterns and values corresponding to ordinary integer values ––, and the signed type right-shift is always the arithmetic one (filling new bits with the sign bit), programmers have full control and portable use of both.  This is how GCC behaves on all architectures it supports.  I believe, but have not been able to verify from the documentation, that Clang behaves the same way on all architectures.  To select between logical/arithmetic right shift on expressions using these types, one needs at most two casts, which are almost always no-ops affecting only code generation, generating no machine code themselves.

This is why I believe it likely that a future standard C revision will standardize the current GCC behaviour.  I don't think there is any current architecture that supports C that would suffer from this, and the benefit to low-level and systems programmers is huge.

[SiliconWizard]

But I believe this warning is wrong:

How can they be sure len is not 0. Inbuf could contain any garbage.

How much is INBUF_SEARCH_LEN?

[peter-h]

It is 7.

So if inbuf[4,5] contain 0xffff, len=0.

Admittedly if len=0xffff then the other condition will also fail But that is not the same thing as saying len==0 is always false.

But this isn't worth worrying about because the ==0 check is just good programming, even if some weird analysis decides it cannot happen.

[SiliconWizard]

It is 7.

So if inbuf[4,5] contain 0xffff, len=0.

Admittedly if len=0xffff then the other condition will also fail But that is not the same thing as saying len==0 is always false.

But this isn't worth worrying about because the ==0 check is just good programming, even if some weird analysis decides it cannot happen.

The diagnostic is indeed wrong but it still points (possibly unwillingly) to a potential issue.

I guess (at least for this particular type of diagnostic) CppCheck doesn't consider the modulo (wrap-around) in C arithmetics, or doesn't properly consider the integer width here. In which case, it wouldn't see the possibility of a zero (which would mean the sum has wrapped around).

The potential issue I mentiioned is that checking the values in inbuf (so the value of 'len' *before* adding (8 - INBUF_SEARCH_LEN) ) would be a good idea (input validation) and would avoid a possible wraparound.
So I would check len before the line 'len = len + ...'. You can also check it after the sum if that makes sense for your application. But I would not potentially let the sum wrap around.

[coppice]

That implementation-defined behavior for right shifts may be tracked down to early versions of C before standardization. It’s even mentioned in “The C programming language” (2.9): “Right shifting a signed quantity will fill with sign bits ("arithmetic shift") on some machines such as the PDP-11, and with 0-bits ("logical shift") on others.” The associated getbits example evades the issue entirely by just assuming the value is positive. A similar note may be found in the Ritchie’s memo, where it’s indicated that “the use of arithmetic rather than logical shift does not survive transportation between machines.”

Both versions still assumed a more or less specific integer representation. In particular the 1974 version assumes 16-bit, 2’s complement integers.

Early C compilers were all over the place with shift behaviour. I think they mostly implemented whatever was quick to do with the instruction set available. This was a bad idea even than, as from day 1 C had signed and unsigned integers, and treating them in a natural way for the type was perfectly reasonable to do, even when the instruction set didn't directly support both arithmetic and logical shifts. Early C was not well thought through in many ways. Like not having integers which nailed their number of bits in a clear unambiguous way. Retrofitting that decades later is still gradually working its way through the system. In the last 30 years or so the only C compilation I have seen which doesn't obey the natural behaviour of signed and unsigned values when shifting have been DSPs. which often have a pretty quirkly form of C for many reasons.

[peter-h]

What is needed is a dedicated CPPcheck thread

It does pick up some curious stuff e.g. I had a prototype for a function, 3 int parms, and in the prototype I had their names in the wrong order. I wondered for a moment... how can the code possibly work? The answer is that their names don't matter (obviously). I verified this with a binary comparison.

It also disregards #defines, so e.g. the above len==0 code was not even included in the project.

And it reports loads of functions as never called, because it doesn't seem to relate multiple source files. Probably there is a config for that...

Many "scope of ... can be reduced" comments. Many"Local variable 'ch' shadows outer variable [shadowVariable]" but I see no issue with using "i" as a for loop counter; it is private to the loop.

So far I have not found anything serious in this multi-year project. But I have not yet let it loose on MbedTLS

[newbrain]

How can they be sure len is not 0. Inbuf could contain any garbage.

Just a question: how is inbuf defined?
If a char array has been used, does cppcheck know that, by default, char is unsigned on gcc for arm?

If cppcheck thinks char to be signed, you'll never be able to obtain 0xFFFFu from the line:

Code: [Select]

    len = inbuf[4] + 256 * inbuf[5];for semi-obvious reasons.

If inbuf is a uint8_t (or equivalently, explicit unsigned char) array, then the warning is indeed spurious, as you suggest.

[peter-h]

static uint8_t inbuf[PKT_BUF_LEN];
and PKT_BUF_LEN is 2048 bytes.