S836 False Positive with right shift on TI Code Composer

Rule in question: c:S836

SonarQube warning: The result of the right shift is undefined due to shifting by '8', which is greater or equal to the width of type 'uint16_t'

SonarQube Server: 9.2.4
SonarScanner 4.6.2.2472
TI Code Composer 9.0.1

Relevant code snippets:

#include <stdint.h>

typedef enum
{
   GRPID_1 = 1,
   GRPID_2,
   GRPID_3,
   GRPID_4,
   GRPID_5,
   GRPID_6,
   GRPID_7,
   GRPID_8,
   GRPID_9,
   GRPID_10,
   GRPID_11,
   GRPID_12,
   GRPID_13,
   GRPID_14
} SCI_GROUP_ID_t;

typedef struct
{
   const SCI_GROUP_ID_t groupID;
   const uint16_t groupSize;
   bool isOnDemand;
   const uint16_t minUpdateTime;
   uint32_t sequenceNum;
   uint32_t *pTimer;
   uint8_t *pPayload;
} Group_info_t;

typedef struct
{
    uint8_t bytes[40];
} sci_group_header_t;

sci_group_header_t grp_header_buf;
static Group_info_t  Group_info[20]

Initial iteration of code that raised the error:

grp_header_buf.bytes[4] = (uint8_t) ((Group_info[index].groupID >> 8) & 0x00FF);

Updated code to try to resolve the error by casting explicitly:

grp_header_buf.bytes[4] = (uint8_t) (((uint16_t) Group_info[index].groupID >> 8) & 0x00FF);

Note: uint32/16/8 types are TI-defined in their <stdint.h> header file. On the TI TMS320C architecture, the smallest memory-based size you can get is a 16-bit value.

The field in question has been cast to a uint16_t prior to being shifted, which per operator precedence table, the cast should happen prior to the shift.

However, even adding the explicit cast in the second code sample to uint16_t was not enough to get rid of the warning. Also important here is that the Group_info.groupID field is technically an enumeration, and thus what I saw as the need to explicitly cast to a strict type for this shifting.

I am asking the developer whose code this is to make the following modification (adding parentheses around the cast and the variable being cast) to see if it resolves the warning, but if it does, that seems like a parser issue if the cast is not being handled before the shifting. Based on that experiment I will add to this thread with the results.

grp_header_buf.bytes[4] = (uint8_t) ((((uint16_t) Group_info[index].groupID) >> 8) & 0x00FF);

Hello @mikeschlag ,

Could you tell us how those types are defined there? We don’t support very well architectures where CHAR_BIT is not 8 (which might be the case, according to what you said), so for instance if CHAR_BIT is 16, and uint16_t is a typedef to unsigned char, we will consider it is 8 bits, while it really is 16 bits, and that might cause issues such as the one you are reporting.

On this TI architecture (TMS320C2000 series), 8-bit values are in-memory stored in 16-bit spaces, and accessed as such, regardless of their typing. In TI’s stdint.h file, they define as follows:

    typedef   signed char    int8_t;
    typedef unsigned char   uint8_t;
    typedef          int    int16_t;
    typedef unsigned int   uint16_t;

There are a couple specific architecture definitions that define 16-bit values as such, but I’m 90% positive those don’t apply to our architecture in this case, but either way a short should still be a 16-bit definition generally:

    typedef          short  int16_t;
    typedef unsigned short uint16_t;

Thank you for that information.

Can you also tell us what are the values of sizeof(int) and sizeof(short) on this platform?

I will ask a dev to run that and let me know, and I will update this here when I have that info.

Additionally, we have now tried the last case I mentioned above, and it’s still showing the same warning.

Of note, this line of code does not generate the warning, as a native-typed uint16_t:

grp_header_buf.bytes[6] = (uint8_t) ((Group_info[index].groupSize >> 8) & 0x00FF);

So, it seems like the issue is that the type for groupID is an enumeration, but the casting isn’t being considered.

Hi Mike,

Could you then also ask him what is sizeof(SCI_GROUP_ID_t)?
Thanks,

For this architecture, all three sizeof checks came back with a value of 1.

I feel like I should point out, the size of 1 here is expected because of the 16-bit minimum size of this processor. sizeof(char) comes out as 1 as well.

Hello @mikeschlag,

So, this is what I expected… We currently consider that a char is 8-bits (and it’s not something that can be easily changed). Since uint16_t is a typedef to unsigned int, and since sizeof(int) (and therefore sizeof(unsigned int) is 1, we also believe that uint16_t is 8 bits.

We already have a ticket to describe the issue, CPP-3622, but we currently don’t have any plans to tackle it.

For now, you should either mark this as false positive or deactivate the rule.

1 Like

Okay, thanks for the information, we’ll proceed as such!

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.