#2188 Function miscompiled when inline selected (Regression)

open
Erik Petrich
None
Front-end
5
2014-03-28
2013-06-16
Pavel Pisa
No

When attached code fragment is compiled ul_gavlchk.prep.c by SDCC 3.3.1 SVN 130616 then next warning is reported

ul_gavlchk.prep.c:85: warning 127: non-pointer type cast to generic pointer
from type 'int fixed'
to type 'const-int generic* fixed'

and code behaves incorrectly.

If inline keyword is removed from next function definition, then warning is not reported and function of is correct.

static inline cust2_key_t *
cust2_node2key (const cust2_root_t * root, gavl_node_t * node)
{
return &(cust2_node2item (root, node)->my_val);
}

Code worked well with some older SDCC versions (3.1, may be 3.2).

Code is part of the uLUt project

https://sourceforge.net/p/ulan/ulut/ci/master/tree/ulut/

but it is heavily simplified and complex macro expansion is applied before test case submitting.

Code is expansion of macros from
ul_gavl.h
ul_gavlcust.h
in file ul_gavlchk.c by
GAVL_CUST_NODE_INT_DEC(cust2, cust2_root_t, cust2_item_t, cust2_key_t,
my_root, my_node, my_val, cust2_cmp_fnc)
GAVL_CUST_NODE_INT_IMP(cust2, cust2_root_t, cust2_item_t, cust2_key_t,
my_root, my_node, my_val, cust2_cmp_fnc)

Probably same or similar error causes miscompilation of ul_gsachk.c as well.

1 Attachments

Related

Bugs: #1360

Discussion

  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    I have invest time to git bisecting through 3.1 to 3.3 SDCC sources

    I have found that introduction of the warning does not correspond to the bug source. Warning appears at the commit

    722f3d5ac2a292f291a9e2070069d2c4f5914b47 is the first commit with warning
    commit 722f3d5ac2a292f291a9e2070069d2c4f5914b47
    Author: epetrich <epetrich@4a8a32a2-be11-0410-ad9d-d568d2c75423>
    Date:   Sun Feb 19 07:01:11 2012 +0000
    
    * src/SDCCsymt.h,
      src/SDCCsymt.c (findSymWithLevel),
      src/SDCCast.c (resolveSymbols, processBlockVars): fixed bugs 3403429,
      3153215, 3107914
    * support/regression/tests/bug3403429.c: enabled test
    
    git-svn-id: svn://svn.code.sf.net/p/sdcc/code/trunk@7331 4a8a32a2-be11-0410-ad9d-d568d2c75423
    

    :040000 040000 79a605f40e64fe6105ae5332c47957e22a8ef94d 09d5291c05f15be22a59e22733c5146bc0516610 M sdcc

    I was not able to find exact commit responsible for runtime misbehavior of compiled code, because sequence was not bisect-able (s51 ends with signal or empty output for uLUt project). But problem has been introduced by one of next commits

    commit bb3a13c3173c89d827b4de4c2520e7a61f8a7078
    Author: spth <spth@4a8a32a2-be11-0410-ad9d-d568d2c75423>
    Date:   Sat Oct 13 06:44:06 2012 +0000
    
    Fixed bug #3566360.
    
    git-svn-id: svn://svn.code.sf.net/p/sdcc/code/trunk@8145 4a8a32a2-be11-0410-ad9d-d568d2c75423
    
    commit e102e5cfff52248ddfe720978cad67dc868e8e1e
    Author: borutr <borutr@4a8a32a2-be11-0410-ad9d-d568d2c75423>
    Date:   Mon Oct 8 09:26:07 2012 +0000
    
    * support/sdbinutils/include/opcode/ia64.h: added since build
      on IA64 fails with elf64-ia64.c:27:25: error: opcode/ia64.h:
      No such file or directory
    
    git-svn-id: svn://svn.code.sf.net/p/sdcc/code/trunk@8144 4a8a32a2-be11-0410-ad9d-d568d2c75423
    
    commit 7ade9d6e079fd109ed38a3959eafd50631e1ca9a
    Author: epetrich <epetrich@4a8a32a2-be11-0410-ad9d-d568d2c75423>
    Date:   Mon Oct 8 08:09:48 2012 +0000
    
    * src/mcs51/gen.c (genPagedPointerGet): fixed bug #3527222
    * support/regression/tests/gcc-torture-execute-921013-1.c: reenabled
    * src/mcs51/gen.c (genCast),
    * src/ds390/gen.c (genCast): fixed bug3467508 regression test failures
    * support/regression/tests/bug3467508.c: reenabled for mcs51 and ds390
    
    git-svn-id: svn://svn.code.sf.net/p/sdcc/code/trunk@8143 4a8a32a2-be11-0410-ad9d-d568d2c75423
    
    commit 1f1947f73588654d25f746210dbb630d0f6365e3
    Author: MaartenBrock <MaartenBrock@4a8a32a2-be11-0410-ad9d-d568d2c75423>
    Date:   Mon Oct 8 00:52:07 2012 +0000
    
    * sdas/asgb/asgb.vcxproj,
    * sdas/asgb/asgb.vcxproj.filters,
    * sdas/asgb/Makefile.in,
    * sdas/asgb/gbext.c: removed gbext.c
    * sdas/as8051/i51mch.c,
    * sdas/as8051/i8051.h,
    * sdas/asgb/gbadr.c,
    * sdas/asgb/gbmch.c,
    * sdas/asrab/rabadr.c,
    * sdas/asz80/z80adr.c,
    * sdas/asxxsrc/asdata.c,
    * sdas/asxxsrc/aslist.c,
    * sdas/asxxsrc/asout.c,
    * sdas/asxxsrc/asxxxx.h: and yet another step towards ASxxxx v5
    
    git-svn-id: svn://svn.code.sf.net/p/sdcc/code/trunk@8142 4a8a32a2-be11-0410-ad9d-d568d2c75423
    

    I have captured some dumps of project compilation before and after breakage which I can provide on request. But it is about 14 MB of garbage.

    On the other hand diff is quite minimal. Except many changes in relocation
    type

        0000                     489 __interrupt_vect:
    -   0000 02s00r00      [24]  490        ljmp    __sdcc_gsinit_startup
    +   0000 02r00r00      [24]  490        ljmp    __sdcc_gsinit_startup
    

    there is only minimal change in affected code

    diff -u -p -r -x CVS --minimal _build/ulut/ul_gavlchk.asm /home/pi/repo/ulan/ulut-test/_tests/bad-1/_build/ulut/ul_gavlchk.asm
    --- _build/ulut/ul_gavlchk.asm  2014-01-09 13:24:40.741813670 +0100
    +++ /home/pi/repo/ulan/ulut-test/_tests/bad-1/_build/ulut/ul_gavlchk.asm        2014-01-09 13:27:02.614517179 +0100
    @@ -1,7 +1,7 @@
     ;--------------------------------------------------------
     ; File Created by SDCC : free open source ANSI-C Compiler
     ; Version 3.2.1 # (Jan  9 2014) (Linux)
    -; This file was generated Thu Jan  9 13:24:40 2014
    +; This file was generated Thu Jan  9 13:27:02 2014
     ;--------------------------------------------------------
            .module ul_gavlchk
            .optsdcc -mmcs51 --model-large
    @@ -2483,7 +2483,10 @@ _cust2_search_node:
            inc     dptr
            movx    a,@dptr
            mov     (_cust2_search_node_sloc0_1_0 + 2),a
    -       mov     r4,#0x00
    +       mov     a,r3
    +       rlc     a
    +       subb    a,acc
    +       mov     r4,a
            mov     dptr,#_cust2_cmp_fnc_PARM_2
            mov     a,_cust2_search_node_sloc0_1_0
            movx    @dptr,a
    

    and

    diff -u -p -r -x CVS --minimal _build/ulut/ul_gsachk.asm /home/pi/repo/ulan/ulut-test/_tests/bad-1/_build/ulut/ul_gsachk.asm
    --- _build/ulut/ul_gsachk.asm   2014-01-09 13:24:40.213811052 +0100
    +++ /home/pi/repo/ulan/ulut-test/_tests/bad-1/_build/ulut/ul_gsachk.asm 2014-01-09 13:27:02.094514600 +0100
    @@ -1,7 +1,7 @@
     ;--------------------------------------------------------
     ; File Created by SDCC : free open source ANSI-C Compiler
     ; Version 3.2.1 # (Jan  9 2014) (Linux)
    -; This file was generated Thu Jan  9 13:24:40 2014
    +; This file was generated Thu Jan  9 13:27:02 2014
     ;--------------------------------------------------------
            .module ul_gsachk
            .optsdcc -mmcs51 --model-large
    @@ -3963,7 +3963,10 @@ _cust2_bsearch_indx:
            inc     dptr
            movx    a,@dptr
            mov     (_cust2_bsearch_indx_sloc0_1_0 + 2),a
    -       mov     r4,#0x00
    +       mov     a,r1
    +       rlc     a
    +       subb    a,acc
    +       mov     r4,a
            mov     dptr,#_cust2_cmp_fnc_PARM_2
            mov     a,_cust2_bsearch_indx_sloc0_1_0
            movx    @dptr,a
    @@ -4245,7 +4248,10 @@ _cust2_bsearch_indx:
            inc     dptr
            movx    a,@dptr
            mov     r2,a
    -       mov     r7,#0x00
    +       mov     a,r4
    +       rlc     a
    +       subb    a,acc
    +       mov     r7,a
            mov     dptr,#_cust2_cmp_fnc_PARM_2
            mov     a,r0
            movx    @dptr,a
    @@ -4873,7 +4879,10 @@ _cust2_delete:
            inc     dptr
            movx    a,@dptr
            mov     r6,a
    -       mov     r7,#0x00
    +       movx    a,@dptr
    +       rlc     a
    +       subb    a,acc
    +       mov     r7,a
            mov     dptr,#_cust2_cmp_fnc_PARM_2
            mov     a,r2
            movx    @dptr,a
    

    Diffs are from whole uLUt project test build. May be I find nimimized version but for attached fragment but it I it does not contain runtime output so checking is more problematic.

     
  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    So even difference for shortened example is the same

    diff -u -b -B ul_gavlchk.rel-good ul_gavlchk.rel-bad
    --- ul_gavlchk.rel-good 2014-01-09 15:25:14.365683274 +0100
    +++ ul_gavlchk.rel-bad  2014-01-09 15:18:00.459531650 +0100
    @@ -1,7 +1,7 @@
     ;--------------------------------------------------------
     ; File Created by SDCC : free open source ANSI-C Compiler
     ; Version 3.2.1 # (Jan  9 2014) (Linux)
    -; This file was generated Thu Jan  9 15:24:42 2014
    +; This file was generated Thu Jan  9 15:18:00 2014
     ;--------------------------------------------------------
    
    ;   ul_gavlchk.prep.c:49: return (cust2_item_t *) ((char *) node -
        mov ar2,r5
        mov ar3,r6
        mov ar4,r7
    ;   ul_gavlchk.prep.c:50: (long) &((cust2_item_t *) 0)->my_node);
        mov a,r2
        add a,#0xFE
        mov r2,a
        mov a,r3
        addc    a,#0xFF
        mov r3,a
    ;   ul_gavlchk.prep.c:85: cmp = cust2_cmp_fnc (cust2_node2key (root, n), key);
        mov dptr,#_cust2_search_node_PARM_2
        movx    a,@dptr
        mov _cust2_search_node_sloc0_1_0,a
        inc dptr
        movx    a,@dptr
        mov (_cust2_search_node_sloc0_1_0 + 1),a
        inc dptr
        movx    a,@dptr
        mov (_cust2_search_node_sloc0_1_0 + 2),a
    -   mov r4,#0x00
    +   mov a,r3
    +   rlc a
    +   subb    a,acc
    +   mov     r4,a
        mov dptr,#_cust2_cmp_fnc_PARM_2
        mov a,_cust2_search_node_sloc0_1_0
        movx    @dptr,a
        mov a,(_cust2_search_node_sloc0_1_0 + 1)
        inc dptr
        movx    @dptr,a
        mov a,(_cust2_search_node_sloc0_1_0 + 2)
        inc dptr
        movx    @dptr,a
        mov dpl,r2
        mov dph,r3
        mov b,r4
        push    ar7
        push    ar6
        push    ar5
        lcall   _cust2_cmp_fnc
    

    Value of r4 is used later as memory type for __gptrget in the _cust2_cmp_fnc function.

    It looks like both versions of the code are incorrect but older works
    because memory access type in r4 is set to #0x00 (i.e. xdata) unconditionally (which is the case for test). The correct memory access type is set by initial "mov ar4,r7" which copies type from the input "node" parameter.

    The actual version is total nonsense because it takes bits 8 to 15 of the computed address (i.e. DPH) and does rlc and puts them into memory access type.

    The current SVN version generates exactly same bad code except one ninor optimization of constant load in different part of the code.

     
  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    It seems that problem is related to incorrect result type of operation

    generic pointer - constant

    or lost of the pointer type in return from inline function.

    Anyway, the core of the problem is that the value returned by cust2_node2item() is casted from incorrectly deducted int type to pointer with sign extension. cust2_node2key() should be no-op, it does not modify address, because "my_val" is at the beginning of the structure cust2_item.

    cmp = cust2_cmp_fnc (cust2_node2key (root, n), key);
    

    My code uses hand-coded offsetof .. "(long) &((cust2_item_t *) 0)->my_node" which is the source of that "constant" value, but even use of __builtin_offsetof (cust2_item_t, my_node)) results in same code.

    I have tried to debug type decoration for AST minus operation.

    Breakpoint 4, decorateType (resultType=RESULT_TYPE_GPTR, tree=<optimized out>) at ../../sdcc/sdcc/src/SDCCast.c:3969
    3969      ast_print (tree, stdout, 0x00);
    (gdb) n
    ul_gavlchk.prep.c:50:SUB (0xa2f2d0) type (void)
    ul_gavlchk.prep.c:49:  CAST (0xa2e450) from type (const-struct gavl_node generic* xdata) to type (char generic* fixed)
    ul_gavlchk.prep.c:49:    SYMBOL (L1 B1 node=0xa2e310 @ 0xa2d430) type (const-struct gavl_node generic* xdata)
    ul_gavlchk.prep.c:50:  CAST (0xa2f230) from type (struct gavl_node generic* fixed) to type (long-int fixed)
    ul_gavlchk.prep.c:50:    ADDRESS_OF (0xa2f0f0) type (struct gavl_node generic* fixed)
    ul_gavlchk.prep.c:50:      PTR_ACCESS (0xa2f050) type (struct gavl_node fixed)
    ul_gavlchk.prep.c:50:        CONSTANT (0xa2e970) value = 0, 0x0, 0.000000 type (struct cust2_item generic* literal)
    ul_gavlchk.prep.c:50:        SYMBOL (L1 B2 my_node=0xa2efb0 @ 0xa2ec60)
    3973          if ((IS_ARRAY (LTYPE (tree)) || IS_PTR (LTYPE (tree))) && (IS_ARRAY (RTYPE (tree)) || IS_PTR (RTYPE (tree))))
    

    But resulting tree has proper type

    ul_gavlchk.prep.c:50:SUB (0xa815f0) type (char generic* fixed)
    

    so I am not sure why type is lost somewhere (may it be in inline return) and then incorrect and unnecessary cast is inserted into output

    ;   genCast
        mov a,r3
        rlc a
        subb    a,acc
        mov r4,a
    
     
  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    I have prepared much simpler test case which shows same misbehavior. It seems to be really problem of inline function return type. The same warning

    test-inline-ret-type.c:17: warning 127: non-pointer type cast to generic pointer
    from type 'int fixed'
      to type 'int generic* fixed'
    

    and same hurting cast

    ;   genAssign
    ;   test-inline-ret-type.c:17: {
    ;   genCast
        mov a,r6
        rlc a
        subb    a,acc
        mov r7,a
    ;   genCall
    
     
  • Pavel Pisa
    Pavel Pisa
    2014-01-10

    Yet another part of the puzzle. The problem is really caused by inappropriate change of type of the temporary variable used to store inline function result. expandInlineFuncs creates symbol to hold result.

          if (!IS_VOID (func->type->next))
            {
              /* ...
              /* {{inline_function_code}}, retsym                         */
    
              retsym = inlineTempVar (func->type->next, block->level);
              SPEC_SCLS (retsym->etype) = S_FIXED;
              inlineAddDecl (retsym, block, TRUE, TRUE);
            }
    

    This symbol has correct type. Then createFunction() calls decorateType(). This finds CALL operation to non inlined function and starts to process ints parameters processParms(). It recurses with use resolveSymbols() which finaly finds temporary variable used to pass inline result to function parameters.

    But function resolveSymbols() does not find symbol in the symbol table and marks its type as undefined

    /* do only for leafs */
    if (IS_AST_VALUE (tree) && tree->opval.val->sym && !tree->opval.val->sym->implicit)
    
    ...
    
      /* if not found in the symbol table */
      /* mark it as undefined & assume it */
      /* is an integer in data space      */
      if (!csym && !tree->opval.val->sym->implicit)
    
      ...
    
              tree->opval.val->sym->undefined = 1;
              tree->opval.val->type = tree->opval.val->etype = newIntLink ();
              tree->opval.val->sym->type = tree->opval.val->sym->etype = newIntLink ();
    

    Actual call-trase of the sequence leading to the problem is there

    0x000000000042b11a in resolveSymbols (tree=0xa29f50) at ../../sdcc/sdcc/src/SDCCast.c:641
    641                   tree->opval.val->sym->type = tree->opval.val->sym->etype = newIntLink ();
    (gdb) i s
    #0  0x000000000042b11a in resolveSymbols (tree=0xa29f50) at ../../sdcc/sdcc/src/SDCCast.c:641
    #1  0x000000000042ad09 in resolveSymbols (tree=0xa29ff0) at ../../sdcc/sdcc/src/SDCCast.c:669
    #2  0x000000000042ad09 in resolveSymbols (tree=0xa2bbe0) at ../../sdcc/sdcc/src/SDCCast.c:669
    #3  0x000000000042ad15 in resolveSymbols (tree=0xa2bb40) at ../../sdcc/sdcc/src/SDCCast.c:670
    #4  0x000000000042ad09 in resolveSymbols (tree=0xa2c990) at ../../sdcc/sdcc/src/SDCCast.c:669
    #5  0x000000000042ad15 in resolveSymbols (tree=0xa2dc50) at ../../sdcc/sdcc/src/SDCCast.c:670
    #6  0x000000000042ae18 in resolveSymbols (tree=0xa2ca30) at ../../sdcc/sdcc/src/SDCCast.c:658
    #7  0x000000000042ad15 in resolveSymbols (tree=0xa2d4f0) at ../../sdcc/sdcc/src/SDCCast.c:670
    #8  0x000000000042ae18 in resolveSymbols (tree=0xa2cf30) at ../../sdcc/sdcc/src/SDCCast.c:658
    #9  0x000000000042ad09 in resolveSymbols (tree=0xa2b430) at ../../sdcc/sdcc/src/SDCCast.c:669
    #10 0x00000000004382e2 in processParms (func=0xa2abb0, defParm=0xa262c0, actParm=actParm@entry=0xa2b550,
        parmNumber=parmNumber@entry=0x7fffffff3f44, rightmost=rightmost@entry=true) at ../../sdcc/sdcc/src/SDCCast.c:961
    #11 0x0000000000431289 in decorateType (resultType=<optimized out>, tree=<optimized out>)
        at ../../sdcc/sdcc/src/SDCCast.c:5124
    #12 decorateType (tree=0xa2b4d0, resultType=<optimized out>) at ../../sdcc/sdcc/src/SDCCast.c:2917
    #13 0x000000000042e3b3 in decorateType (resultType=RESULT_TYPE_NONE, tree=<optimized out>)
        at ../../sdcc/sdcc/src/SDCCast.c:3074
    #14 decorateType (tree=0xa2b570, resultType=RESULT_TYPE_NONE) at ../../sdcc/sdcc/src/SDCCast.c:2917
    #15 0x000000000042e3b3 in decorateType (resultType=RESULT_TYPE_NONE, tree=<optimized out>)
        at ../../sdcc/sdcc/src/SDCCast.c:3074
    #16 decorateType (tree=0xa2b610, resultType=RESULT_TYPE_NONE) at ../../sdcc/sdcc/src/SDCCast.c:2917
    #17 0x000000000043c768 in createFunction (name=0xa29b80, body=<optimized out>) at ../../sdcc/sdcc/src/SDCCast.c:6996
    #18 0x000000000040dbcc in yyparse () at ../../sdcc/sdcc/src/SDCC.y:202
    #19 0x00000000004093d5 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
        at ../../sdcc/sdcc/src/SDCCmain.c:2544
    
     
  • Erik Petrich
    Erik Petrich
    2014-01-10

    I think you have narrowed down the bug to a point where it should be easily fixable. This is all pointing at a problem of scope visibility. The temporary return symbol is not resolving and the warning appears starting from my 19 Feb 2012 commit. That commit fixed several problems with variables remaining visible when they should be out of scope, so I suspect that the temporary return symbol is being created in such a way that it fails the stricter scope tests that are now in place. Thanks for all your hard work debugging today!

     
  • Pavel Pisa
    Pavel Pisa
    2014-01-10

    I have found reason why symbol is not found. Problem is in the function

    SDCCsymt.c:findSymWithLevel (bucket ** stab, symbol * sym)

    The test fails because symbols (bp->sym))->isinscope is False for variable introduced by inlineTempVar when it is reached by resolveSymbols called from decorateType.

          if (bp->level && bp->level != sym->level && bp->block <= sym->block
              && ((symbol *) (bp->sym))->isinscope
              && (stab == LabelTab || ((symbol *)(bp->sym))->seqPoint <= sym->seqPoint))
            return (bp->sym);
    

    symbol type seems to be correct at that point

    (gdb) p printTypeChainRaw(((symbol *)(bp->sym))->type, 0)
    generic*  int
    

    The original matched symbol is still OK at

    (gdb) p printTypeChainRaw(tree->opval.val->sym->type, 0)
    generic*  int
    

    SDCCast.c:649 in resolveSymbols ()

    if (tree->type == EX_OP && tree->opval.op == BLOCK && tree->values.sym)
    

    and then its type is turned to int

    639    tree->opval.val->sym->undefined = 1;
    640    tree->opval.val->type = tree->opval.val->etype = newIntLink ();
    641    tree->opval.val->sym->type = tree->opval.val->sym->etype =
    
    (gdb) p printTypeChainRaw(tree->opval.val->sym->type, 0)
    int
    
     
  • Erik Petrich
    Erik Petrich
    2014-01-11

    The problematic flow is from createFunction() to decorateType() to processParms() to resolveSymbols(), but this call to resolveSymbols() is going directly to an inner block level and so not passing through the block that would activate the isinscope flag of the temporary variable. createFunction() has already called resolveSymbols() with the root of the function tree, so all of the symbols should already be resolved.

    Changing resolveSymbols() to ignore symbols that have already been resolved seems to fix this bug (at least test-inline-ret-type.c) without causing any regressions, but perhaps some of the resolveSymbols() calls could be removed too and/or instead.

     
  • Erik Petrich
    Erik Petrich
    2014-01-11

    • assigned_to: Erik Petrich
     
  • Pavel Pisa
    Pavel Pisa
    2014-03-28

    Tested with 3.4.0 prerelease

    DCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8 3.4.0 # (Mar 28 2014) (Linux)
    published under GNU General Public License (GPL)

    and SDCC still broken

    /home/pi/repo/ulan/ulut-test/ulut/ul_gsachk.c:53: warning 127: non-pointer type cast to generic pointer
    from type 'int fixed'
    to type 'const-int generic fixed'
    /home/pi/repo/ulan/ulut-test/ulut/ul_gsachk.c:53: warning 127: non-pointer type cast to generic pointer
    from type 'int fixed'
    to type 'const-int generic
    fixed'
    /home/pi/repo/ulan/ulut-test/ulut/ul_gsachk.c:53: warning 127: non-pointer type cast to generic pointer
    from type 'int fixed'
    to type 'const-int generic* fixed'

    I understand that time is a problem. I have big problem to invest time to this problem and if there is no resolution by core developers where should be problem solved than it would be only waste of my time.

    But it would be honest to inform users in 3.4.0 release notes that "inline" is seriously broken and who uses inline should stay with pre 19 Feb 2012, i.e. 3.1.0 version.