Possible compiler bug ?

[saraben]

Here is a test (sanity check) which if I haven't made a silly mistake or misunderstand some aspect of C is possibly a compiler bug.
It shows that for uint16_t type, conditional statements with expressions, not simple variables or constants, yield incorrect results.
This is a test for a problem I have in the Nil chSysTimerHandlerI and chSchGoSleepTimeoutS functions in Tickless mode. See [url]
viewtopic.php?f=12&t=1301&start=10#p15278[/url]

The following code is inserted into a demo as a thread. It requires SD2 be running.

Code: Select all

/*
 * Test code thread.
 */
static THD_WORKING_AREA(waThread4, 128);
static THD_FUNCTION(Thread4, arg) {

  (void)arg;
  chRegSetThreadName("TestCode");

#define BITS_16  TRUE    // Set to FALSE for 32 bit

  chnWrite(&SD2, (const uint8_t *)"-----------------\r\n", 19);
  chnWrite(&SD2, (const uint8_t *)"Testing C code if statement\r\n", 29);

#if BITS_16
  uint16_t a = 100;
  chnWrite(&SD2, (const uint8_t *)"uint16_t a = 100;\r\n", 19);
  uint16_t b = 0;
  chnWrite(&SD2, (const uint8_t *)"uint16_t b =   0;\r\n", 19);
#else
  uint32_t a = 100;
  chnWrite(&SD2, (const uint8_t *)"uint32_t a = 100;\r\n", 19);
  uint32_t b = 0;
  chnWrite(&SD2, (const uint8_t *)"uint32_t a =   0;\r\n", 19);
#endif

  chnWrite(&SD2, (const uint8_t *)"-----------------\r\n", 19);
  chnWrite(&SD2, (const uint8_t *)"a <= b ", 7);
  if (a <= b)
    chnWrite(&SD2, (const uint8_t *)" : TRUE\r\n", 9);
  else
    chnWrite(&SD2, (const uint8_t *)" : FALSE\r\n", 10);

  chnWrite(&SD2, (const uint8_t *)"-----------------\r\n", 19);

  chnWrite(&SD2, (const uint8_t *)"a <= (b - 1) ", 13);
  if (a <= (b - 1))
    chnWrite(&SD2, (const uint8_t *)" : TRUE\r\n", 9);
  else
    chnWrite(&SD2, (const uint8_t *)" : FALSE\r\n", 10);

  while (true) {
    chThdSleepMilliseconds(500);
  }
}

// In main add
  chThdCreateStatic(waThread4, sizeof(waThread4), NORMALPRIO+4, Thread4, NULL);

Now if we set
#define BITS_16 FALSE
We get

Code: Select all

-----------------
Testing C code if statement
uint32_t a = 100;
uint32_t a =   0;
-----------------
a <= b  : FALSE
-----------------
a <= (b - 1)  : TRUE

Which looks ok to me. In the second statement the left side is 100 and right side should be 4294967295.

But if we set
#define BITS_16 TRUE
We get

Code: Select all

-----------------
Testing C code if statement
uint16_t a = 100;
uint16_t b =   0;
-----------------
a <= b  : FALSE
-----------------
a <= (b - 1)  : FALSE

As far as I understand, that last result is incorrect. The right side should be 65535 which is much greater than 100.
It is my understanding that in C uint '-1' is defined as uint_MAX.
This was run on an STM32, complied with gcc-arm-none-eabi-4_7-2013q3 or gcc-arm-none-eabi-4_8-2014q1.

Could someone please review this code and possibly try on a different compiler.

Regards
saraben.

[saraben]

Some further infomation.

For the uint16_t case the compiler produces

Code: Select all

             if ((b - 1) > a)
0800a2b2:   ldrh.w r2, [sp, #14]
0800a2b6:   ldrh.w r3, [sp, #12]
0800a2ba:   subs r3, #1
0800a2bc:   cmp r2, r3
0800a2be:   bgt.n 0x800a2d0 <Thread4+208>

For the uint23_t

Code: Select all

 75         if (a <= (b - 1))
0800a2aa:   ldr r3, [sp, #8]
0800a2ac:   subs r2, r3, #1
0800a2ae:   ldr r3, [sp, #12]
0800a2b0:   cmp r2, r3
0800a2b2:   bcc.n 0x800a2c4 <Thread4+196>

Note that r2 and r3 are swapped in the comparison between the 16 and 32 bit compiler outputs.
The 32 bit case uses BCC which looks correct and gives the correct answer.

Now looking at ARM DUI 0552A, Cortex -M3 Devices Generic User Guide page 3-15 for Condition code suffixes.

Code: Select all

Suffix           : Flags                    Meaning
BGT             : Z = 0 and N = V  : Greater than, signed
BHI              : C = 1 and Z = 0   : Higher, unsigned
BCC or BLO : C=0                    : Lower, unsigned
BGE             : N=V                   : Greater than or equal, signed
BCS or BHS : C=1                    : Higher or same, unsigned
BLE              : Z = 1 and N != V : Less than or equal, signed
BLS              : C = 0 or Z = 1     : Lower or same, unsigned

It would appear that the test in the unsigned 16 bit output should have been BHI instead of BGT as we are dealing with unsigned variables.

If the <= is replaced by < the "BGT" the compiler output test becomes BGE which again looks wrong. Should be BCS or BHS.

If the condition is turned arround "(b - 1) > a" the compiler output test becomes BLE which also look wrong. Should be BLS.

I hope this makes sense and I am not completely missing somerhing.
Regards
saraben

[Giovanni]

Interesting, it would be nice to write a minimum code that fails because of this, then we could submit it on the GCC mailing list. The confirmation could be compiling it using Keil or IAR.

Giovanni

[saraben]

Giovanni,
I was only thinking of it being only an ARM issue, but trying the following.

Code: Select all

/*
 * Test code for C "if" statements as expressions as conditions 
 * Chris Healy 2014-4-16
 * */

#include <stdio.h>
#include <string.h>
#include <stdint.h>

int main(void) {
        
  printf("########################################\r\n");
  printf("Testing GCC X86 C code if statement\r\n", 29);
  printf("########################################\r\n");
  // ### Testing uint16_t if statement #####
  printf("uint16_t a = 100;\r\n", 19); 
  printf("uint16_t b =   0;\r\n", 19);
  printf("a <= b ");
  uint16_t a = 100;
  uint16_t b = 0;
  if (a < b)
    printf(" : TRUE  NOT OK\r\n");
  else
    printf(" : FALSE  OK\r\n");

  printf("a <= (b - 1) ");
  
  if (a < (b - 1))
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");

  printf("(b - 1) > a");
  if ((b - 1) > a)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
    
  printf("a <= --b");
  if (a <= --b)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
    
  printf("--b > a");
  if (--b > a)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
    
  printf("########################################\r\n");
  // ### Testing uint32_t if statement #####
  printf("uint32_t c = 100;\r\n");
  printf("uint32_t d =   0;\r\n");
  printf("c <= d ");
  uint32_t c = 100;
  uint32_t d = 0;

  if (c < d)
    printf(" : TRUE  NOT OK\r\n");
  else
    printf(" : FALSE  OK\r\n");

  printf("c <= (d - 1) ");
  if (c < (d - 1))
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");

  printf("(d - 1) > c");
  if ((d - 1) > c)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
    
  printf("c <= --d");
  if (c <= --d)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
    
  printf("--d > c");
  if (--d > c)
    printf(" : TRUE  OK\r\n");
  else
    printf(" : FALSE  NOT OK\r\n");
  printf("########################################\r\n");

        return 0;
}

Compiler gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
Target: x86_64-linux-gnu
Yields.

Code: Select all

########################################
Testing GCC X86 C code if statement
########################################
uint16_t a = 100;
uint16_t b =   0;
a <= b  : FALSE  OK
a <= (b - 1)  : FALSE  NOT OK
(b - 1) > a : FALSE  NOT OK
a <= --b : TRUE  OK
--b > a : TRUE  OK
########################################
uint32_t c = 100;
uint32_t d =   0;
c <= d  : FALSE  OK
c <= (d - 1)  : TRUE  OK
(d - 1) > c : TRUE  OK
c <= --d : TRUE  OK
--d > c : TRUE  OK
########################################

The same as for the stm32F103.
So if the uint32_t behaviour is in accordance with standard C, there is a bug for uint16_t.
I don't understand how it could have been around for so long. 4.6.3 is a few years old.
uint16_t types are relatively common for 32 bit microcontrollers.
Perhaps there is a habit of programmers not putting expressions in if statements. I had a look at your code and only found one x++ in an if. All the others are variables or constants.
Regards
saraben

EDIT:
Added tests for --b and --d in the above, as I found some lines with that format in Chibios.
They actually look OK.

[panacekcz]

The behavior you are observing is expected, it is called integer promotion. (see https://www.securecoding.cert.org/confl ... sion+rules or somewhere else)
As uint16_t fits into int, it is converted to int before doing the subtraction, so you are comparing signed integers.

[saraben]

panacekcz,
Now I'm really confused.

The behavior you are observing is expected, it is called integer promotion. (see https://www.securecoding.cert.org/confl ... sion+rules or somewhere else)
As uint16_t fits into int, it is converted to int before doing the subtraction, so you are comparing signed integers.

The reference distinguished between integers and unsigned integers.

If all values of the original type can be represented as an int, the value of the smaller type is converted to an int; otherwise, it is converted to an unsigned int.

However further down it states

The rank of any unsigned integer type shall equal the rank of the corresponding signed integer type, if any.

Which is a bit confusing.

I added some further tests as follows.

Code: Select all

uint16_t a = 100;
uint16_t b =   0;
uint16_t z =   0;
a <= b  : FALSE  OK
a <= (b - 1)  : FALSE  NOT OK
(b - 1) > a : FALSE  NOT OK

a <= (b - z)  : FALSE  NOT OK
(b - z) > a : FALSE  NOT OK

a <= (unsigned int)(b - 1)  : TRUE  OK
(unsigned int)(b - z) > a : TRUE  OK

a <= --b : TRUE  OK
--b > a : TRUE  OK

So the two with the cast within the conditional expression give the required (as distinct from expected) answer. This appears to indicate that the compiler is treating the expression result as signed and it would be converted back to unsigned in a normal expression but it isn't normally as part as a condition.

However, if that is the case why do the uint32_t type cases give the required answer.
Should not in

Code: Select all

uint32_t c = 100;
uint32_t d =   0;
c <= (d - 1)  : TRUE  OK

As (int)100 is greater than d - 1 = (int)-1 then the result should be false.
I tried
c <= (unsigned int)(d - 1) : TRUE OK
c <= (int)(d - 1) : TRUE OK

So now I'm really confused.
I don't have access to the ISO to look furthrer but it looks like comparisons, with expressions in the comparisons, on unsigned integers or at least reduced lengthed unsigned integers may be unreliable for the type of application Giovanni is implementing.

Regards
saraben

EDIT:
OK I just talked to my son about this and the difference between the 32 bit and 16 bit is that a uint16_t can be represented in a int32_t but a uint_32 doesn't fit in int32_t. So the 32 is forced to be completely unsigned.
It does highlight that the result will be architecture dependant when using reduced length unsigned integers unless the results of expressions are explicitly cast.
I wonder why my code on X86_64 doesn't give the undisered result. I thought the native int was 64 bit.

Giovanni, I tried the following and it appears to work fine.
In chSysTimerHandlerI

Code: Select all

      else {
        if (tp->timeout <= (systime_t)(next - 1))
          next = tp->timeout;
      }

and in chSchGoSleepTimeoutS

Code: Select all

 if ((systime_t)(abstime - nil.lasttime) < (systime_t)(nil.nexttime - nil.lasttime)) {
IN PLACE OF
  if (chVTIsTimeWithinX(abstime, nil.lasttime, nil.nexttime)) {

Note the casts could probably be to (uint) and give more universal required reault.

It is getting a bit late for me to play around with macros.

But if this is how conditional statements are meant to relate to contained expresseions in C I think there is too much potential for hard to find intermittent bugs.
It would be more reliable never to have expressions in comparison statements.

Regards
saraben

[saraben]

OK, thanks to help from panacekcz, here is a summary of handling unsigned integers, in particular comparisons.
I hope it is a bit clearer than the above content. If there are any error please let us know.

Unlike many comments that "overflows" and "underflows" are handled automatically in C for unsigned integers, this does not always hold true for non architecture native unsigned integer types, i.e. uint8_t and uint16_t on 32 bit systems or possibly uint32_t on 64 bit systems.

The issue is that in C, if unsigned integers can be represented as a signed integer, they are converted to a signed integer before evaluation of an expression.
If the unsigned signed integer is of native format (eg uint32_t on 32 bit system) the conversion cannot take place, as a uint32_t will not fit in int32_t and the expression must be evaluated unsigned.
However for reduced length unsigned integers (eg unsigned16_t on a 32 bit system) the conversion can take place and the uint16_t represented as an int (int32_t).
The reason for this is fairly obvious as the processor operations and flags work on the whole of regesters and it would be complicated to handle reduced length unsigned integers.
In normal expressions this is compensated for because the result is placed in a variable of the same type as the right side variables and the result will be truncated to uint16_t.

However if we have in a 32 bit architecture,

if ( ((uint16_t)a - uint16_t)b) <= ((uint16_t)c - uint16_t)d) ) {...}

The results of ((uint16_t)a - uint16_t)b) and ((uint16_t)c - uint16_t)d) are of type int (int32_t in our example), that is SIGNED so the <= operation is done SIGNED.

This leads to what may be an unintended result even if it may be the expected result in the C standard.

So if the intended result is a 16 bit unsigned comparison, it is necessary to cast the results of the individual expressions.
if ( (uint16_t)((uint16_t)a - uint16_t)b) <= (uint16_t)((uint16_t)c - uint16_t)d) ) {...}

The practical problem is that the type declaration of variables is normally quite removed from such a conditional statement and it is not immediately apparent that an unsigned comparison may be required but not being performed.
There are also problems when code is ported from one platform to another. The native size may change or as in the case that resulted in this discussion, the length of a variable (counter size) may change leading to different results than on the original platform.

So to make code portable and avoid this issue, the safest method would be to only use variables and constants in comparisons, no maths. Otherwise all comparisons of unsigned integers with fixed length must include a cast for the results of expressions.

I hope this helps avoid some of those hard to find niggly intermittent bugs that occur in embedded software.

Regards
saraben