#2038 Comparisons between bool and char broken

closed-fixed
8
2013-05-25
2012-06-03
mz-fuzzy
No

Sdcc generates wrong code for condition with conditional operator.

see sdccerr.lst:
- result of rows 57-61 is: acc is set if param a is nonzero. Then, on 62-65, acc is compared with result of conditional expression. This works wrong for flag=1 and a>1.
===================
int f(char a, char flag)
{
if (a == (flag ? 1 : 0))
return 0;
return 1;
}
===================

Discussion

  • mz-fuzzy

    mz-fuzzy - 2012-06-03
     
  • mz-fuzzy

    mz-fuzzy - 2012-06-03

    prio 7 as a wrong code is generated silently

     
  • mz-fuzzy

    mz-fuzzy - 2012-06-03
    • priority: 5 --> 7
     
  • mz-fuzzy

    mz-fuzzy - 2012-06-03

    ... sorry, forgot to add:
    SDCC : mcs51/gbz80/z80/z180/r2k/r3ka/ds390/pic16/pic14/TININative/ds400/hc08/s08 3.1.5 #7827 (Jun 1 2012) (Linux)
    command line:
    sdcc -mz80 -c sdccerr.c

     
  • Philipp Klaus Krause

    This is even more serious:

    1) The bug is not z80-specific, it affects all ports.
    2) It affects all comparisons (equality, less-than, etc) between char (both signed and unsigned) and bool.

    In all these cases the char operand is cast to bool before comparing.

    In your case we have the comparison == between the char a and the bool (flag ? 1 : 0) resulting in a being cast to boolbefore the comparison.

    Philipp

     
  • Philipp Klaus Krause

    • labels: 100692 -->
    • priority: 7 --> 8
     
  • Philipp Klaus Krause

    • summary: z80: wrong code for conditional operator --> Comparisons between bool and char broken
     
  • mz-fuzzy

    mz-fuzzy - 2012-06-04

    Philip,

    for me, it's strange that you are writing about evaluating of "(flag ? 1 : 0)" as bool. It should be evaluated as "1" or "0" - meaning integers, shouldn't it? But I'm not a compiler expert, just from a logical point of view.

    Martin

     
  • Philipp Klaus Krause

    Well, by the standard it is an int. However the standard does not require we treat it that way, it only requires that the code behaves as if it was an int. In this case, making in a bool or unsigned char or char or long or float or whatever would not change the behaviour of the program - the value is either 0 or 1 and any of the types can hold both 0 and 1. So some optimization in sdcc decides to use the cheapest type that does not change the behaviour, which is bool here.
    Thus this code, even though it does not explicitly use bool then hits the bug in comparison of bool to char.

    Philipp

     
  • Maarten Brock

    Maarten Brock - 2012-06-05

    While I was creating regression tests for getByte/getWord/getHbit/getAbit I found similar issues. The fact that getSize returns 1 for both char and bool is related and should probably be replaced by bitsForType.

     
  • Maarten Brock

    Maarten Brock - 2012-06-05
    • assigned_to: nobody --> maartenbrock
     
  • Maarten Brock

    Maarten Brock - 2012-06-07

    Fixed in SDCC 3.1.5 #7875.

     
  • Maarten Brock

    Maarten Brock - 2012-06-07
    • labels: --> C-Front End
    • milestone: --> fixed
    • status: open --> closed-fixed
     
  • mz-fuzzy

    mz-fuzzy - 2012-06-08

    Hi Maarten,

    for me, sdcc #7875 generates the same buggy code for reported example. Can you please double check?
    Thanks,

    Martin

     
  • Maarten Brock

    Maarten Brock - 2012-06-09
    • status: closed-fixed --> open-fixed
     
  • Maarten Brock

    Maarten Brock - 2012-06-09

    You're right. For z80 and hc08 it still generates wrong code. Even the underlying iCodes are totally different.

     
  • Philipp Klaus Krause

    I added a (currently #if 0'ed) regression test for this at support/regression/tests/bug3531687.c in revision #7880.

    Philipp

    P.S.: AFAIK, pic handles _Bool similar to z80 and hc08.

     
  • Maarten Brock

    Maarten Brock - 2012-06-10

    The bug was caused by different behaviour for non-mcs51-likes in SDCCsymt.c which makes 0 and 1 bool in ? 1 : 0 constructs when resultType is CHAR. It should never do that for resultType CHAR.

    Should now be fixed in SDCC 3.1.5 #7882.

     
  • Maarten Brock

    Maarten Brock - 2012-06-10
    • status: open-fixed --> closed-fixed
     
  • Philipp Klaus Krause

    I disagree with how this was handled:

    1) IMO the bug is that comparisons between char and bool are broken.

    2) The test removed with "removed unrelated and already covered test" comment tests exactly comparisons between char and bool, and still fails as of revision #7882.

    3) IMO replacing char by bool where possible is a good optimization that should have stayed. Code generation for a bool operand is always at least as efficient as for a char operand (we can always handle the bool like an unsigned char). But there are cases where code for bool is more efficient. One example is in ifx (here for z80 with operand on stack):

    if(a) for a of type char on stack:

    ld a, d(ix)
    or a , a

    if(a) for a of type bool on stack:

    bit 0, d(ix)

    The latter is faster and leaves register a free for other uses.

    AFAIK most architectures have such bit-test instructions which can be used when testing a bool.

    Philipp

     
  • Philipp Klaus Krause

    • status: closed-fixed --> open-fixed
     
  • Maarten Brock

    Maarten Brock - 2012-06-12

    Ok, I was pretty sure we already had all sorts of combinations of bool and int covered, but apparently this is not true. Still this bug was not caused by a problem in comparing a char with a bool, but by casting the char to bool instead of the other way around. That comparison of bool and char fails is a different bug.

    I'm currently running regression tests for yet another try to fix these bugs.

     
  • Philipp Klaus Krause

    Sorry, it seems what I wrote was unclear.

    Comparisons of bool to char are broken, since sdcc always casts the char to bool before the comparison. All comparisons of bool to char fail due to this issue, both the ones where bool is used explicitly and the ones where bool is introduced implicitly, like what the bug reporter gave us:

    Here, the result of (flag ? 1 : 0) was optimized into a bool, which is perfectly legal. We thus have a comparison of char (a) to bool (flag ? 1 : 0). This now looks like a comparison of char to bool like any other to me. And it fails the same way as any other comparison of char to bool: By sdcc inserting a cast from char to bool for the char operand.

    Philipp

     
  • Maarten Brock

    Maarten Brock - 2012-07-21
    • status: open-fixed --> closed-fixed
     
  • Maarten Brock

    Maarten Brock - 2012-07-21

    The other bug is fixed as well in SDCC 3.2.1 #8051. The removed test is back.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks