#624 missing type propagation to int

closed-fixed
5
2013-05-25
2003-10-29
No

Hi.

There seem to be a bug which causes a data loses during
the evaluation of some complex expressions.
The program that demonstrates the bug, is attached.
Also here it is:
---
#include <at89S8252.h>

int c;
unsigned char p;

int main()
{
p = SBUF + 13;
c = 2 | ((int)1 << p);
return c;
}
---

The result is that c==2.
The asm code in question follows:
---
;shift.c:9: c = 2 | ((int)1 << p);
; genLeftShift
mov b,_p
inc b
mov r2,#0x01
mov r3,#0x00
sjmp 00104$
00103$:
mov a,r2
add a,acc
mov r2,a
mov a,r3
rlc a
mov r3,a
00104$:
djnz b,00103$
; genCast
--> There might be no cast to byte <---
; genOr
orl ar2,#0x02
; genCast
mov _c,r2
mov (_c + 1),#0x00
--> so it was casted, the higher byte is lost <---
;shift.c:10: return c;
---

The strange thing here is that the expression
c = (1 << p);
gets evaluated correctly, but
c = (int)2 | (1 << p);
is not and only
c = (int)2 | ((int)1 << p);
is evaluated correctly.
What puzzles me is that adding "2 |" affects the
evaluation of (1 << p) making it to loose the higher
byte, while it seems logical to expect that these
expressions are independant.

Discussion

  • Stas Sergeev

    Stas Sergeev - 2003-10-29
     
  • Bernhard Held

    Bernhard Held - 2003-10-31

    Logged In: YES
    user_id=203539

    Fixed in SDCCast.c 1.193

     
  • Bernhard Held

    Bernhard Held - 2003-10-31
    • assigned_to: nobody --> bernhardheld
    • milestone: --> 100457
    • status: open --> closed-fixed
     
  • Stas Sergeev

    Stas Sergeev - 2003-10-31

    Logged In: YES
    user_id=501371

    Thanks for the fix, although I think it is not complete, or
    at least there is something fishy about it.
    It fixes the original test-case, but if I change the line
    c = 2 | ((int)1 << p);
    to
    c = 2 | (1 << p);
    the result is that c==2 again. I think this is not correct.
    Also considering that "c = 1 << p" gives the proper value,
    not zero, I suspect the bug is still there.
    Am I missing something?

     
  • Bernhard Held

    Bernhard Held - 2003-10-31

    Logged In: YES
    user_id=203539

    You're right, it's a bug.
    SDCC tries to use 8 bit values as far as possible. Therefore
    the constants 1 and 2 are "signed char". At least SDCC is
    doing the whole computation in 8 bit only (see the output with
    --dumptree). It would be easy to change everything to "int",
    SDCC would perfectly comply with the standard. But I'm sure,
    that dozens of users will complain the next day about the
    unacceptable increase of memory consumption ...

    The real bug is, that type flows only bottom up
    (decorateType()). A type propagation, which flows the other
    way round, is still missing. I've already faced the problem 2
    weeks ago, but there's still a far way to go. A lot of code will
    be necessary to handle many exceptions.

    I've re-opened your report to have a reminder for this special
    case.

     
  • Bernhard Held

    Bernhard Held - 2003-10-31
    • milestone: 100457 -->
    • summary: incorrect type casts --> missing type propagation to int
    • labels: 101550 --> Icode generator
    • status: closed-fixed --> open
     
  • Stas Sergeev

    Stas Sergeev - 2003-11-01

    Logged In: YES
    user_id=501371

    > SDCC tries to use 8 bit values as far as possible.
    > Therefore the constants 1 and 2 are "signed char".
    Yes, I suspected that, and from that point of view
    the result looks perfectly correct. What made me wonder
    however, is the fact that "c = 1 << p" gives a non-zero
    result, which means to me that in that case it automatically
    promotes the expression to int for some reasons, and
    then I don't see the reason of not doing the same when
    I add "2 |" to the expression.
    But why "c = (int)2 | (1 << p)" still gives the wrong
    result? Isn't that a different bug? I mean, if I cast 2
    to int explicitly, and "c = 1 << p" is evaluated as int,
    then why they in combination gives a char?

     
  • Bernhard Held

    Bernhard Held - 2003-11-01

    Logged In: YES
    user_id=203539

    c = (1 << p);
    is evaluated correctly, because in decorateType() is a very special
    "band aid" to fix this case. It works only for casts and
    assignments, and if the right value is the result from +, -, * or
    <<. It doesn't even look at the next node of the tree. In this special
    case type flows up one single node.

    In order to fix (and all other cases)
    c = (int)2 | ((int)1 << p);
    type has to flow up the whole tree.

     
  • Bernhard Held

    Bernhard Held - 2004-01-08

    Logged In: YES
    user_id=203539

    Fixed, see ChangeLog 1.563

     
  • Bernhard Held

    Bernhard Held - 2004-01-08
    • milestone: --> fixed
    • status: open --> closed-fixed
     
  • Stas Sergeev

    Stas Sergeev - 2004-01-11

    Logged In: YES
    user_id=501371

    Hello.

    I've finally got around to update from CVS and test the fix.
    Yes, indeed the patch works as expected, thanks.
    But... but it also works not as expected.
    Actually I am sorry to say that, but it is unacceptable
    for my needs. My program was ~7300bytes before the patch.
    With the patch applied, the program have grown up to ~8400
    bytes and no longer fits into the flash. Well, giving up
    more than 1100 bytes is not possible for me.
    I realize your patch is (hopefully) correct and does what
    I demanded myself. But now I can't use it, no matter whether
    I want to or not.
    Very sad. I don't know what to do now. Perhaps I'll have
    to no longer upgrade sdcc and stick with the last version
    that didn't have that fix.
    Maybe you have some hints as to how can I solve that
    problem?
    If you perhaps need a testcases, I can try to create a few,
    but it will show nothing even nearly compared to 1100 bytes
    losses...

     
  • Bernhard Held

    Bernhard Held - 2004-01-11

    Logged In: YES
    user_id=203539

    My patch generally casts the left operator of a left shift operation to at
    least 'signed int'. This operation costs only few bytes. I've no idea
    about the reason for the 13% increase of your code size. You've to
    supply more information.

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-12

    Logged In: YES
    user_id=501371

    OK, so this is not expected, good. Then there is a hope.
    I'll try to figure out more.

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-12

    Logged In: YES
    user_id=501371

    It seems your patch always calculates a left shift as an
    int. Even a=1<<b is now evaluated as int, while both "a"
    and "b" are "unsigned char"s.
    Perhaps it is possible to do that as a char in case the
    result is char? I know there are some fundamental problems
    with that, but at least the trivial cases can be optimized.

    My program does an enormous amount of a shifting stuff,
    although 99% of it is done with the "unsigned char"
    arguments and result. Now it all is suddenly done via ints,
    then the growth.
    Would it be possible to do something/anything/whatever
    about that?

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-12

    Logged In: YES
    user_id=501371

    Hmm, why the previous follow-up got dropped? Retrying.

    It seems your patch always calculates a left shift as an
    int. Even a=1<<b is calculated as int, while both "a" and
    "b" are the "unsigned char"s.
    Perhaps it would help to not use int in case the result and
    all the arguments in an expression are the "unsigned char"s?

    My program is doing an enormous amount of a shifting stuff,
    but 99% of it are done with the "unsigned char"s. Now they
    all are done via ints, which adds a lot.
    Would it be possible to do something/anything/whatever
    about that?

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-12

    Logged In: YES
    user_id=501371

    Ouch, it looks like I can't type the certain characters
    here, in particular the &lt sign (or two &lt signs in a
    row).
    Trying again... sorry about a spam.

    It seems your patch always evaluates a left shift as an
    int. Even the simple shifting expressions with all args
    and the result being char, are now evaluated via ints.
    Perhaps it is possible to evaluate the expression as a
    char in case all the args and the result are char's?

    My program is doing an enormous amount of shifting stuff,
    but 99% of it is done with "unsigned char" args and result.
    Now they all are evaluated via ints, which adds a lot.
    Would it be possible to do something about it?

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-13

    Logged In: YES
    user_id=501371

    OK, in case the proper solution (optimization) is untrivial
    and will have to wait, could you please at least put your
    fix under #ifdef? That way I'll be able to quickly disable
    it and keep updating from CVS. Since updating from CVS used
    to help me so much in the past, I really would like to keep
    doing that. #ifdef looks like a perfect short-terms
    solution for my problem.
    Btw, what is your opinion on that? Do you think some
    optimization is possible and can/will be added sometimes
    later, or do you think everything will stay as it is, or
    perhaps something else?
    Just wondering, no whining:)

     
  • Bernhard Held

    Bernhard Held - 2004-01-13

    Logged In: YES
    user_id=203539

    First I've to fix the bug, then he wants it back. What's next? ;-)
    I don't know why I'm doing this, because it's a waist of time, but I
    attached a diff.

    Yes, I'm already thinking about an optimization. But it's everything but
    trivial (at least for me). And it will last several months until it's ready.
    Especially if people keep me from doing productive work.

     
  • Bernhard Held

    Bernhard Held - 2004-01-13

    Disables promotion from char to int for left-shift

     
  • Stas Sergeev

    Stas Sergeev - 2004-01-14

    Logged In: YES
    user_id=501371

    No, it is not the waste of time. It allowed me to fit the
    program in the flash again by returning 800 bytes.
    Also it allowed me to detect another source of the code
    growth, which steals 300 bytes more from me.
    I have opened bug #877103 for this. Hopefully it was
    just an oversight, not a real problem like this one.

     

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