Menu
▾
▴
#1360 Parameters lost from scope of intermediate inline function
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
Source demonstarting problem
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.
001 REQ Process inline function expansion even in block variables declarations.
002 MAYBE Move temporary inline return value storage in front of declarators.
003 DEBUG Support for better AST tree debug print.
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.
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
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
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.
0004 REQ Implemented missing incorrect parts of inline expansion
0005 REQ Code for update/expansion of FOR statemetnt.
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.
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
Pavel's patches applied in svn revision #5646.
Borut
Even though patch 003 was applied, what was the thought behind the following?
This ignores undecorated branches only iff indentation is not too deep. So with deep indentation an undecorated branch is still printed.
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.
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:
#2188Thanks 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