Menu
▾
▴
[sdcc-devel] [ sdcc-Bugs-3535290 ] mcs51 long switch/case blocks cause jmp into oblivion
|
From: SourceForge.net <no...@so...> - 2012-11-17 06:51:02
|
Bugs item #3535290, was opened at 2012-06-14 13:43 Message generated for change (Comment added) made by epetrich You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=3535290&group_id=599 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. >Category: mcs51(8051) target Group: non bugs >Status: Closed >Resolution: Fixed Priority: 5 Private: No Submitted By: Kamikaze Dominic Fandrey (lonkamikaze) Assigned to: Erik Petrich (epetrich) Summary: mcs51 long switch/case blocks cause jmp into oblivion Initial Comment: I have the following snippet of code at the end of several switch/case blocks: #ifdef SDCC /* * This is here for what appears to be a bug in SDCC. * I suspect a problem with the jump table optimisation for switch * statements, the disparate case number will result in less optimised * code. * Without this workaround multiple calls to the function lock up the * controller. The first call always works. As far as I've been able to * determine the end of the function is reached, but the line after * the second function call never is. */ case 0xFF: RESET_RMAP(); break; #endif It reads a little weird. My details: > sdcc -v SDCC : mcs51/gbz80/z80/z180/r2k/ds390/pic16/pic14/TININative/ds400/hc08 3.1.0 #7066 (Feb 12 2012) (FreeBSD) Compile: sdcc -mmcs51 --xram-loc 0xF000 --xram-size 3072 -I inc/ -o bin.sdcc/hsk_flash/hsk_flash.rel -c src/hsk_flash/hsk_flash.c [...] Link: sdcc -mmcs51 --xram-loc 0xF000 --xram-size 3072 -I inc/ -o bin.sdcc/main.hex bin.sdcc/hsk_timers/hsk_timer01.rel bin.sdcc/hsk_icm7228/hsk_icm7228.rel bin.sdcc/hsk_pwc/hsk_pwc.rel bin.sdcc/hsk_pwm/hsk_pwm.rel bin.sdcc/hsk_adc/hsk_adc.rel bin.sdcc/hsk_can/hsk_can.rel bin.sdcc/hsk_boot/hsk_boot.rel bin.sdcc/main.rel bin.sdcc/hsk_isr/hsk_isr.rel bin.sdcc/hsk_flash/hsk_flash.rel The code runs on the Infineon XC878-16FF. ---------------------------------------------------------------------- >Comment By: Erik Petrich (epetrich) Date: 2012-11-16 22:51 Message: There's now (as of revision #8221) a new option for the mcs51 backend "--no-ret-without-call" that should ensure that everywhere that SDCC uses ret to do a computed jump it will also use a call instruction to keep the Memory Extension Stack from underflowing. With this option you should no longer need the custom peephole rule (it's been integrated into the code generator) and it takes care of both the switch/case problem you were seeing as well as the call via function pointer to function with parameters problem that you hadn't encountered yet (and would not have been fixed by the peephole rule) ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-10-10 07:38 Message: quote: epetrich ...the XC878 only has a single bank implemented, but the documentation shows the banking registers are still there and presumably the associated hardware is as well ... The banking actually has a purpose. E.g. on the 16-FF variant (64k flash) you can access the XRAM and bootloader from bank 2. So you can execute functions provided by the boot loader and execute code from XRAM. This is useful to bootstrap the µC for live flashing. There's probably a way to bank in external RAM/Flash, too. But I never looked into this. 64k of flash are plenty for my purposes (so far I'm only using up to 20k). ---------------------------------------------------------------------- Comment By: Erik Petrich (epetrich) Date: 2012-10-03 21:06 Message: Calling via function pointer should work fine for functions that have no parameters. In this case, there is a lcall to a library function _sdcc_call_dptr() which then does a jmp to the actual function entry point; this lcall will keep the Memory Extension Stack balanced with the ret at the end of the function. The problematic case that's not covered by my peephole rule is using a function pointer with functions that have parameters. As Maarten points out, the above strategy can not be used in this case due to conflicting register needs. So the compiler instead uses the same method that was used to jump to the cases in the larger switch statements and is failing for this microcontroller. It looks like currently the function pointers in hsk use no parameters, so the one peephole rule I've given you is sufficient for now. However, I'd like to make sure we also have a working solution for function pointers with parameters so that all the cases are covered. So I will leave this report open a little longer. Thanks for confirming that the peephole rule fixed the problem; this was certainly obscure. ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-10-03 15:38 Message: Also, I'm making excessive use of function pointers in ISRs. There is no issue with them. Or if there is, it's masked by the reti. ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-10-03 09:04 Message: @epetrich I have removed my workaround from all files and can confirm that your peephole rule works. Without it the system freezes, with it works as intended. Thank you very much! As for making SDCC XC800 compatible, that's the only issue I've encountered so far. Will you add an -mxc800 flag for the xc800 specific peephole rules? It seems a little harsh to inflict the burden of additional cycles on unaffected platforms. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2012-09-29 04:59 Message: Hmm, this is another good reason to want to use JMP @A+DPTR in crtcall.asm for all function pointers. But that requires ACC and DPTR to be free for the call and currently they're used for passing the first parameter. Thus crtcall.asm can only used for (void) functions. The jump table probably can always use JMP @A+DPTR but will give slightly larger code. The <112 case can use MOV DPL/DPH,A instead of PUSH ACC, but requires an extra CLR A. The <256 case requires PUSH ACC and an extra POP DPL on top of that. I have no objections against such a modification. Maarten ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-09-29 03:11 Message: Will try this next week (I'm visiting my folks and didn't take any hardware with me). And thanks for the insightful reply! ---------------------------------------------------------------------- Comment By: Erik Petrich (epetrich) Date: 2012-09-29 00:07 Message: Oops, that peephole rule should be: replace { push acc ret } by { push acc lcall .+3 dec sp dec sp ret } ---------------------------------------------------------------------- Comment By: Erik Petrich (epetrich) Date: 2012-09-28 23:55 Message: I've carefully looked through this for many hours and as far as I can tell, SDCC's handling of the switch statement is correct for an 8051/2 compatible processor. I suspect the problem you are seeing is due to a slight incompatibility introduced with how the XC800 series processors implement their banked memory support (the XC878 only has a single bank implemented, but the documentation shows the banking registers are still there and presumably the associated hardware is as well). The processor has a secondary stack (Memory Extension Stack) and stack pointer register (MEXSP) that are updated by the LCALL, ACALL, RET, and RETI instructions. This allows the processor to seamlessly transition between functions spread over a 1M address space (again, I realize the XC878 only implements a small portion of this memory). In the switch statements that you've encountered problems, SDCC has pushed the address of the appropriate case onto the regular stack and then executed a RET instruction. The RET instruction forms an address from the top two bytes popped from the regular stack and then jumps to this address. However, the XC800 series additionally pops a byte from the Memory Extension Stack and stores this to the MEX1 register, which potentially changes the currently selected bank of memory. The problem is that this RET instruction is not matched with a previous ACALL or LCALL instruction that would have pushed the correct byte onto the Memory Extension Stack (and since this does not exist on an 8051/2, SDCC has no idea that this needs to be taken into account). This is consistent with your description of the function working correctly on the first invocation, but failing to return on the second. There has been a call from the start-up code to main(), from main() to hsk_ex_port_open(), and perhaps another function in between. So by the time the switch statement is processed, there are several bytes on the Memory Extension Stack. The RET instructions in the two switch statements each take a byte of this stack, as does the RET at the end of the function. Although the MEX1 register is being reloaded in each instance, the value is always the same (indicating bank 0 is selected) so the correct code continues to execute. However, after the first call to hsk_ex_port_open(), MEXSP is pointing closer to the start of the Memory Extension Stack than it should. During the second invocation, MEXSP underflows past the start of the Memory Extension Stack, and MEX1 is loaded with some indeterminate value while processing one of the RET instructions, thus selecting a bank of memory that does not exist. SDCC uses a similar code sequence to invoke a function via a function pointer and would probably result in the same sort of crash. I think you could probably make SDCC compatible with the XC800 series by using a few custom peephole rules. This one should keep the Memory Extension Stack from underflowing during a switch statement: replace { push acc ret } with { push acc lcall .+3 dec sp dec sp ret } I think the dispatching via a function pointer could be handled similarly, but I've been staring at this too long today to attempt a peephole rule for that right now. Please try out the above peephole rule and confirm whether or not it fixed the problem you are seeing. ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-09-27 13:07 Message: Well, finally there's some affected code online: https://sourceforge.net/p/hsk/libs/ci/cb8d8415c84b2178fdd5115437fdfb6cd2f290ff/tree/src/hsk_ex/hsk_ex.c#l243 ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-08-22 00:50 Message: Hmm, that's strange, I never got an e-mail that there was a comment. I'll wrap something up in a couple of days. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2012-06-22 04:51 Message: How are we to check this bug if you only provide small snippets of code? Can you please attach an example file that reproduces the bug you mentioned? ---------------------------------------------------------------------- Comment By: Kamikaze Dominic Fandrey (lonkamikaze) Date: 2012-06-14 13:44 Message: What I forgot, I cannot debug this, because the debugger looses contact to the µC when the issue occurs. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=3535290&group_id=599 |