[Ohrrpgce] SVN: james/8209 SUBify the GOSUB/RETRACE block that loaded the hero formation previews i

[James Paige]

I will gladly tackle the removal of gosubs :)

---
James

On Friday, November 11, 2016, Ralph Versteegen <teeemcee at gmail.com> 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 <Bob at hamsterrepublic.com
> <javascript:_e(%7B%7D,'cvml','Bob at hamsterrepublic.com');>> 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, <subversion at hamsterrepublic.com
>> <javascript:_e(%7B%7D,'cvml','subversion at hamsterrepublic.com');>> 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
>>> ohrrpgce at lists.motherhamster.org
>>> <javascript:_e(%7B%7D,'cvml','ohrrpgce at lists.motherhamster.org');>
>>> http://lists.motherhamster.org/listinfo.cgi/ohrrpgce-motherhamster.org
>>>
>>
>>
>> _______________________________________________
>> Ohrrpgce mailing list
>> ohrrpgce at lists.motherhamster.org
>> <javascript:_e(%7B%7D,'cvml','ohrrpgce at lists.motherhamster.org');>
>> http://lists.motherhamster.org/listinfo.cgi/ohrrpgce-motherhamster.org
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.motherhamster.org/pipermail/ohrrpgce-motherhamster.org/attachments/20161111/493287c2/attachment-0001.htm>