#1261 badly optimized xdata pointer

closed-accepted
Maarten Brock
None
5
2013-05-25
2006-12-18
wek
No

SDCC : mcs51/gbz80/z80/avr/ds390/pic16/pic14/TININative/xa51/ds400/hc08
2.6.1 #4478 (Nov 23 2006) (MINGW32)

When compiling the attached snippet for mcs51 (using plainly "sdcc.exe testbug.c"), both *px translates to

mov dptr,#_x
movx a,@dptr

i.e. *x, which may not be true.

In this short test program, I am unable to insert some code modifying px before "if", to demonstrate that the "if" is not redundant, without "spoiling the error", but I have a long-long program where this really happens.

Jan Waclawek

Discussion

  • wek
    wek
    2006-12-18

    testbug.c

     
    Attachments
  • to be copied into support/regression/tests

     
    Attachments
  • Logged In: YES
    user_id=589052
    Originator: NO

    the attached file "bug1618050.c" allows
    to reproduce the bug within the regression
    test suite.
    All targets except host and hc08 fail.
    File Added: bug1618050.c

     
  • Logged In: NO

    the offending piece of code can be reduced, for debugging purpose to

    -----------------------

    data char * data px;
    data char x;

    void fiddle_px(data char * data);

    char foo(void) {
    px = &x;
    fiddle_px(&x);
    return *px;
    }

    ---------------------

    the generated code looks like this:

    ; bug-8b.c:11: px = &x;
    mov _px,#_x
    ; bug-8b.c:13: fiddle_px(&x);
    mov dpl,#_x
    lcall _fiddle_px
    ; bug-8b.c:15: return *px;
    mov dpl,_x
    ret

    demonstrating, the compiler erronously assumes
    px can't change across the function call

    there is no commandline switch from the --no.* familly
    which avoids this bug.

    -------------------------

    debugging the function packRegisters in src/mcs51/ralloc.c
    reveals, in

    /* if straight assignment then carry remat flag if
    this is the only definition */
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    {
    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    OP_SYMBOL (IC_RIGHT (ic))->rematiCode;
    }

    the variable px on the left side of the assignment will be marked *remat*
    gdb call PICC(ic) before that sequence shows

    (l11:s4:k4:d0:s0) _px [k2 lr0:0 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* } := iTemp0 [k4 lr3:5 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* }
    (l13:s5:k6:d0:s0) send iTemp0 [k4 lr3:5 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* }{argreg = 1}
    (l13:s6:k7:d0:s0) iTemp3 [k8 lr6:6 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void} = call _fiddle_px [k6 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( char near* ) }
    (l15:s7:k8:d0:s0) iTemp4 [k9 lr7:8 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* } := _px [k2 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* }
    (l15:s8:k9:d0:s0) iTemp5 [k10 lr8:9 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{data-char} = @[iTemp4 [k9 lr7:8 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* }]
    (l15:s9:k10:d0:s0) ret iTemp5 [k10 lr8:9 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{data-char}

    and behind that sequence

    (l11:s4:k4:d0:s0) _px [k2 lr0:0 so:0]{ ia1 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* } := iTemp0 [k4 lr3:5 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* }
    (l13:s5:k6:d0:s0) send iTemp0 [k4 lr3:5 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* }{argreg = 1}
    (l13:s6:k7:d0:s0) iTemp3 [k8 lr6:6 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void} = call _fiddle_px [k6 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( char near* ) }
    (l15:s7:k8:d0:s0) iTemp4 [k9 lr7:8 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* } := _px [k2 lr0:0 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0}{char data-near* }
    (l15:s8:k9:d0:s0) iTemp5 [k10 lr8:9 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{data-char} = @[iTemp4 [k9 lr7:8 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{char data-near* }]
    (l15:s9:k10:d0:s0) ret iTemp5 [k10 lr8:9 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{data-char}

    that is _px is now marked rm1
    when i disable this if block, the compiler produces correct code.

    i suppose this "if" needs another condition,
    checking whether there is a function call
    in the liverange of the variable.

    Robert Larice
    larice 0x40 vidisys 0x2e de

     
  • Logged In: NO

    this patch seems to be a temporary workaround

    ---------------------------

    --- sdcc-4870/src/ds390/ralloc.c 2007-06-28 11:18:31.000000000 +0200
    +++ sdcc-4875-all/src/ds390/ralloc.c 2007-07-08 13:36:37.000000000 +0200
    @@ -3065,18 +3065,18 @@ packRegisters (eBBlock ** ebpp, int bloc
    /* if straight assignment then carry remat flag if
    this is the only definition */
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    - {
    + if(0) { // FIXME disabled due to bug 1618050
    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    OP_SYMBOL (IC_RIGHT (ic))->rematiCode;
    }

    /* if cast to a generic pointer & the pointer being
    cast is remat, then we can remat this cast as well */
    --- sdcc-4870/src/mcs51/ralloc.c 2007-06-28 11:16:35.000000000 +0200
    +++ sdcc-4875-all/src/mcs51/ralloc.c 2007-07-08 13:35:22.000000000 +0200
    @@ -3000,18 +3000,18 @@ packRegisters (eBBlock ** ebpp, int bloc
    /* if straight assignment then carry remat flag if
    this is the only definition */
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    - {
    + if(0) { // FIXME disabled due to bug 1618050
    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    OP_SYMBOL (IC_RIGHT (ic))->rematiCode;
    }

    /* if cast to a generic pointer & the pointer being
    cast is remat, then we can remat this cast as well */
    --- sdcc-4870/src/z80/ralloc.c 2007-06-28 11:17:41.000000000 +0200
    +++ sdcc-4875-all/src/z80/ralloc.c 2007-07-08 13:38:22.000000000 +0200
    @@ -2974,17 +2974,17 @@ packRegisters (eBBlock * ebp)
    /* Safe: just propagates the remat flag */
    /* if straight assignment then carry remat flag if this is the
    only definition */
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    - {
    + if(0) { // FIXME disabled due to bug 1618050

    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    OP_SYMBOL (IC_RIGHT (ic))->rematiCode;
    }

    /* if the condition of an if instruction is defined in the

    ---------------------------------

    not only mcs51 suffers from the problem, but z80 and ds390 as well,
    and only those.

    i've run support/regression with this additional test file

    ----------------------------------
    bug1618050b.c

    /** bug 1618050
    * global variable px in function foo, is marked rematr..
    * and is erronously assumed to retain its value "x" across
    * the function call to fiddle_px
    */

    #include <testfwk.h>

    data char * data px;
    data char x[2] = {0,42};

    void fiddle_px(data char * unused) {
    (volatile char) unused[0]; // shut up
    px++;
    }

    char buggy(void) {
    px = x;
    fiddle_px(x);
    return *px;
    }

    void
    testBug(void) {
    ASSERT(buggy() == 42);
    }

    ------------------------

    unfortunatly the patch lets creep in another serious problem,
    but only for ds390

    Processing bitfields.c
    gen/ds390/bitfields/bitfields.out:5:--- FAIL: "Assertion failed" on size2b_bf.b0==1 at gen/ds390/bitfields/bitfields.c:226
    gen/ds390/bitfields/bitfields.out:6:--- FAIL: "Assertion failed" on size2b_bf.b0==1 at gen/ds390/bitfields/bitfields.c:239

    Robert Larice
    larice 0x40 vidisys 0x2e de

     
  • Robert Larice
    Robert Larice
    2007-07-11

    Logged In: YES
    user_id=1840151
    Originator: NO

    I've attached a less restrictive patch, (_patch-1618050-snd.diff)
    which adds a condition for !globalness of the target variable.
    The patch contains the testcode vor regression as well.
    Applied to #4879 this allows a successufull run of support/regression

    A side note:
    there is a subtle difference between
    mcs51/ralloc.c ds390/ralloc.c and z80/ralloc.c
    that is, z80 doesn't check for
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    is this by accident, or intentionally ?

    Robert Larice

     
  • Robert Larice
    Robert Larice
    2007-07-11

    Logged In: YES
    user_id=1840151
    Originator: NO

    --- sdcc-4879/src/ds390/ralloc.c 2007-06-28 11:18:31.000000000 +0200
    +++ sdcc-4879-gdb/src/ds390/ralloc.c 2007-07-11 20:12:03.000000000 +0200
    @@ -3067,10 +3067,11 @@
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    + !isOperandGlobal(IC_RESULT(ic)) && /* due to bug 1618050 */
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    {
    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    --- sdcc-4879/src/mcs51/ralloc.c 2007-06-28 11:16:35.000000000 +0200
    +++ sdcc-4879-gdb/src/mcs51/ralloc.c 2007-07-11 20:12:24.000000000 +0200
    @@ -3002,10 +3002,11 @@
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    + !isOperandGlobal(IC_RESULT(ic)) && /* due to bug 1618050 */
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    {
    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    OP_SYMBOL (IC_RESULT (ic))->rematiCode =
    --- sdcc-4879/src/z80/ralloc.c 2007-06-28 11:17:41.000000000 +0200
    +++ sdcc-4879-gdb/src/z80/ralloc.c 2007-07-11 20:12:51.000000000 +0200
    @@ -2976,10 +2976,11 @@
    only definition */
    if (ic->op == '=' &&
    !POINTER_SET (ic) &&
    IS_SYMOP (IC_RIGHT (ic)) &&
    OP_SYMBOL (IC_RIGHT (ic))->remat &&
    + !isOperandGlobal(IC_RESULT(ic)) && /* due to bug 1618050 */
    bitVectnBitsOn (OP_SYMBOL (IC_RESULT (ic))->defs) <= 1)
    {

    OP_SYMBOL (IC_RESULT (ic))->remat =
    OP_SYMBOL (IC_RIGHT (ic))->remat;
    diff -P -x .svn --rec -u5 sdcc-4879/support/regression/tests/bug1618050b.c sdcc-4879-gdb/support/regression/tests/bug1618050b.c
    --- sdcc-4879/support/regression/tests/bug1618050b.c 1970-01-01 01:00:00.000000000 +0100
    +++ sdcc-4879-gdb/support/regression/tests/bug1618050b.c 2007-07-11 20:07:14.000000000 +0200
    @@ -0,0 +1,26 @@
    +/** bug 1618050
    +* global variable px in function buggy, is marked rematr..
    + * and is erronously assumed to retain its value "x" across
    + * the function call to fiddle_px
    + */
    +
    +#include <testfwk.h>
    +
    +data char * data px;
    +data char x[2] = {0,42};
    +
    +void fiddle_px(data char * unused) {
    + (volatile char) unused[0]; // shut up
    + px++;
    +}
    +
    +char buggy(void) {
    + px = x;
    + fiddle_px(x);
    + return *px;
    +}
    +
    +void
    +testBug(void) {
    + ASSERT(buggy() == 42);
    +}

     
  • Maarten Brock
    Maarten Brock
    2008-03-04

    • milestone: --> fixed
    • assigned_to: nobody --> maartenbrock
    • status: open --> closed-accepted
     
  • Maarten Brock
    Maarten Brock
    2008-03-04

    Logged In: YES
    user_id=888171
    Originator: NO

    Robert,

    Thanks for the patch! I have applied it in SDCC 2.7.5 #5069.

    > z80 doesn't check for
    > !IS_CAST_ICODE(OP_SYMBOL (IC_RIGHT (ic))->rematiCode) &&
    > is this by accident, or intentionally ?

    I have a strong feeling that the patch made by Sandeep in revision #1508 of src/mcs51/ralloc.c never made it into the z80 target. So I added the extra check there too now.

    Maarten