#1360 Parameters lost from scope of intermediate inline function

closed-fixed
Borut Ražem
7
2014-01-09
2007-08-05
Pavel Pisa
No

If inline function calls another inline function,
the calling inline function does not see its parameters.
If there is global variable of same name, it hides
problem.

It seems, that same problem affects even local variables
declared before function call.

SDCC : mcs51/gbz80/z80/avr/ds390/pic16/pic14/TININative/xa51/ds400/hc08 2.7.3 #4892 (Aug 5 2007)

sdcc --std-c99 --debug --dumpall -mmcs51 --model-large -o inline-test-1.rel -c inline-test-1.c

Discussion

  • Pavel Pisa
    Pavel Pisa
    2007-08-05

    Source demonstarting problem

     
    Attachments
  • Erik Petrich
    Erik Petrich
    2007-08-07

    • assigned_to: nobody --> epetrich
     
  • Pavel Pisa
    Pavel Pisa
    2008-08-28

    Logged In: YES
    user_id=523128
    Originator: YES

    The behavior changed after close of #1864577 to report next error

    inline-test-1.c:9: error 20: Undefined identifier 'x'

    The change reverts previous changes introducing inlineFindMaxBlockno to solve #1731741.
    May it be, that blockno has to be increased to not lose scope of parameters
    some other way still.

     
  • Pavel Pisa
    Pavel Pisa
    2009-08-09

    • priority: 5 --> 7
    • status: open --> open-works-for-me
     
  • Pavel Pisa
    Pavel Pisa
    2009-08-09

    The proposed series solves inline functions in initializers processing

    0001-Process-inline-function-expansion-even-in-block-vari.patch
    - This is required the functions in initializers has to be expanded

    0002-Move-temporary-inline-return-value-storage-in-front.patch
    - Move temporary inline return value storage in front of declarators.
    This may be needed for correct functionality of expansion
    of inline functions block variable declarations and default
    values assignment.

    0003-Support-for-better-AST-tree-debug-print.patch
    - this patch has been used to help debugging

    The result has not been tested much, but patched SDCC compiles all support libraries,
    Actual mcs51 port version of uLan http://ulan.sourceforge.net/ project and even GSA
    and GAVL code from uLUt library. Basic tests works in s51 simulator. This is big leap
    forward. If SDCC can be fully used with inlines and uLUt we can get rid of nasty macro
    machinery required for mcs51 support and this cleans up project in whole.

     
  • Martin Leopold
    Martin Leopold
    2009-09-30

    Hi All,
    It was suggested to me to try this patch sine the TinyOS 8051 project is very much in need of an 8051 compiler that does inlining. I've taken the patch out for a spin and not sure what to say - it doesn't compile my code right out of the box. But then again the auto-generated code that I'm trying to compile is a mess.

    Here's what I did:

    * I took the patches and applied them to the current SVN they applied cleanly and I compiled SDCC with no hickups. I haven't read or attempted to understand the patches.

    * I took two simple examples for TinyOS and ran them through the entire TinyOS toolchain which ends up with an app.c file with a bunch of very bizarre inline functions.

    * I fed this app.c through the newly compiled SDCC.

    What I see is the following:
    Both programs are compiled from C to asm by SDCC, but the assembly step of both programs fail with a whole lot of errors like the one below. If I drop the "inline" keyword (which is what I normally do) both compile
    and work just fine.

    app.asm:1548: Error: <u> undefined symbol encountered during assembly
    ...

    I've attached both applications for your amusement.

    Best regards,
    Martin Leopold

     
  • Pavel Pisa
    Pavel Pisa
    2009-11-08

    Hello Martin and others,

    I have found a while to test patches and SDCC again a little.
    The result from my testing is, that your bug is different one then one I have tried to solve.
    The switch statement is problem in your case. The function inlining does not create labels
    correctly. It would worth to be fixed, but I am not sure if/when I find time for that.

    If the switch statement is commented out (in echo.c:SchedulerBasicP__TaskBasic__runTask
    and blinknotimertask.c:SchedulerBasicP__TaskBasic__runTask functions) then both
    source files compiles correctly to object files. The linking reports unresolved symbols
    ___nesc_atomic_start and ___nesc_atomic_end, which is expected, I do not have TiniOS
    environment there.

    It would worth to try, if the code really works as expected on real hardware/in simulator when
    inline attribute is removed from functions with switch statements.

    I have run testing of build of our uLUt library core (GSA and GAVL) with patched actual SDCC
    version and it works correctly. I have incorporated SDCC compatibility changes into uLUt GIT
    repository.
    http://ulan.git.sourceforge.net/git/gitweb.cgi?p=ulan/ulut;a=summary
    git://ulan.git.sourceforge.net/gitroot/ulan/ulut
    There are some obstacles to build whole library and special sdcc rules version has to be
    but most of the other functionalities do not make big sense for so small microcontrollers
    and even many of them can be corrected (use memcpy instead of aggregate assignment,
    reentrant attribute etc).

    So generally current SDCC with provided patches is much more suitable for our use.
    It is run for long distance, the my first inline corrections are many years old, but there
    is some progress.

    Best wishes,

    Pavel

     
  • Pavel Pisa
    Pavel Pisa
    2009-11-17

    Implemented and corrected missing/broken parts of inline functions expansion for switch statements.
    This should correct build of generated TinyOS code sent by Martin. Patch attached

    0004-Implemented-missing-incorrect-parts-of-inline-expans.patch

    Please, test the code and please, do not let this effort to be only waste of time.
    Try to check proposed changes. They are not extremely releasable code,
    but series corrects real bugs and problems best and least intrusive
    way, I have been able to find in SDCC framework. To make it really
    clean means to change SDCC AST and parser significantly.

     
  • Martin Leopold
    Martin Leopold
    2009-11-19

    Hi Pavel.
    Great job! This seems to take a big step towards compiling my code! While you mention that your code might not be mature enough to be included in sdcc, I'll take this chance to encourage the authors to work on the ideas that you propose!

    Here is what I did: I was unable to compile current sdcc-svn with the patches. So I applied the patches the current release 2.9.0 and it compiled cleanly.

    I used the patched sdcc to try 3 test apps: echo, blinknotimertask and compressiontest. Blinknotimertask and compressiontest compile cleanly, but only blinknotimertask seem to work. Echo does not compile for me (with inline enabled) and compressiontest results in a non-responsive device for me. I cannot attach files to this bugreport, so I'll mail you the sources directly. Arguments to sdcc are:
    -mmcs51 --std-sdcc99 --model-large --no-c-code-in-asm --out-fmt-ihx --main-return --xram-loc 0xE000 --xram-size 0x1F00

    Echo
    Compiling "Echo" with inlining disabled seems to work. When enabling inlining it fails with the following error:
    ?ASlink-Warning-Undefined Global '___vector_2___nesc_atomic_14_68' referenced by module 'app'

    BlinkNoTimerTask
    I noticed something that puzzled me: while inlining should logically increase compiled size, the size nearly exactly doubled for to two apps that compile using inlining. This seems relatively strange and if I run the sources through my CIL based C-to-C based inline tool the code size is roughly the same.

    Here is the breakdown for BlinkNoTimerTask (a very, very simple app):
    BlinkNoTimerTask (inlining disabled)
    Name Start End Size Max
    ---------------- -------- -------- -------- --------
    PAGED EXT. RAM 0 256
    EXTERNAL RAM 0xe000 0xe00e 15 7936
    ROM/EPROM/FLASH 0x0000 0x03ba 955 65536

    BlinkNoTimerTask (inlining enabled)
    SDCC+ inline
    Name Start End Size Max
    ---------------- -------- -------- -------- --------
    PAGED EXT. RAM 0 256
    EXTERNAL RAM 0xe000 0xe024 37 7936
    ROM/EPROM/FLASH 0x0000 0x06db 1756 65536

    Phew, I hope you can make some sense of that, if you have any questions or suggestions, don't hesitate to contact me. The c and asm source are on their way by mail.

    Best regards,
    Martin Leopold.

     
  • Pavel Pisa
    Pavel Pisa
    2009-11-19

    Hello Martin,

    I have used Tiny-OS 2.1 Debian packages from tiny.net for base
    and TinyOS8051wg-0.1pre4.tgz 26 Oct 2008 for mcs51 support
    and applications.

    With proposed changes to mangleAppC.pl, I have been able
    to compile even Echo, but none of apps seems to be responsive
    under s51 emulator. I have luck to compile most of applications
    but TTXDemo and some other complex one failed on some internal
    compiler error. Many of applications use FOR statement, so require
    the fifth patch in series

    0005-Added-code-to-update-for-statement-during-inline-fun.patch

    As for SDCC version, I have used and patched actual SVN version,
    rev 5568 for this round of tests. So patches should apply to actual SVN
    cleanly.

    As for applications size, the TinyOS use of inline could be classified
    more as abuse. In GCC case, GCC decides, what worth to be
    inlined and what not when inline keyword is used. SDCC belives
    to programmer and if some function is called twice or even twice
    from functions which are again called twice code size and even variables
    count could grow exponentially. So this could be a problem. CSE optimization
    could sometimes for more complex function (i.e. formed by multiple inline parts)
    go crazy. On the other hand, inline is great for same functions simplification
    and making more functions into call graph leaf node could help with more
    optimal use of memory due to overlay enable rules for parameters, local
    variables and CSE temporaries.

    Best wishes,

    Pavel

     
  • I had a look at patches 1,3,4 and 5. They seem OK to me.

    On the other hand these generic parts of sdcc are much less familiar to me than the Z80-specific ones, so I could have missed something.

    Philipp

     
  • Borut Ražem
    Borut Ražem
    2010-01-23

    Pavel's patches applied in svn revision #5646.

    Borut

     
  • Borut Ražem
    Borut Ražem
    2010-01-23

    • milestone: --> fixed
    • assigned_to: epetrich --> borutr
    • status: open-works-for-me --> closed-fixed
     
  • Maarten Brock
    Maarten Brock
    2014-01-08

    Even though patch 003 was applied, what was the thought behind the following?

    if (!tree->decorated && !(indent & 0x100)) return;
    

    This ignores undecorated branches only iff indentation is not too deep. So with deep indentation an undecorated branch is still printed.

     
  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    I have used that during my fixing of SDCC inline support. I do not remember exactly where I have used option to print AST even before decoration. May it be, I have called ast_print(x,y,0x100) directly from GDB or I have some debug modification of the code.

    Anyway, it seems to be leftover from inline fixing. So !(indent & 0x100) can be removed or replaced with some other way to print undecorated tree.

    But what is more important is, that SDCC inline support is broken for many revisions now. But I have not found time to simplify test case to be in publishable state and whole uLUt UL_GAVL/UL_GSA is so complex that it has no reason to report these errors. Other option is to run git bisect on the sources but that is time consuming as well. Problem is that code compiles and bug only causes runtime misbehavior/bad tree data manipulations.

    There were many revisions when inline worked correctly with uLUt test but practical usability is zero, because code length of the example is so big that actual application using this would exceed memory available on all 8051 chips.

    Other problem is that I have no real SDCC MCU target usable now and we do not plan any 8051 based stuff in future - all our systems are Cortex-M/A, PowerPC or MSP430 . My colleague works with PICs but only in ASM and SDCC compiled equivalent C code would probably exceed available memory of these apps.

    So even that I follow SDCC development occasionally, I am inactive and do not plan its real use. One possible exception is CC2540 if there is some chance for open source or stubs for binary BLE stack.

    If I find time, I try to create new bug report with some simplified inline breakage test case. But I do not promise that.

     
  • Pavel Pisa
    Pavel Pisa
    2014-01-09

    In the fact I have found that I have already reported the problem under

    [bugs:#2188] Function miscompiled when inline selected (Regression)

    But the test case is still so complex that it is not usable.

     

    Related

    Bugs: #2188

  • Maarten Brock
    Maarten Brock
    2014-01-09

    Thanks Pavel,

    I only wanted to know why the indent & 0x100 was added, because it seems wrong to me. I will remove it.

    Maybe sometime I will also get around to the other bug ;)

    Maarten