MISRA2004-12_7-3 - Underlying type of LHS operand

[jbrookley]

I'm currently using Parasoft's C++ Test program and it's flagging the last line of the code below and I'm not seeing the error . . . "The error I'm getting is Underlying type of LHS operand of bitwise operator is signed type." Below is the code:

Code: Select all

typedef signed short sshort16;

struct DACStruct
{
     sshort16 Off;
};

struct DACStruct DACData;

DACData.Off = (sshort16)(EEPROM_BANK1[1]) + (((sshort16)(EEPROM_BANK1[0])) << 8u);  /* Error on this line */

Just to be clear, each EEPROM bank byte is 8 bits so I'm basically trying to place the MSB (EEPROM_BANK1[0]) in the upper byte and the LSB (EEPROM_BANK1[1]) in the lower byte and then store it into DacData.Off, which is 2 bytes long.

Can anyone see what I'm clearly looking past or can help me resolve this issue? Any help you can give me would be greatly appreciated. Thanks, everyone!

[Steve Montgomery]

The LHS operand of the << operator is ((sshort16)(EEPROM_BANK1[0])). The underlying type of this operand, i.e. the type that it would have in the absence of integral promotion, is sshort16. So, I'd say that the tool is correct in reporting a violation of Rule 12.7 in this case because << is counted as a bitwise operator for purposes of the rule.

If you don't need the offset to be a signed quantity then changing the typedef so that sshort16 is an unsigned short should sort things out. However, if you need a signed offset then you could still use an unsigned type to combine the MSB and LSB and then convert the result to a signed type. You will need to use a temporary variable though in order to avoid violating Rule 10.3. For example, the following might work:

Code: Select all

typedef signed short sshort16;
typedef unsigned short ushort16;

struct DACStruct
{
     sshort16 Off;
};

struct DACStruct DACData;

ushort16 tmp = (ushort16)(EEPROM_BANK1[1]) + (((ushort16)(EEPROM_BANK1[0])) << 8u);
DACData.Off = (sshort16)tmp;

[jbrookley]

Thanks for the suggestion! At least now I know what it's referring to. I actually tried this two different ways and both seemed to violate 10.5 (The result of [a<<b]operation should be cast to the uchar8 type).

I tried your suggestion:

Code: Select all

ushort16 temp = 0;
ushort16 tmp = (ushort16)(EEPROM_BANK1[1]) + (((ushort16)(EEPROM_BANK1[0])) << 8u); /* Error seen on this line */
DACData.Off = (sshort16)tmp;

Then I tried this:

Code: Select all

ushort16 temp = 0;
temp = (ushort16)(EEPROM_BANK1[1]) + (ushort16)(((EEPROM_BANK1[0])) << 8u); /* Error seen on this line */
DACData.Off = (sshort16)tmp;

I was finally able to resolve this using the following:

Code: Select all

ushort16 temp = 0;
temp = (ushort16)(EEPROM_BANK1[1]) + (ushort16)(((ushort16)(EEPROM_BANK1[0])) << 8u); /* Error no longer seen on this line */
DACData.Off = (sshort16)tmp;

Is this the best way I can approach this or is there likely a better or more elegeant way to code it? It seems a little silly to need two conversions like that. Thanks!

[Steve Montgomery]

I'd made assumption, for no good reason, that your int size was 16 bits in which case Rule 10.5 wouldn't have been a problem because the LHS operand (ushort16)(EEPROM_BANK1[0]) would not be promoted.

I think you can avoid the second cast by ensuring that you do the operation using the unsigned int type instead of in ushort16, e.g.

Code: Select all

unsigned int tmp = 0;
tmp = (unsigned int)EEPROM_BANK1[1]) + ((unsigned int)EEPROM_BANK1[0] << 8u);
DACData.Off = (sshort16)tmp;

With any luck, your compiler will be smart enough to see that bits 16-31 of tmp must always be zero so will generate 16-bit shift and add instructions instead of 32-bit ones, assuming that your target (a) has 16-bit operations and (b) that they are any shorter/quicker than 32-bit ones. If it already generates 32-bit instructions then your aren't losing anything anyway.

You may find a complaint about use of the unsigned int type (advisory Rule 6.3). It suggests that you use a type that specifies the width explicitly, e.g. uint32_t. Personally I think this is a situation in which using unsigned int is justified because it's a portable way to express the smallest unsigned type that it not subject to integer promotion. I would therefore justify not applying Rule 6.3 in this situation. If your project / organisation prefers to comply 100% with every advisory rule then you'll have to use a type that specifies the width of the integer.

Note: I don't have access to a MISRA C:2004 checker so I can't be sure that the above will work but hopefully it, or something like it, will.

[jbrookley]

Steve,

I'm looking at the code and I don't see how this code could work as you're trying to left shift a byte by 8 bits and store it into a variable that is only 8 bits. If both EEPROM banks are 0xFF, the final result comes to be 0x00FF, which makes sense as the top byte is basically shifting itself nowhere. Was the code meant to show the following?

Code: Select all

unsigned short tmp = 0;  /* changed here from int to short */
tmp = (unsigned int)EEPROM_BANK1[1]) + ((unsigned int)EEPROM_BANK1[0] << 8u);
DACData.Off = (sshort16)tmp;

This way, both EEPROM banks are being converted to unsigned ints, as you suggested, while still providing enough space to store both of them.

[Steve Montgomery]

I was assuming that unsigned int is at least 16 bits wide. The C standard doesn't permit it to be any narrower than this (see C99 Sections 6.2.5 and 5.2.4.2.1).

So, the left shift and the addition are both performed in a type that is at least 16 bits wide. It it guaranteed that the results of both operations will fit in those 16 (or more) bits. It's also guaranteed that all bits numbered 16 and higher will be 0 so your compiler can select 16-bit instructions if it's able to and if doing so would be beneficial. That means you shouldn't lose out on efficiency and will gain portability.

[misra-c]

This matter appears to be resolved, so we have nothing more to add.