Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#1420 explicit typecast is ineffective for unsigned char parameter

closed-fixed
Borut Ražem
7
2013-05-25
2008-01-18
Laszlo BORTEL
No

The compiler seems to ignore the explicit (unsigned char) typecast when it is applied to the actual parameter of a function with variable arguments. (Explicit (unsigned char) typecast is necessary even for unsigned char type otherwise the compiler would automatically push a two byte integer onto the stack.) The bug for example blocks the use of printf() function with "%bu" format specifier.

C:\>sdcc -v
SDCC : mcs51/gbz80/z80/avr/ds390/pic16/pic14/TININative/xa51/ds400/hc08 2.7.4 #4972 (Nov 23 2007) (MINGW32)

C:\>ver

Microsoft Windows XP [Version 5.1.2600]

Discussion

<< < 1 2 3 (Page 3 of 3)
  • Borut Ražem
    Borut Ražem
    2008-02-23

    Logged In: YES
    user_id=568035
    Originator: NO

    Maarten, what do you think about the command-line switch proposed by Laszlo? I have to say that I'm not very favorable to it...

    I've found the piece of code which disables the integer promotion of varargs in case of explicit type cast: it is in SDCCast.c, function processParms(), condition statement around the comment /* Parameter was explicitly typecast; don't touch it. */. If the condition is removed the integer propagation is done even in case of explicit type casting.

    And now a question: as far as I could find, the %b and %B printf modifiers, which take one byte parameter, are available only in device/lib/printf_large.c. Is this correct?

    BTW: I found the %b modifier also in pic libraries: device/lib/pic16/libc/stdio, files printf_tiny.c and vfprintf.c, but the meaning is totally different: it means output in binary format.

    Borut

     
  • Maarten Brock
    Maarten Brock
    2008-02-24

    Logged In: YES
    user_id=888171
    Originator: NO

    I don't like to add another command-line switch for this.

    I looked at the history of that piece that disables the promotion and it was added together with varargs support in revision #541. I also seem to remember that I once bumped into this issue of a lost cast when changing some code, but I can't find it anymore. Somehow I have the feeling that I broke this behaviour by removing 'unnecessary' casts in favour of better optimization. But this might just as well have been during a local test and was never commited.

    Even though this is non-standard behaviour I would somehow like to keep it for SDCC. SDCC is targeted for small (8 bit) devices and I think this is a very useful extension.

    Yes %b modifiers are only in printf_large as far as I know. And they have been there since the initial revision. See
    http://sdcc.svn.sourceforge.net/viewvc/sdcc/trunk/sdcc/device/lib/vprintf.c?revision=3&view=markup
    The pics use %b for something else as you found out. Maybe we should split and merge them. How about %B for bytes and %b for bits?

    Maarten

     
  • Borut Ražem
    Borut Ražem
    2008-02-24

    Logged In: YES
    user_id=568035
    Originator: NO

    Maarten, if I understand you correctly, you are more favorable to the solution proposed by Lazslo. In this case the following condition should be removed from file SDCCasc.c, function decoreteType(), case CAST:

    /* if 'from' and 'to' are the same remove the superfluous cast, */
    /* this helps other optimizations */
    if (compareTypeExact (LTYPE(tree), RTYPE(tree), -1) == 1)
    {
    return tree->right;
    }

    This lines were added in svn revision #4124 by Bernhard Held on 2006-04-25 with the comment: "partial fix for RFE 1475769, remove cast to same type". If we revert this change, we will probably have other problems and lose some optimizations, as you wrote, and we would not be compliant with the ISO C standard.

    Maybe introduction of a command line switch is not so bad idea after all...

    Borut

     
  • Borut Ražem
    Borut Ražem
    2008-02-24

    Logged In: YES
    user_id=568035
    Originator: NO

    The better solution would be to execute the if (compareTypeExact(...) == 1) condition only in case if the left operand of cast is not actual function parameter:

    if (!ACTUAL_FUNCTION_PARAMETER (tree->right) && compareTypeExact (LTYPE(tree), RTYPE(tree), -1) == 1)
    {
    return tree->right;
    }

    Now the problem is how the ACTUAL_FUNCTION_PARAMETER() should be implemented: the CAST is evaluated before the CALL, so we probably don't know that the operand is a function parameter?

    Any idea?

    Borut

     
  • Maarten Brock
    Maarten Brock
    2008-02-24

    Logged In: YES
    user_id=888171
    Originator: NO

    Apart from introducing a new command line switch I don't see a solution by Laszlo. SDCC has had special behaviour for this from the beginning and allthough non-standard I don't see a reason to reject it now.

    But I'm really not in favour of yet another switch. And thinking of it now, why not use --std-cXX vs. --std-sdccXX switches. If this is a SDCC specific language extension then it should be enabled by --std-sdccXX and disabled by --std-cXX.

    Is this code in decorateType() the part that removes the cast in this case? Maybe we should find a way to remember that it was an explicit or implicit cast. And also use that in processParms like literalFromCast.

    Maarten

     
  • Borut Ražem
    Borut Ražem
    2008-02-24

    • milestone: --> fixed
    • status: open --> closed-fixed
     
  • Borut Ražem
    Borut Ražem
    2008-02-24

    Logged In: YES
    user_id=568035
    Originator: NO

    Fixed as proposed in my previous post: casts of the same type of actual function parameters aren't removed (optimized). The ISO C standard nonconformity (or sdcc extension) remains and backward compatibility is not broken.

    Borut

     
  • Maarten Brock
    Maarten Brock
    2008-02-24

    Logged In: YES
    user_id=888171
    Originator: NO

    Borut,
    Unless I am mistaken this holding on to the cast is only relevant for varargs functions. Looking at your patch is it not possible to change .actualArgument to .actualVarArg and only set it for a varargs function? This could keep the loss of other optimizations low.
    Maarten

     
  • Borut Ražem
    Borut Ražem
    2008-02-24

    Logged In: YES
    user_id=568035
    Originator: NO

    Maarten, you are right. I committed the version which implements the values.removedCast flag, as you proposed. Please review the fix. I'm not sure if testing for IS_AST_SYM_VALUE (*actParm) is correct and needed when testing for AST_VALUES (*actParm, removedCast).

    Borut

     
<< < 1 2 3 (Page 3 of 3)