This patchset fixes up the r-stack in interpret mode and the evaluate word so OpenBIOS can boot OS 9 without r-stack corruption or introducing any regressions.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
Cormac O'Brien (1): interpreter.fs: allow evaluate to split words on CR as well as LF
Mark Cave-Ayland (2): bootstrap.fs: add pseudo r-stack implementation for interpret mode interpreter.fs: don't clobber stack across evaluate strings split on newlines
forth/bootstrap/bootstrap.fs | 14 ++++++++++++++ forth/bootstrap/interpreter.fs | 4 +++- 2 files changed, 17 insertions(+), 1 deletion(-)
The OS 9 boot loader uses the r-stack outside of a word in interpret mode. Provide an r-stack implementation which allows r-stack accesses in interpret mode using a separate pseudo stack.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- forth/bootstrap/bootstrap.fs | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/forth/bootstrap/bootstrap.fs b/forth/bootstrap/bootstrap.fs index 0668cf7..cb791f8 100644 --- a/forth/bootstrap/bootstrap.fs +++ b/forth/bootstrap/bootstrap.fs @@ -1587,4 +1587,18 @@ false value capital-hex? here 200 allot tmp-comp-buf ! ;
+\ +\ Pseudo r-stack implementation for interpret mode +\ + +variable prstack 20 cells allot +variable #prstack 0 #prstack ! + +: prstack-push prstack #prstack @ cells + ! 1 #prstack +! ; +: prstack-pop -1 #prstack +! prstack #prstack @ cells + @ ; + +: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate + \ the end
On Sun, Jul 17, 2016 at 03:20:57PM +0100, Mark Cave-Ayland wrote:
+\ +\ Pseudo r-stack implementation for interpret mode +\
+variable prstack 20 cells allot +variable #prstack 0 #prstack !
20 hex or 20 decimal? If it isn't clear, put hex or decimal before this?
This allocates 21 cells and isn't portable, btw ("variable" allocate one cell already, not necessarily contiguous with the "allot" allocated cells).
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
['] >r , is very non-portable as well, "postpone >r" is much more modern.
Segher
On Sun, Jul 17, 2016 at 11:20:15AM -0500, Segher Boessenkool wrote:
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
Btw, why do you need those r> ... smth ... >r gymnastics?
Segher
On Sun, Jul 17, 2016 at 11:44:06AM -0500, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 11:20:15AM -0500, Segher Boessenkool wrote:
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
Btw, why do you need those r> ... smth ... >r gymnastics?
Hrm, okay, coffee helps thinking... Could use some documenting ;-)
Segher
On 17/07/16 18:29, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 11:44:06AM -0500, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 11:20:15AM -0500, Segher Boessenkool wrote:
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
Btw, why do you need those r> ... smth ... >r gymnastics?
Hrm, okay, coffee helps thinking... Could use some documenting ;-)
As you've already worked out, the reason is that : calls the primitive docol which always pushes the caller onto the r-stack when calling another Forth word. Hence there's an extra parameter on the r-stack that needs to be juggled to make sure the >r primword and the resulting parameter are placed back correctly on the r-stack.
Regarding the documentation, I wasn't too worried about this since it's fairly obvious that calling another word is going to involve another r-stack push from the caller (and indeed it was the first thing that stood out when I tried your code in the debugger).
If you would like to suggest something suitable in a maximum of a couple of lines you think would help then I don't mind adding it in a v2, however compared to other undocumented gotchas in the Forth code I've come across whilst working on OpenBIOS this was very easy to figure out so I can't get too excited about it.
ATB,
Mark.
On Sun, Jul 17, 2016 at 08:21:58PM +0100, Mark Cave-Ayland wrote:
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
Btw, why do you need those r> ... smth ... >r gymnastics?
Hrm, okay, coffee helps thinking... Could use some documenting ;-)
As you've already worked out, the reason is that : calls the primitive docol which always pushes the caller onto the r-stack when calling another Forth word. Hence there's an extra parameter on the r-stack that needs to be juggled to make sure the >r primword and the resulting parameter are placed back correctly on the r-stack.
Regarding the documentation, I wasn't too worried about this since it's fairly obvious that calling another word is going to involve another r-stack push from the caller (and indeed it was the first thing that stood out when I tried your code in the debugger).
It surprises me every. single. time.
If you would like to suggest something suitable in a maximum of a couple of lines you think would help then I don't mind adding it in a v2, however compared to other undocumented gotchas in the Forth code I've come across whilst working on OpenBIOS this was very easy to figure out so I can't get too excited about it.
\ WARNING: Deep magic ahead!
Segher
On 17/07/16 20:37, Segher Boessenkool wrote:
Regarding the documentation, I wasn't too worried about this since it's fairly obvious that calling another word is going to involve another r-stack push from the caller (and indeed it was the first thing that stood out when I tried your code in the debugger).
It surprises me every. single. time.
Really?!?
If you would like to suggest something suitable in a maximum of a couple of lines you think would help then I don't mind adding it in a v2, however compared to other undocumented gotchas in the Forth code I've come across whilst working on OpenBIOS this was very easy to figure out so I can't get too excited about it.
\ WARNING: Deep magic ahead!
Ha! If this were the general guideline then that comment would be replicated a lot throughout the entire OpenBIOS codebase so I'll give this a miss for now ;)
ATB,
Mark.
On Sun, Jul 17, 2016 at 09:51:23PM +0100, Mark Cave-Ayland wrote:
Regarding the documentation, I wasn't too worried about this since it's fairly obvious that calling another word is going to involve another r-stack push from the caller (and indeed it was the first thing that stood out when I tried your code in the debugger).
It surprises me every. single. time.
Really?!?
Yes. I also use systems that do not use the traditional implementation approach (return address not on the return stack, DO loop counters not on the return stack, etc. -- Apple OF has both of these, or the latter at least, I can never remember such things).
And I did write TOES> so I really should be able to handle juggling two stacks. Sigh.
If you would like to suggest something suitable in a maximum of a couple of lines you think would help then I don't mind adding it in a v2, however compared to other undocumented gotchas in the Forth code I've come across whilst working on OpenBIOS this was very easy to figure out so I can't get too excited about it.
\ WARNING: Deep magic ahead!
Ha! If this were the general guideline then that comment would be replicated a lot throughout the entire OpenBIOS codebase so I'll give this a miss for now ;)
:-)
Ideally, most code should not have any trickiness to it.
Segher
On 17/07/16 17:20, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 03:20:57PM +0100, Mark Cave-Ayland wrote:
+\ +\ Pseudo r-stack implementation for interpret mode +\
+variable prstack 20 cells allot +variable #prstack 0 #prstack !
20 hex or 20 decimal? If it isn't clear, put hex or decimal before this?
The default in OpenBIOS is hex, so the intended value is 32 decimal. Skimming through the existing files, it looks like the conventional way to do "h# 20" so I can do that on a v2.
This allocates 21 cells and isn't portable, btw ("variable" allocate one cell already, not necessarily contiguous with the "allot" allocated cells).
Looking at the existing code, the most common way to reserve memory in the dictionary is with allot, e.g.
variable prstack here h# 20 cells allot prstack !
Does that look reasonable?
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
['] >r , is very non-portable as well, "postpone >r" is much more modern.
The version above is what is used throughout the OpenBIOS codebase as opposed to postpone which is why I went with it. I've just tried using postpone in OpenBIOS and it causes things to break, so I suspect that either it does something strange to the r-stack itself or perhaps it is another word that needs fixes for immediate mode.
ATB,
Mark.
On Sun, Jul 17, 2016 at 08:05:33PM +0100, Mark Cave-Ayland wrote:
+\ +\ Pseudo r-stack implementation for interpret mode +\
+variable prstack 20 cells allot +variable #prstack 0 #prstack !
20 hex or 20 decimal? If it isn't clear, put hex or decimal before this?
The default in OpenBIOS is hex, so the intended value is 32 decimal. Skimming through the existing files, it looks like the conventional way to do "h# 20" so I can do that on a v2.
If the convention is to have hex everywhere, then your existing code is fine. h# 20 is clear as well.
This allocates 21 cells and isn't portable, btw ("variable" allocate one cell already, not necessarily contiguous with the "allot" allocated cells).
Looking at the existing code, the most common way to reserve memory in the dictionary is with allot, e.g.
variable prstack here h# 20 cells allot prstack !
Does that look reasonable?
No, that means something entirely different (extra indirection).
create version1 20 cells allot variable version2 20 cells allot
version1 is portable and allocates 20 cells. version2 is not portable, and allocates 21 cells.
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
['] >r , is very non-portable as well, "postpone >r" is much more modern.
The version above is what is used throughout the OpenBIOS codebase as opposed to postpone which is why I went with it. I've just tried using postpone in OpenBIOS and it causes things to break, so I suspect that either it does something strange to the r-stack itself or perhaps it is another word that needs fixes for immediate mode.
Huh, wow. The whole point of POSTPONE is that you can use it on words that are either IMMEDIATE or not; i.e. it does the jobs of both [COMPILE] and COMPILE . Strange that POSTPONE does not work for you.
Segher
On 17/07/16 20:25, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 08:05:33PM +0100, Mark Cave-Ayland wrote:
+\ +\ Pseudo r-stack implementation for interpret mode +\
+variable prstack 20 cells allot +variable #prstack 0 #prstack !
20 hex or 20 decimal? If it isn't clear, put hex or decimal before this?
The default in OpenBIOS is hex, so the intended value is 32 decimal. Skimming through the existing files, it looks like the conventional way to do "h# 20" so I can do that on a v2.
If the convention is to have hex everywhere, then your existing code is fine. h# 20 is clear as well.
Great, thanks - I'll go with that for now.
This allocates 21 cells and isn't portable, btw ("variable" allocate one cell already, not necessarily contiguous with the "allot" allocated cells).
Looking at the existing code, the most common way to reserve memory in the dictionary is with allot, e.g.
variable prstack here h# 20 cells allot prstack !
Does that look reasonable?
No, that means something entirely different (extra indirection).
create version1 20 cells allot variable version2 20 cells allot
version1 is portable and allocates 20 cells. version2 is not portable, and allocates 21 cells.
Looks like the issue with create is that it triggers a bug in the Forth bootstrap by the C kernel. If I move the r-stack functions into a separate file which is compiled by the Forth engine then it works fine so I'm tempted to do that for now (plus it means the file can be removed/changed for one or all architectures if required although I haven't seen a regression across all my tests with the patch applied).
I'll make the change here and repost the entire patchset as a v2.
+: >r state @ if ['] >r , exit then r> swap prstack-push >r ; immediate +: r> state @ if ['] r> , exit then r> prstack-pop swap >r ; immediate +: r@ state @ if ['] r@ , exit then r> prstack-pop dup prstack-push swap >r ; immediate
['] >r , is very non-portable as well, "postpone >r" is much more modern.
The version above is what is used throughout the OpenBIOS codebase as opposed to postpone which is why I went with it. I've just tried using postpone in OpenBIOS and it causes things to break, so I suspect that either it does something strange to the r-stack itself or perhaps it is another word that needs fixes for immediate mode.
Huh, wow. The whole point of POSTPONE is that you can use it on words that are either IMMEDIATE or not; i.e. it does the jobs of both [COMPILE] and COMPILE . Strange that POSTPONE does not work for you.
I tried the use of postpone again in the separate file, but that still doesn't work so there must be something else wrong the implementation in OpenBIOS. Hmmm.
ATB,
Mark.
When an evaluate string is split across a newline, the current string position is assumed to be the top-most item on the stack. However in the case of yaboot, items are left on the stack to be used by a subsequent line within the same evaluate statement and so subsequent lines are parsed incorrectly.
Fix this by saving the current string position on the r-stack across calls to (evaluate) so the stack remains correct for subsequent lines.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- forth/bootstrap/interpreter.fs | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/forth/bootstrap/interpreter.fs b/forth/bootstrap/interpreter.fs index 5187058..b66e95e 100644 --- a/forth/bootstrap/interpreter.fs +++ b/forth/bootstrap/interpreter.fs @@ -165,7 +165,9 @@ defer outer-interpreter over + over do i c@ 0a = if i over - + rot >r (evaluate) + r> i 1+ then loop
On Sun, Jul 17, 2016 at 03:20:58PM +0100, Mark Cave-Ayland wrote:
When an evaluate string is split across a newline, the current string position is assumed to be the top-most item on the stack. However in the case of yaboot, items are left on the stack to be used by a subsequent line within the same evaluate statement and so subsequent lines are parsed incorrectly.
Nice catch! These things are hard to debug.
--- a/forth/bootstrap/interpreter.fs +++ b/forth/bootstrap/interpreter.fs @@ -165,7 +165,9 @@ defer outer-interpreter over + over do i c@ 0a = if i over -
rot >r (evaluate)r> i 1+
This becomes hard to read. So you have (inside the "if"):
( a b ) i over - ( a b i-b ) rot >r ( b i-b R: a ) (evaluate) ( R: a ) r> ( a )
swap >r dup i swap - (evaluate) r>
but that is even longer. Wow. Long definitions are bad, mkay? ;-)
It might be better without the DO loop (it blocks the R stack). Probably it is best to break out a natural factor (like, find the length of a string until the first line break -- isn't there a word like it already, like "left-split" but splitting on any line break char instead of on one delim).
Segher
On 17/07/16 17:39, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 03:20:58PM +0100, Mark Cave-Ayland wrote:
When an evaluate string is split across a newline, the current string position is assumed to be the top-most item on the stack. However in the case of yaboot, items are left on the stack to be used by a subsequent line within the same evaluate statement and so subsequent lines are parsed incorrectly.
Nice catch! These things are hard to debug.
Yeah. Tell me about it! ;)
--- a/forth/bootstrap/interpreter.fs +++ b/forth/bootstrap/interpreter.fs @@ -165,7 +165,9 @@ defer outer-interpreter over + over do i c@ 0a = if i over -
rot >r (evaluate)r> i 1+This becomes hard to read. So you have (inside the "if"):
( a b ) i over - ( a b i-b ) rot >r ( b i-b R: a ) (evaluate) ( R: a ) r> ( a )
swap >r dup i swap - (evaluate) r>
but that is even longer. Wow. Long definitions are bad, mkay? ;-)
It might be better without the DO loop (it blocks the R stack). Probably it is best to break out a natural factor (like, find the length of a string until the first line break -- isn't there a word like it already, like "left-split" but splitting on any line break char instead of on one delim).
Actually I did experiment without the do loop, but like you I found that the slight ease of the code within the if didn't seem to justify the complexity outside of it which is why I decided to stick with what was there.
The other reason, of course, is that the code is fairly well tested in its current form and so if we want to get this in for 2.7 I'd much prefer to minimise the changes in this particular patchset :)
It seems that left-split does exist in OpenBIOS, but according to the IEEE1275 standard it takes only a single character to split upon so I'm not convinced it would make things more sane in this particular case where we want to split on more than one character.
ATB,
Mark.
On Sun, Jul 17, 2016 at 08:10:27PM +0100, Mark Cave-Ayland wrote:
It might be better without the DO loop (it blocks the R stack). Probably it is best to break out a natural factor (like, find the length of a string until the first line break -- isn't there a word like it already, like "left-split" but splitting on any line break char instead of on one delim).
Actually I did experiment without the do loop, but like you I found that the slight ease of the code within the if didn't seem to justify the complexity outside of it which is why I decided to stick with what was there.
If you use a BEGIN loop (instead of a DO loop) it becomes easier to factor the word (because you do not have to deal with I using the return stack).
The other reason, of course, is that the code is fairly well tested in its current form and so if we want to get this in for 2.7 I'd much prefer to minimise the changes in this particular patchset :)
That is of course a good reason.
It seems that left-split does exist in OpenBIOS, but according to the IEEE1275 standard it takes only a single character to split upon so I'm not convinced it would make things more sane in this particular case where we want to split on more than one character.
My suggestion was to make a word like it, something that takes everything smaller than BL as delimiter, like BL FINDCHAR and BL PARSE and SKIPWS etc. I probably wasn't very clear, sorry :-/
Segher
On 17/07/16 20:34, Segher Boessenkool wrote:
On Sun, Jul 17, 2016 at 08:10:27PM +0100, Mark Cave-Ayland wrote:
It might be better without the DO loop (it blocks the R stack). Probably it is best to break out a natural factor (like, find the length of a string until the first line break -- isn't there a word like it already, like "left-split" but splitting on any line break char instead of on one delim).
Actually I did experiment without the do loop, but like you I found that the slight ease of the code within the if didn't seem to justify the complexity outside of it which is why I decided to stick with what was there.
If you use a BEGIN loop (instead of a DO loop) it becomes easier to factor the word (because you do not have to deal with I using the return stack).
The other reason, of course, is that the code is fairly well tested in its current form and so if we want to get this in for 2.7 I'd much prefer to minimise the changes in this particular patchset :)
That is of course a good reason.
It seems that left-split does exist in OpenBIOS, but according to the IEEE1275 standard it takes only a single character to split upon so I'm not convinced it would make things more sane in this particular case where we want to split on more than one character.
My suggestion was to make a word like it, something that takes everything smaller than BL as delimiter, like BL FINDCHAR and BL PARSE and SKIPWS etc. I probably wasn't very clear, sorry :-/
No worries, I really appreciate all the advice. I'm just really conscious of the time this would take, so unless you can see an absolute blocker then I'd be inclined to go with the patch in its current form and then figure out a better strategy moving forward.
ATB,
Mark.
On Sun, Jul 17, 2016 at 09:48:17PM +0100, Mark Cave-Ayland wrote:
No worries, I really appreciate all the advice. I'm just really conscious of the time this would take, so unless you can see an absolute blocker then I'd be inclined to go with the patch in its current form and then figure out a better strategy moving forward.
Oh absolutely, take things step by step. If I see anything that is really wrong, I'll say so, you won't be able to miss it. Same if I think something is a step backwards. Everything else I say is just commentary, do with it as you please :-)
Segher
From: Cormac O'Brien cormac@c-obrien.org
Otherwise the Forth intepreter fails due to lack of buffer space when trying to execute large boot scripts on platforms that use CR instead of LF for line endings (particularly MacOS 9).
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk Signed-off-by: Cormac O'Brien cormac@c-obrien.org Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- forth/bootstrap/interpreter.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/forth/bootstrap/interpreter.fs b/forth/bootstrap/interpreter.fs index b66e95e..f02000f 100644 --- a/forth/bootstrap/interpreter.fs +++ b/forth/bootstrap/interpreter.fs @@ -163,7 +163,7 @@ defer outer-interpreter : evaluate ( str len -- ?? ) 2dup + -rot over + over do - i c@ 0a = if + i c@ dup 0a = swap 0d = or if i over - rot >r (evaluate)
-
Mark Cave-Ayland -
Segher Boessenkool