Incorrect implementation of b?branch?
Hi everyone, I've been spending some time over the past few weeks trying to get a Sparc64/Solaris disk image to boot under qemu, and as far as I can tell, the first reason that the code terminates incorrectly is because b?branch doesn't work. This can be demonstrated fairly easily using the following couple of Fcode programs. Note that while I'm working on Sparc64, I see the same issue on x86 OpenBIOS too. Example 1: b?branch with -1 on stack: ignore branch & abort CC \ offset16 10 \ b(lit) FF FF FF FF \ (-1) 14 00 04 \ b?branch 02 16 \ if false, abort 9D \ if true, display stack 00 \ end 0 > true to ?fcode-verbose ok 0 > here CC10FFFFFFFF1400 /n allot over ! ok 1 > here 0402169D00000000 /n allot over ! drop ok 1 > 1 byte-load byte-load: evaluating fcode at 0xffd9d128 fcode-table at 0xffdb7f90 offset16 [ 0xcc ] b(lit) [ 0x10 ] b?branch [ 0x14 ] abort [ 0x216 ] byte-load: exception caught! ok Example 2: b?branch with 0 on stack: skip abort, display stack CC \ offset16 10 \ b(lit) 00 00 00 00 \ (0) 14 00 04 \ b?branch 02 16 \ if false, abort 9D \ if true, display stack 00 \ end 0 > true to ?fcode-verbose ok 0 > here CC10000000001400 /n allot over ! ok 1 > here 0402169D00000000 /n allot over ! drop ok 1 > 1 byte-load byte-load: evaluating fcode at 0xffd9d128 fcode-table at 0xffdb7f90 offset16 [ 0xcc ] b(lit) [ 0x10 ] b?branch [ 0x14 ] abort [ 0x216 ] byte-load: exception caught! ok Okay, so did anyone notice the difference?! This led me to look into the definition of b?branch in forth/device/fcode.fs: \ b?branch ( continue? -- ) \ Conditional branch FCode. Followed by FCode-offset. : b?branch ['] do?branch , fcode-offset 0< if \ if we jump backwards, we can forsee where it goes resolve-dest else here 0 , then ; immediate By adding some extra debugging into b?branch, I can see that for the above Fcode, do?branch is *never* called, although it correct takes the "else" branch listed above. I'm still reasonably new to Forth, but AIUI, all the code above does is allocate one cell to store the xt of do?branch, and in the case of a forward branch, push the here location onto the stack and then store 0 into another allocated cell. This agrees with my investigation that shows that do?branch is never actually called. Given that this code is also missing a section that will read and discard Fcode-offset-2 bytes for a forward branch, I'm inclined to think that this code is just wrong. Can anyone else with any experience with this code comment upon this? ATB, Mark. -- Mark Cave-Ayland Sirius Corporation - The Open Source Experts http://www.siriusit.co.uk T: +44 870 608 0063
On 21.02.2009 13:20 Uhr, Mark Cave-Ayland wrote:
Hi everyone,
I've been spending some time over the past few weeks trying to get a Sparc64/Solaris disk image to boot under qemu, and as far as I can tell, the first reason that the code terminates incorrectly is because b?branch doesn't work.
This can be demonstrated fairly easily using the following couple of Fcode programs. Note that while I'm working on Sparc64, I see the same issue on x86 OpenBIOS too.
Example 1: b?branch with -1 on stack: ignore branch & abort
CC \ offset16 10 \ b(lit) FF FF FF FF \ (-1) 14 00 04 \ b?branch 02 16 \ if false, abort 9D \ if true, display stack 00 \ end
0 > true to ?fcode-verbose ok 0 > here CC10FFFFFFFF1400 /n allot over ! ok 1 > here 0402169D00000000 /n allot over ! drop ok 1 > 1 byte-load byte-load: evaluating fcode at 0xffd9d128 fcode-table at 0xffdb7f90 offset16 [ 0xcc ] b(lit) [ 0x10 ] b?branch [ 0x14 ] abort [ 0x216 ]
byte-load: exception caught! ok
Example 2: b?branch with 0 on stack: skip abort, display stack
CC \ offset16 10 \ b(lit) 00 00 00 00 \ (0) 14 00 04 \ b?branch 02 16 \ if false, abort 9D \ if true, display stack 00 \ end
0 > true to ?fcode-verbose ok 0 > here CC10000000001400 /n allot over ! ok 1 > here 0402169D00000000 /n allot over ! drop ok 1 > 1 byte-load byte-load: evaluating fcode at 0xffd9d128 fcode-table at 0xffdb7f90 offset16 [ 0xcc ] b(lit) [ 0x10 ] b?branch [ 0x14 ] abort [ 0x216 ]
byte-load: exception caught! ok
Okay, so did anyone notice the difference?! This led me to look into the definition of b?branch in forth/device/fcode.fs:
\ b?branch ( continue? -- ) \ Conditional branch FCode. Followed by FCode-offset.
: b?branch ['] do?branch , fcode-offset 0< if \ if we jump backwards, we can forsee where it goes resolve-dest else here 0 , then ; immediate
By adding some extra debugging into b?branch, I can see that for the above Fcode, do?branch is *never* called, although it correct takes the "else" branch listed above.
I'm still reasonably new to Forth, but AIUI, all the code above does is allocate one cell to store the xt of do?branch, and in the case of a forward branch, push the here location onto the stack and then store 0 into another allocated cell. This agrees with my investigation that shows that do?branch is never actually called.
Given that this code is also missing a section that will read and discard Fcode-offset-2 bytes for a forward branch, I'm inclined to think that this code is just wrong. Can anyone else with any experience with this code comment upon this?
I think you indeed hit a bug. b?branch in openbios only works in word definitions, not in interpretion mode. It needs similar handling as setup-tmp-comp/execute-tmp-comp in the definition of "if" in bootstrap.fs. In addition to that your example contains non-valid code. A b?branch always needs to point behind a b(>resolve) as in the below example. here cc c, \ offset16 \ a4 c, \ -1 a5 c, \ 0 14 c, 00 c, 05 c, \ b?branch 02 c, 16 c, \ abort b2 c, \ b(>resolve) 9f c, \ .s 00 c, \ end0 1 byte-load Forth equivalent: \ -1 0 if abort then .s end0 Stefan -- coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br. Tel.: +49 761 7668825 • Fax: +49 761 7664613 Email: info@coresystems.de • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
Stefan Reinauer wrote:
I think you indeed hit a bug. b?branch in openbios only works in word definitions, not in interpretion mode. It needs similar handling as setup-tmp-comp/execute-tmp-comp in the definition of "if" in bootstrap.fs.
No way! I was actually looking at the code in bootstrap.fs thinking that the code assumed it was working inside a temporary buffer, and wondering if the setup-tmp-comp word would help me. Thanks for the pointer.
In addition to that your example contains non-valid code. A b?branch always needs to point behind a b(>resolve) as in the below example.
here cc c, \ offset16 \ a4 c, \ -1 a5 c, \ 0 14 c, 00 c, 05 c, \ b?branch 02 c, 16 c, \ abort b2 c, \ b(>resolve) 9f c, \ .s 00 c, \ end0 1 byte-load
Indeed, that is my mistake. Please find the attached patch, which when used with your code example above now seems to do the right thing for b?branch: 0 > here ok 1 > cc c, ok 1 > a5 c, ok 1 > 14 c, 00 c, 05 c, ok 1 > 02 c, 16 c, ok 1 > b2 c, ok 1 > 9f c, ok 1 > 00 c, ok 1 > 1 byte-load <e> -1 1 0 -1 0 0 0 ffd79378 0 0 0 0 0 0 ok 0 > here ok 1 > cc c, ok 1 > a4 c, ok 1 > 14 c, 00 c, 05 c, ok 1 > 02 c, 16 c, ok 1 > b2 c, ok 1 > 9f c, ok 1 > 00 c, ok 1 > 1 byte-load byte-load: exception caught! ok At least now if I find similar errors, I have an idea of how to fix them :) Many thanks, Mark. -- Mark Cave-Ayland Sirius Corporation - The Open Source Experts http://www.siriusit.co.uk T: +44 870 608 0063
On 2/21/09, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Stefan Reinauer wrote:
I think you indeed hit a bug. b?branch in openbios only works in word definitions, not in interpretion mode. It needs similar handling as setup-tmp-comp/execute-tmp-comp in the definition of "if" in bootstrap.fs.
No way! I was actually looking at the code in bootstrap.fs thinking that the code assumed it was working inside a temporary buffer, and wondering if the setup-tmp-comp word would help me. Thanks for the pointer.
In addition to that your example contains non-valid code. A b?branch always needs to point behind a b(>resolve) as in the below example.
here cc c, \ offset16 \ a4 c, \ -1 a5 c, \ 0 14 c, 00 c, 05 c, \ b?branch 02 c, 16 c, \ abort b2 c, \ b(>resolve) 9f c, \ .s 00 c, \ end0 1 byte-load
Indeed, that is my mistake. Please find the attached patch, which when used with your code example above now seems to do the right thing for b?branch:
0 > here ok 1 > cc c, ok 1 > a5 c, ok 1 > 14 c, 00 c, 05 c, ok 1 > 02 c, 16 c, ok 1 > b2 c, ok 1 > 9f c, ok 1 > 00 c, ok 1 > 1 byte-load <e> -1 1 0 -1 0 0 0 ffd79378 0 0 0 0 0 0 ok
0 > here ok 1 > cc c, ok 1 > a4 c, ok 1 > 14 c, 00 c, 05 c, ok 1 > 02 c, 16 c, ok 1 > b2 c, ok 1 > 9f c, ok 1 > 00 c, ok 1 > 1 byte-load byte-load: exception caught! ok
At least now if I find similar errors, I have an idea of how to fix them :)
Thanks, I'll apply this. My marTux and Milax test images get a little further with this patch.
Mark Cave-Ayland wrote:
At least now if I find similar errors, I have an idea of how to fix them :)
In terms of related errors, it now looks as if backwards branches (b<mark)...(b?branch) are broken in a similar way. I'm still struggling at the moment to write some code that works (I can't seem to get setup-tmp-comp/execute-tmp-comp to nest correctly at the moment), so I may have some more questions related to this coming up over the next few days. However, the one thing I did find is that fcode-offset is broken for backwards branches, since it currently treats the 8-bit or 16-bit offset as unsigned - which is not helpful when trying to execute backwards branches. Please find attached a patch that does the right thing here - can someone review and commit this to SVN? Many thanks, Mark. -- Mark Cave-Ayland Sirius Corporation - The Open Source Experts http://www.siriusit.co.uk T: +44 870 608 0063
Mark Cave-Ayland wrote: (cut)
However, the one thing I did find is that fcode-offset is broken for backwards branches, since it currently treats the 8-bit or 16-bit offset as unsigned - which is not helpful when trying to execute backwards branches. Please find attached a patch that does the right thing here - can someone review and commit this to SVN?
*Poke* ? ATB, Mark. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063
On 2/25/09, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Mark Cave-Ayland wrote:
(cut)
However, the one thing I did find is that fcode-offset is broken for backwards branches, since it currently treats the 8-bit or 16-bit offset as unsigned - which is not helpful when trying to execute backwards branches. Please find attached a patch that does the right thing here - can someone review and commit this to SVN?
*Poke* ?
Basically looks OK to my untrained eyes, except I wonder if there are sign extension functions in Forth.
Blue Swirl wrote:
Basically looks OK to my untrained eyes, except I wonder if there are sign extension functions in Forth.
Thanks for the review. The nearest things I can find are in section 7.3.2.4 of forth/bootstrap/bootstrap.fs, but these are clearly just byte-swapping words. I'm reasonably confident that it's safe to apply as its quite a small contained patch. However it may be worth someone with more Forth experience casting a quick eye over it to make sure I haven't done something too stupid... ATB, Mark. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063
On 2/27/09, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Blue Swirl wrote:
Basically looks OK to my untrained eyes, except I wonder if there are sign extension functions in Forth.
Thanks for the review. The nearest things I can find are in section 7.3.2.4 of forth/bootstrap/bootstrap.fs, but these are clearly just byte-swapping words.
OK, then the patch is about optimal.
I'm reasonably confident that it's safe to apply as its quite a small contained patch. However it may be worth someone with more Forth experience casting a quick eye over it to make sure I haven't done something too stupid...
I'm not suspecting of any problems either. I'll apply the patch, thanks for your patience.
Basically looks OK to my untrained eyes, except I wonder if there are sign extension functions in Forth.
In OF, there are <w@ and <l@ , which do loads of signed 16-bit and 32-bit values. For other sign extensions: here, have some: : >s16 ffff and 8000 xor 8000 - ; : >s32 ffffffff and 80000000 xor 80000000 - ; Segher
participants (4)
-
Blue Swirl -
Mark Cave-Ayland -
Segher Boessenkool -
Stefan Reinauer