0

snip of my code:

uint16 myArray_A[5];
uint8 myArray_B[10] = {0x1,0x2,0x3,0x4,0x5,0x6,0x7,0x8,0x9,0x10};
uint8 idx = 0;

for (idx= 0; idx< 5; idx++)
{ 
     myArray_A[idx] = ((myArray_B[idx *2]<<8) | myArray_B[((idx++ *2)+1)]);
}

The code works so far. But I get the MISRA warning from the subject. Any idea how to avoid them?

Lundin
  • 195,001
  • 40
  • 254
  • 396
JohnDoe
  • 825
  • 1
  • 13
  • 31
  • @walnut: Thanks! It was a copy paste failure – JohnDoe Mar 25 '20 at 09:57
  • Is it OK to read and modify idx in the same expression? Also, why don't you simply `idx+=2` in the loop? – Peter - Reinstate Monica Mar 25 '20 at 10:17
  • @Peter-ReinstateMonica: Ahh it was again a copy paste error :). See the edited code above. – JohnDoe Mar 25 '20 at 10:25
  • 1
    Doesn't it say on which line do you get the warning? – KamilCuk Mar 25 '20 at 10:26
  • 1
    Please don't fix the code on the fly, that gives the question "moving goal posts". I'll rollback the edit since I have now taken the time to post an answer. – Lundin Mar 25 '20 at 10:28
  • 1
    FWIW: the `idx++` in the middle of the `|` expression is ugly as sin; also (I looked it up) wrong... The C11/C18 Standard says "The grouping of an expression does not completely determine its evaluation.", and gives an example which includes a `p++`, about which it says "the actual increment of `p` can occur at any time between the previous sequence point and the next sequence point". It's worth knowing what a "sequence point" is, but _very loosely_ the `;` at the end of a statement is a sequence point (but there are others). – Chris Hall Mar 25 '20 at 10:40
  • I think the code analyzer is mistaken here. The analyzer appears to think that the right side of the assignment is `unsigned char`, while it actually is `int`, after the integer promotions (the shift operators are no exception to the promotion rules, even though that would make some sense). There is a similar issue with an analyzer complaining about an allegedly "widening" cast [here](https://stackoverflow.com/q/47491192/3150802). It would help if you could tell us which analyzer produced this diagnostic (was it a normal compiler or a specialized tool?). One might file a bug report. – Peter - Reinstate Monica Mar 25 '20 at 10:59
  • @Peter-ReinstateMonica It isn't wrong, the right operand is what MISRA-C calls "essentially unsigned". The essential type concept is MISRA specific and serves to group types in categories, so that it can list which kind of conversions that are safe and which ones that aren't. Implicit conversion from unsigned char to int is generally dangerous. The tool correctly reports a violation against MISRA-C:2012 10.8, though there are many other violations in that single line too. – Lundin Mar 25 '20 at 11:40
  • @Lundin The error says "widening on assignment of essentially unsigned to wider unsigned type". This implies three things: (1) The complaint is about a *widening* (which may be surprising at first glance, because widening is not usually a problem); (2) The widening is performed upon the assignment, not during any computation or promotion on the right side; (3) the complaint is not about a sign change: Both the source and the target type are unsigned. For the rule and examples, see http://www.iarsys.co.jp/download/LMS2/arm/7502/ewarm7502doc/arm/doc/EW_CSTATGuide.ENU.pdf. – Peter - Reinstate Monica Mar 25 '20 at 11:52
  • @Lundin Specifically, a widening example is `int s16a = 3; int s16b = 3; /* arithmetic makes it a complex expression */ long long x = (long long)(s16a + s16b);` The "complex" (or, in the OP, "composite") condition exempts casts of simple variables or array elements etc. – Peter - Reinstate Monica Mar 25 '20 at 11:53
  • @Peter-ReinstateMonica The key here is that essential type is the type of the operand _before_ any promotion takes place. The operands are essentially uint8 and the result is assigned to a variable which is essentially uint16. Historically, MISRA-C have been afraid of beginner programmers being so daft that they think that the arithmetic in `uint32_t a = u16a + u16b;` is carried out on `uint32_t` just because the result is stored in such a type. I always found that to be a silly concern, but maybe they based this rule on some actual population studies... they don't give out a source. – Lundin Mar 25 '20 at 12:12
  • 1
    @Peter-ReinstateMonica - one of the curious issues with C is that shifting a `unsigned char` (or `uint8_t`) results in a `signed integer`... so your shift not only widens, but converts unsigned to signed! – Andrew Mar 26 '20 at 07:29

1 Answers1

8

You have lots of problems in this code.

  • The specific warning would probably be about the operands of | being uint8 but you assign them to a wider unsigned type uint16. In general, you need to study Implicit type promotion rules if you are to attempt MISRA compliance. The MISRA document itself is also good study material regarding this.
  • myArray_B[idx *2]<<8 is regardless of MISRA dangerous code, since there is a silent promotion to a signed type. I recommend to always convert to a large, unsigned integer type before using bit shifts.
  • Similarly, the | is carried out on promoted operands that are signed.
  • myArray_A[idx] = ... idx++ is the FAQ beginner bug, invoking undefined behavior. You cannot write code like that in C. Why are these constructs using pre and post-increment undefined behavior?.
  • Modifying a loop iterator from inside a for loop is horrible practice and a violation of MISRA-C 14.2.
  • Don't use home-brewed integer types, use stdint.h.
  • MISRA requires u suffix on integer constants that are meant to be unsigned (doesn't matter the slightest in this specific case though).

To fix the bugs and get MISRA-C compliance, you should rewrite it into something along the lines of this:

uint16_t myArray_A[5];
uint8_t myArray_B[10] = {0x1u,0x2u,0x3u,0x4u,0x5u,0x6u,0x7u,0x8u,0x9u,0x10u};

for (uint8_t i=0u; i<5u; i++)
{ 
     myArray_A[i] = ((uint16_t)myArray_B[i*2u] << 8u) |
                    ((uint16_t)myArray_B[i*2u + 1u]);
}
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • 1
    @KamilCuk Ah right... come to think of it, maybe wasn't that much more readable to change the loop then. – Lundin Mar 25 '20 at 10:29
  • Thanks for the answer! I understood the issue. But after changing the code I got the MISRA Error: An expression which is the result of a ~ or << operation has not been cast to its essential type. It appears on the first uint16 cast – JohnDoe Mar 25 '20 at 10:54
  • So my understanding is that I need to cast back the complete expression to uint8 ? – JohnDoe Mar 25 '20 at 11:43
  • @JohnDoe Sounds like a false positive to me. Which rule does it point at? Such casts were required in MISRA-C:2004 but not in 2012. To cast _before_ shifting isn't explicitly required by MISRA, but it makes the code much more rugged. For example `(uint32_t)(1<<31)` invokes undefined behavior _before_ the cast is executed, but `(uint32_t)1 << 31` avoids undefined behavior all together. – Lundin Mar 25 '20 at 11:46
  • @JohnDoe That doesn't make much sense. Basically that rule is saying that both operands of for example `|` should have the same type, which they have. It matters if your system is 8/16 or 32 bits though. In case it is 32 bit, I guess MISRA-C might be annoyed that the sub-expressions `i*2u` etc have a wider essential type than the whole expression, even though that's harmless. If so you could try to do something like `size_t index1 = i*2u; size_t index2 = i*2u+1u;` and then `myArray_B[index1]` etc, but that would be a nonsense remark regardless. – Lundin Mar 25 '20 at 11:58
  • @JohnDoe More likely though, your tool is broken. In MISRA-C:2004 we had "if the bitwise operators ~ and << ... the result shall be immediately cast to the underlying type". That sounds much more like the error you quoted than MISRA-C.2012 10.7, which is about something else entirely. – Lundin Mar 25 '20 at 12:02
  • Note: `8u` in `<< 8u` is OK (and more clear), yet I think the `u` here is optional as the type for the shift length does not influence the shift result. – chux - Reinstate Monica Mar 25 '20 at 14:17
  • @chux-ReinstateMonica Yep, spamming `u` all over is pointless in 9 out of 10 cases. Static analysers don't tend to be intelligent enough to know the specific promotion rule of shift operators. – Lundin Mar 25 '20 at 14:19
  • @Lundin: So I solved the issue by doing this cast ((uint16_t)(uint16_t)(myArray_B[i*2u] << 8u)) . – JohnDoe Mar 26 '20 at 08:00
  • @JohnDoe That's nonsense and not a MISRA requirement. It sounds like you have a tool bug in the static analyser. – Lundin Mar 26 '20 at 08:30