Why did my tool throw a MISRA error here?

Question

What can I do to avoid MISRA giving this error for the code below? I tried casting with (unit16_t). But then it didn't allow an explicit conversion.

Illegal implicit conversion from underlying MISRA type "unsigned char" to "unsigned int" in complex expression (MISRA C 2004 rule 10.1)

 uint8_t rate = 3U; 
 uint8_t percentage = 130U;      
 uint16_t basic_units = rate * percentage;

Aspirin

Answer 1

The problem is that both rate and percentage are silently promoted by the integer promotions to type "int". The multiplication is therefore performed on a signed type.

MISRA compatible code is to either rewrite the code as

 uint16_t basic_units = (uint16_t)rate * (uint16_t)percentage;

or do as MISRA suggests, immediately typecast the result of an expression to its "underlying type":

 uint16_t basic_units = (uint8_t)(rate * percentage);

EDIT: Clarification follows.

ISO 9899:1999 6.3.1.1 2

If an int can represent all values of the original type, the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.

Informative text from MISRA-C:

MISRA-C:2004 6.10.3 Dangerous type conversions:

/--/

- Change of signedness in arithmetic operations: Integral promotion will often result in two unsigned operands yielding a result of type (signed) int. For example, the addition of two 16-bit unsigned operands will yield a signed 32-bit result if int is 32 bits but an unsigned 16-bit result if int is 16 bits.

I'm actually not sure whether the 2nd line of mine above would satisfy MISRA or not, at second thought I may have confused MISRA 10.1 with 10.5, where the latter enforces an immediate cast to underlying type, but only in case of certain bitwise operators.

I tested both lines with LDRA static code analysis and it didn't complain (but gives some incorrect, non-related warnings), but then LDRA also performs very poorly at MISRA-C.

Anyway, the problem in the original question is that rate and percentage are both implicitly converted by the integer promotions to type int which is signed, since int can represent all values of a uint8_t. So it becomes

uint16_t basic units = (int)rate * (int)percentage.

To prevent this you have to typecast explicitly. After giving it more thought, I'd go with the 1st line of my two above.

Interknit

Why would the promotion of two unsigned types uint8_t result in a signed type int rather than a larger unsigned type unsigned int or more simply uint16_t? – Shainashaine

@pmg I think I actually mixed up MISRA rules 10.1 and 10.5. Had it been ~ or << operators I would have been forced by MISRA to typecast into the "underlying type". I've edited my post to clarify things. – Interknit

@loan Because C is a very illogical language. Never assume that any implicit conversion done by C is a sane one! :) I've edited my post to explain further. – Interknit

@Interknit considering that 3x130 > 255, won't casting "rate * percentage" to its underlying type cause arithmetic overflow? – Hiedihiemal

@AbdelAleem This very old answer that predates MISRA C:2012 seems to assume that int is 16 bit on the target system, for some reason. The code in the question has an arithmetic overflow by design, but it doesn't have an overflow as defined by C, where overflow can only happen on signed types and is undefined behavior. Casting the result after the operation has been carried out won't affect the operation, it will just truncate the result down to a single byte in this case. – Interknit

Answer 2

The MISRA rule is trying to ensure that the "underlying type" used for calculation is the same as the resulting type. To achieve that, you can cast the one, or both, of the operands:

uint8_t rate = 3U; 
uint8_t percentage = 130U;      
uint16_t basic_units = (uint16_t)rate * percentage;

On a 32-bit architecture the result without the cast is OK, however, consider the following:

uint32_t rate =  ...;
uint32_t percentage = ...;
uint64_t basic_units = rate * percentage;

On a 32-bit architecture, the operation will be performed in 32 bits - even though the target type is 64 bits wide. Where rate and percentage are large enough, this could result in the operation wrapping in 32 bits and so data that would have fit in the target type will be lost.

The MISRA rule is attempting to make the code safer irrespective of the size of the types on the target platform.

Homozygous

Answer 3

The implicit conversion is performed before the multiplication, for the multiplication. Maybe an explicit conversion right before the multiplication shuts up your tool

uint16_t basic_units = (unsigned)rate * (unsigned)percentage;

The resulting unsigned value, should be implicitly converted to uint16_t with no warnings. If your tool chooses to be a PITA about this too, try another explicit conversion:

uint16_t basic_units = (uint16_t)((unsigned)rate * (unsigned)percentage);

Majorette

Oops I almost posted something identical. Though note that MISRA frowns upon using unsigned (int) in advisory rules, so you should typecast to uint16_t for maximum tool-shut-upping. – Interknit

Hmm, I don't have the MISRA rules present :) I just thought that when uint16_t is of smaller rank than int, the issue won't go away: the values will be converted to the larger type unsigned then too. – Majorette

Indeed, if uint16_t happens to be unsigned short, it will still be promoted. Perhaps the best (and most MISRA-compliant) code is therefore to typecast the result, as in the 2nd line of code in my answer. – Interknit

@Lundin: Isn't the issue the conversion before the multiplication? Your 2nd line of code implicitly converts rate and percentage to unsigned before the operation (and generates the error as in the OP question) – Majorette

Thank you Lundin and pmg. I rewrote the code as "uint16_t basic_units = (uint16_t)rate * (uint16_t)percentage;". Misra is fine with it now. – Aspirin

@Ammamon: Note that given int16_t foo(int16_t x) { if (should_fire_airbag()) fire_airbag()); return x*x;}, a compiler for a 32-bit system may, under the C standard, generate code for foo(0xFF00); that unconditionally fires the airbag. Safe forms of multiplication would be either (0u+x)*x or 1uxx. Note that compilers more than about five years old probably won't generate an unconditional fire_airbag call, but today's aggressive optimizers are more interested in cutting out any code the C Standard doesn't require them to include, than in generating code that behaves as intended. – Claxton

Answer 4

If you try casting your operands using C-style casting, you might just give your tool something else to complain about. So, instead of doing this:

uint16_t basic_units = (uint16_t)rate * (uint16_t)percentage;

You may need to do this:

uint16_t basic_units = static_cast<uint16_t>(rate) * static_cast<uint16_t>(percentage);

Marcum

The question is tagged as C so an answer using C++ casting is inappropriate ;-) – Wadmal