I will gladly tackle the removal of gosubs :) --- James
On Friday, November 11, 2016, Ralph Versteegen <[email protected]> wrote: > I can reproduce this on x86_64. I spent a while poking around with gdb, > but it was only afterwards that I realised that I'd seen the cause of the > bug... > > (gdb) frame 7 > #7 0x000000000044efd0 in FORMATION_SET_EDITOR () at > build/subs.rbas.bas:1036 > 1036 draw_formation_slices form, rootslice, -1, dpage > (gdb) p form > No symbol "form" in current context. > (gdb) p FORM > FORM$1 FORMATION_EDITOR FORMAT_DATE > FORMAT_PERCENT_COND FORM_ID$1 > FORMATIONS FORMATION_SET_EDITOR FORMAT_PERCENT > FORMSET$1 FORM_ID$1.12413 > (gdb) p FORM_ID$1 > $4 = 0 > (gdb) p FORM_ID$1. > Display all 200 possibilities? (y or n) > (gdb) p FORM_ID$1.12413 > A syntax error in expression, near `.12413'. > > Notice the 'if' check: > IF state.pt >= 4 AND form_id >= 0 THEN > draw_formation_slices form, rootslice, -1, dpage > > form_id is -1 when there is no formation the slot, and hence rootslice is > NULL. For some reason, according to gdb there are two form_id variables, > and while I confirmed that form_id is -1 inside the lpreviewform gosub, as > you can see above, form_id is 0 outside the gosub! > Next I had a look at the C code that fbc emitted, and there's only one > form_id variable. I'm really surprised that gdb knows that there's a second > form_id variable, which isn't apparent in the C code (is it listed in the > debug info for this function, or did it deduce it?), or what the .12413 > suffix means (it's not a line number). > > So, of course, what's happening here is that when using -gen gcc > GOSUB/RETRACE is implemented using setjmp/longjmp. Sure enough, the man > pages for those states that local variables which aren't marked 'volatile' > may or may not be restored by longjmp. GCC has stored form_id in a > register, so longjmp *reverts* its value to 0 on the RETRACE. This problem > didn't occur when I tested setjmp/longjmp GOSUBs without -gen gcc years ago > (you can edit config.bi to enable that), because fbc is not an optimising > compiler, and never keeps variables in registers. > > fbc actually has an option to implement normal GOSUB/RETURN with > setjmp/longjmp, so I should also check whether this problem affects > standard FB under -gen gcc. > > So to conclude, GOSUB is currently broken under -gen gcc and must not be > used. This finally explains why we had to get rid of them in Game to get it > to work on Android (one of those things that's been on the Android TODO > list). > What's the solution? Since it isn't possible to put inline C in FB files, > and it isn't possible to use FB's builtin GOSUB (if that happens to do > something special so that it works) since that requires -lang deprecated, > getting rid of GOSUB appears to be the only solution. > > We're pretty close to being rid of GOSUB. There are only 33 GOSUBS and 18 > RETRACEs left in the codebase, and there appear to be 17 GOSUB blocks, of > which 11 are only called once, so GOSUB/RETRACE can just be converted to > GOTO as a quick-fix rather than needing to be converted into a SUB. Number > of occurrences of each GOSUB: > 1 drawing.bas:GOSUB disable > 1 drawing.bas:GOSUB drawoval > 1 drawing.bas:GOSUB drawsquare > 1 drawing.bas:GOSUB floodfill > 1 drawing.bas:GOSUB putdot > 1 drawing.bas:GOSUB readundospr > 1 drawing.bas:GOSUB replacecol > 3 drawing.bas:GOSUB resettool > 5 drawing.bas:GOSUB setupsample > 2 drawing.bas:GOSUB spedbak > 1 drawing.bas:GOSUB sprayspot > 1 drawing.bas:GOSUB sprctrl > 1 drawing.bas:GOSUB straitline > 1 mapsubs.bas:GOSUB mapping > 9 subs4.bas:GOSUB vehmenu > 3 subs.rbas:GOSUB lpreviewform > > What do you know, most of them are in the graphics editors, which need to > be majorly refactored anyway. > > > Next time you see a 64-bit specific bug, compile with scons gengcc=1 to > try to reproduce it; the gcc emitter is more likely to be the cause than > the architecture. > > > On 12 November 2016 at 11:31, James Paige <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> Taoki confirmed that this change does indeed fix the crash he was seeing >> in the formation set editor. >> >> On Fri, Nov 11, 2016 at 2:24 PM, <[email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: >> >>> james >>> 2016-11-11 14:24:57 -0800 (Fri, 11 Nov 2016) >>> 99 >>> SUBify the GOSUB/RETRACE block that loaded the hero formation previews >>> in the formation set editor >>> --- >>> U wip/subs.rbas >>> _______________________________________________ >>> Ohrrpgce mailing list >>> [email protected] >>> <javascript:_e(%7B%7D,'cvml','[email protected]');> >>> http://lists.motherhamster.org/listinfo.cgi/ohrrpgce-motherhamster.org >>> >> >> >> _______________________________________________ >> Ohrrpgce mailing list >> [email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');> >> http://lists.motherhamster.org/listinfo.cgi/ohrrpgce-motherhamster.org >> >> >
_______________________________________________ Ohrrpgce mailing list [email protected] http://lists.motherhamster.org/listinfo.cgi/ohrrpgce-motherhamster.org
