[PATCH] libopenbios/bootinfo_load.c: Implement Mac OS 9.2 boot script support

[Programmingkid]

This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> r and r> with sub_>r and sub_r>. This allows for the return stack to be left

alone. This patch also makes it so that the boot script is executed one line at a time.

Signed-off-by: John Arbuckle

Note: this patch depends on the strstr() and return stack substitution patches.

Index: libopenbios/bootinfo_load.c
===================================================================
--- libopenbios/bootinfo_load.c	(revision 1395)
+++ libopenbios/bootinfo_load.c	(working copy)
@@ -19,6 +19,7 @@
 #include "libopenbios/bootinfo_load.h"
 #include "libopenbios/ofmem.h"
 #include "libc/vsprintf.h"
+#include "libc/string.h"

//#define DEBUG_BOOTINFO

@@ -116,6 +117,75 @@
    return LOADER_NOT_SUPPORT;
 }

+// Replace all occurrences of orig_str in buffer with replace_str
+static void replace_string(char *buffer, const char *orig_str, const char *replace_str)
+{
+    char *ptr;
+    int index, new_buf_index = 0;
+    const int max_size = 5000;
+    char *new_buffer = malloc(max_size * sizeof(char));
+
+    for (index = 0; index < strlen(buffer); index++) {
+        if (buffer[index] == orig_str[0]) {
+            ptr = strstr(buffer + index, orig_str);
+
+            // if we encountered an orig_str in the buffer
+            if (index == (ptr - buffer)) {
+                sprintf(new_buffer + new_buf_index, "%s ", replace_str);
+                new_buf_index += strlen(replace_str) + 1;
+                index += strlen(orig_str);
+                continue;
+            }
+        }
+        new_buffer[new_buf_index++] = buffer[index];
+    }
+
+    // copy new_buffer into buffer
+    for (index = 0; index < strlen(new_buffer); index++) {
+        buffer[index] = new_buffer[index];
+    }
+    buffer[index+1] = '\0';
+}
+
+/* Replace >r and r> with sub_>r and sub_r> */
+static void replace_return_stack_words(char *bootscript)
+{
+    const char *find_str1 = ">r";
+    const char *replace_str1 = "sub_>r";
+    const char *find_str2 = "r>";
+    const char *replace_str2 = "sub_r>";
+    
+    replace_string(bootscript, find_str1, replace_str1);
+    replace_string(bootscript, find_str2, replace_str2);
+}
+
+/* Runs the bootscript one line at a time */
+void run_script(char *bootscript)
+{
+    int index = 0, buf_index = 0;
+    char c;
+    char *buffer = malloc(1000 * sizeof(char));
+    
+    while (1) {
+        c = bootscript[index];
+        
+        // fill up buffer
+        while (c != '\n' && c != '\r' && c != '\0') {
+            buffer[buf_index++] = c;
+            c = bootscript[++index];
+        }
+        buffer[buf_index] = '\0';
+        buf_index = 0;
+        index++;
+        DPRINTF("%s\n", buffer);
+        feval(buffer);
+        if (c == '\0') {
+            break;
+        }
+    }
+    free(buffer);
+}
+
 /*
   Parse SGML structure like:
   <chrp-boot>
@@ -209,9 +279,6 @@
    				script = 0;
    				bootscript[scriptlen] = '\0';

-					DPRINTF("got bootscript %s\n",
-						bootscript);
-
    				scriptvalid = -1;

break;
@@ -261,8 +328,8 @@

/* If the payload is bootinfo then we execute it immediately */
    if (scriptvalid) {
-		DPRINTF("bootscript: %s\n", bootscript);
-		feval(bootscript);
+        replace_return_stack_words(bootscript);
+		run_script(bootscript);
    }
    else
    	DPRINTF("Unable to parse bootinfo bootscript\n");

[BALATON Zoltan]

On Tue, 26 Apr 2016, Programmingkid wrote:
> This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> > r and r> with sub_>r and sub_r>. This allows for the return stack to be left
>
> alone.

Can we have Segher's Forth version of this instead:

https://www.coreboot.org/pipermail/openbios/2016-April/009338.html

That should be equivalent and would make the C code much simpler and allow 
dropping most of these patches.
> This patch also makes it so that the boot script is executed one line at a time.

This part below would be basically all that remains. Is this also 
equivalent to the previous patch to interpreter.fs for also handling \r 
the same as \n or does it do something else? If not then maybe the whole 
thing could be done in Forth entirely.

Regards,
BALATON Zoltan
> +/* Runs the bootscript one line at a time */
> +void run_script(char *bootscript)
> +{
> int index = 0, buf_index = 0;
> char c;
> char *buffer = malloc(1000 * sizeof(char));
> while (1) {
>    c = bootscript[index];
>    // fill up buffer
>    while (c != '\n' && c != '\r' && c != '\0') {
>        buffer[buf_index++] = c;
>        c = bootscript[++index];
>    }
>    buffer[buf_index] = '\0';
>    buf_index = 0;
>    index++;
>    DPRINTF("%s\n", buffer);
>    feval(buffer);
>    if (c == '\0') {
>        break;
>    }
> }
> free(buffer);
>
> +}
>
> /*
>   Parse SGML structure like:
>
>   <chrp-boot>
> @@ -209,9 +279,6 @@
>    				script = 0;
>    				bootscript[scriptlen] = '\0';
> 			DPRINTF("got bootscript %s\n",
> 				bootscript);
> 		scriptvalid = -1;
>
> 		break;
>
> @@ -261,8 +328,8 @@
>
> /* If the payload is bootinfo then we execute it immediately */
>    if (scriptvalid) {
> DPRINTF("bootscript: %s\n", bootscript);
> feval(bootscript);
>    replace_return_stack_words(bootscript);
> run_script(bootscript);
> }
> else
> DPRINTF("Unable to parse bootinfo bootscript\n");

[Programmingkid]

On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> On Tue, 26 Apr 2016, Programmingkid wrote:
> > This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> > > r and r> with sub_>r and sub_r>. This allows for the return stack to be left
> >
> > alone.
>
> Can we have Segher's Forth version of this instead:
>
> https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
>
> That should be equivalent and would make the C code much simpler and allow dropping most of these patches.

Which file should I implement these words in?
> > This patch also makes it so that the boot script is executed one line at a time.
>
> This part below would be basically all that remains. Is this also equivalent to the previous patch to interpreter.fs for also handling \r the same as \n or does it do something else? If not then maybe the whole thing could be done in Forth entirely.

It does break up the script so that only one line is sent to feval() at a time.

[Programmingkid]

On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> On Tue, 26 Apr 2016, Programmingkid wrote:
> > This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> > > r and r> with sub_>r and sub_r>. This allows for the return stack to be left
> >
> > alone.
>
> Can we have Segher's Forth version of this instead:
>
> https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
>
> That should be equivalent and would make the C code much simpler and allow dropping most of these patches.

That code doesn't work. It causes a "Dictionary space overflow" message
to be printed. My code does work.

Here is my patch I made to test out that code. If you have the ability
please test out any changes before submitting any patches.

Index: arch/ppc/ppc.fs
===================================================================
--- arch/ppc/ppc.fs	(revision 1395)
+++ arch/ppc/ppc.fs	(working copy)
@@ -66,3 +66,11 @@

\ Set by BootX when booting Mac OS X
 defer spin
+
+hex
+20 cells CREATE stash  VARIABLE #stash  #stash off
+: stash-push  stash #stash cells + !  1 stash +! ;
+: stash-pop   -1 stash +!  stash #stash cells + @ ;
+: >r  state @ IF postpone >r EXIT THEN  stash-push ; immediate
+: r>  state @ IF postpone r> EXIT THEN  stash-pop ; immediate
+: r@  state @ IF postpone r@ EXIT THEN  stash-pop dup stash-push ; immediate
Index: libopenbios/bootinfo_load.c
===================================================================
--- libopenbios/bootinfo_load.c	(revision 1395)
+++ libopenbios/bootinfo_load.c	(working copy)
@@ -116,6 +116,33 @@
    return LOADER_NOT_SUPPORT;
 }

+/* Runs the bootscript one line at a time */
+void run_script(char *bootscript)
+{
+    int index = 0, buf_index = 0;
+    char c;
+    char *buffer = malloc(1000 * sizeof(char));
+    
+    while (1) {
+        c = bootscript[index];
+        
+        // fill up buffer
+        while (c != '\n' && c != '\r' && c != '\0') {
+            buffer[buf_index++] = c;
+            c = bootscript[++index];
+        }
+        buffer[buf_index] = '\0';
+        buf_index = 0;
+        index++;
+        printk("%s\n", buffer);
+        feval(buffer);
+        if (c == '\0') {
+            break;
+        }
+    }
+    free(buffer);
+}
+
 /*
   Parse SGML structure like:
   <chrp-boot>
@@ -262,7 +289,8 @@
    /* If the payload is bootinfo then we execute it immediately */
    if (scriptvalid) {
    	DPRINTF("bootscript: %s\n", bootscript);
-		feval(bootscript);
+		//feval(bootscript);
+        run_script(bootscript);
    }
    else
    	DPRINTF("Unable to parse bootinfo bootscript\n");

[BALATON Zoltan]

On Tue, 26 Apr 2016, Programmingkid wrote:
> On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> > On Tue, 26 Apr 2016, Programmingkid wrote:
> > > This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> > > > r and r> with sub_>r and sub_r>. This allows for the return stack to be left
> > >
> > > alone.
> >
> > Can we have Segher's Forth version of this instead:
> >
> > https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
> >
> > That should be equivalent and would make the C code much simpler and 
> > allow dropping most of these patches.
>
> That code doesn't work. It causes a "Dictionary space overflow" message
> to be printed. My code does work.

Maybe you need to increase the available cells to have enough space? I 
don't know much about Forth to understand the code better.
> Here is my patch I made to test out that code. If you have the ability
> please test out any changes before submitting any patches.
> Index: arch/ppc/ppc.fs
>
> --- arch/ppc/ppc.fs	(revision 1395)
> +++ arch/ppc/ppc.fs	(working copy)
> @@ -66,3 +66,11 @@
>
> \ Set by BootX when booting Mac OS X
> defer spin
>
> +hex
> +20 cells CREATE stash  VARIABLE #stash  #stash off
> +: stash-push  stash #stash cells + !  1 stash +! ;
> +: stash-pop   -1 stash +!  stash #stash cells + @ ;
> +: >r  state @ IF postpone >r EXIT THEN  stash-push ; immediate
> +: r>  state @ IF postpone r> EXIT THEN  stash-pop ; immediate
> +: r@  state @ IF postpone r@ EXIT THEN  stash-pop dup stash-push ; immediate

Actually what is the problem with the default implementations of these 
words that makes us need a replacement for them? Is it that the space they 
alloc from is too small and cannot be increased due to ROM limits? Can we 
change them to alloc from memory then or are they used while setting up 
memory?

I think the only difference with your patch is maybe that yours alloc from 
memory while these use some Forth space which may not be large enough, so 
probably the right fix could be increasing the Forth space by moving that 
to RAM instead of the limited ROM area or changing these words to move the 
stack to RAM when they run out of ROM space.

These all are just guessing so I might be completely off here though, I
haven't checked the code.

Also it worked with just adding \r as line terminator, so maybe the 
default implementation of the r-stack is OK and only the below part for 
line-by-line execution is needed? Have you tested that?

Regards,
BALATON Zoltan
> Index: libopenbios/bootinfo_load.c
>
> --- libopenbios/bootinfo_load.c	(revision 1395)
> +++ libopenbios/bootinfo_load.c	(working copy)
> @@ -116,6 +116,33 @@
>    return LOADER_NOT_SUPPORT;
> }
>
> +/* Runs the bootscript one line at a time */
> +void run_script(char *bootscript)
> +{
> int index = 0, buf_index = 0;
> char c;
> char *buffer = malloc(1000 * sizeof(char));
> while (1) {
>    c = bootscript[index];
>    // fill up buffer
>    while (c != '\n' && c != '\r' && c != '\0') {
>        buffer[buf_index++] = c;
>        c = bootscript[++index];
>    }
>    buffer[buf_index] = '\0';
>    buf_index = 0;
>    index++;
>    printk("%s\n", buffer);
>    feval(buffer);
>    if (c == '\0') {
>        break;
>    }
> }
> free(buffer);
>
> +}
>
> /*
>   Parse SGML structure like:
>
>   <chrp-boot>
> @@ -262,7 +289,8 @@
>    /* If the payload is bootinfo then we execute it immediately */
>    if (scriptvalid) {
>    	DPRINTF("bootscript: %s\n", bootscript);
> -		feval(bootscript);
> +		//feval(bootscript);
> +        run_script(bootscript);
>    }
>    else
>    	DPRINTF("Unable to parse bootinfo bootscript\n");

[Programmingkid]

On Apr 27, 2016, at 6:14 AM, BALATON Zoltan wrote:
> On Tue, 26 Apr 2016, Programmingkid wrote:
> > On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> > > On Tue, 26 Apr 2016, Programmingkid wrote:
> > > > This patch makes booting Mac OS 9.2 possible. It replaces all occurrences of
> > > > > r and r> with sub_>r and sub_r>. This allows for the return stack to be left
> > > >
> > > > alone.
> > >
> > > Can we have Segher's Forth version of this instead:
> > >
> > > https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
> > >
> > > That should be equivalent and would make the C code much simpler and allow dropping most of these patches.
> >
> > That code doesn't work. It causes a "Dictionary space overflow" message
> > to be printed. My code does work.
>
> Maybe you need to increase the available cells to have enough space? I don't know much about Forth to understand the code better.

That was my first thought. I did try to do just that. It didn't work. Found out it was a calculation problem that caused the error message.
> > Here is my patch I made to test out that code. If you have the ability
> > please test out any changes before submitting any patches.
> > Index: arch/ppc/ppc.fs
> >
> > --- arch/ppc/ppc.fs	(revision 1395)
> > +++ arch/ppc/ppc.fs	(working copy)
> > @@ -66,3 +66,11 @@
> >
> > \ Set by BootX when booting Mac OS X
> > defer spin
> >
> > +hex
> > +20 cells CREATE stash  VARIABLE #stash  #stash off
> > +: stash-push  stash #stash cells + !  1 stash +! ;
> > +: stash-pop   -1 stash +!  stash #stash cells + @ ;
> > +: >r  state @ IF postpone >r EXIT THEN  stash-push ; immediate
> > +: r>  state @ IF postpone r> EXIT THEN  stash-pop ; immediate
> > +: r@  state @ IF postpone r@ EXIT THEN  stash-pop dup stash-push ; immediate
>
> Actually what is the problem with the default implementations of these words that makes us need a replacement for them? Is it that the space they alloc from is too small and cannot be increased due to ROM limits? Can we change them to alloc from memory then or are they used while setting up memory?

The default implementation of the words appear to be fine.
> I think the only difference with your patch is maybe that yours alloc from memory while these use some Forth space which may not be large enough, so probably the right fix could be increasing the Forth space by moving that to RAM instead of the limited ROM area or changing these words to move the stack to RAM when they run out of ROM space.
>
> These all are just guessing so I might be completely off here though, I
> haven't checked the code.

Thank goodness we don't have to do this.
> Also it worked with just adding \r as line terminator, so maybe the default implementation of the r-stack is OK and only the below part for line-by-line execution is needed? Have you tested that?

I just did and it works. Mac OS 9.2 boots to the desktop. I only added the return stack replacement code because Mark said altering the return stack was not something that should be done. I guess it is up to him now to decide what code stays and what code goes.
> Regards,
> BALATON Zoltan
> > Index: libopenbios/bootinfo_load.c
> >
> > --- libopenbios/bootinfo_load.c	(revision 1395)
> > +++ libopenbios/bootinfo_load.c	(working copy)
> > @@ -116,6 +116,33 @@
> >    return LOADER_NOT_SUPPORT;
> > }
> >
> > +/* Runs the bootscript one line at a time */
> > +void run_script(char *bootscript)
> > +{
> > int index = 0, buf_index = 0;
> > char c;
> > char *buffer = malloc(1000 * sizeof(char));
> > while (1) {
> >    c = bootscript[index];
> >    // fill up buffer
> >    while (c != '\n' && c != '\r' && c != '\0') {
> >        buffer[buf_index++] = c;
> >        c = bootscript[++index];
> >    }
> >    buffer[buf_index] = '\0';
> >    buf_index = 0;
> >    index++;
> >    printk("%s\n", buffer);
> >    feval(buffer);
> >    if (c == '\0') {
> >        break;
> >    }
> > }
> > free(buffer);
> >
> > +}
> >
> > /*
> >  Parse SGML structure like:
> >
> >  <chrp-boot>
> > @@ -262,7 +289,8 @@
> >    /* If the payload is bootinfo then we execute it immediately */
> >    if (scriptvalid) {
> >    	DPRINTF("bootscript: %s\n", bootscript);
> > -		feval(bootscript);
> > +		//feval(bootscript);
> > +        run_script(bootscript);
> >    }
> >    else
> >    	DPRINTF("Unable to parse bootinfo bootscript\n");

[Mark Cave-Ayland]

On 27/04/16 11:14, BALATON Zoltan wrote:
> On Tue, 26 Apr 2016, Programmingkid wrote:
> > On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> > > On Tue, 26 Apr 2016, Programmingkid wrote:
> > > > This patch makes booting Mac OS 9.2 possible. It replaces all
> > > > occurrences of
> > > > > r and r> with sub_>r and sub_r>. This allows for the return stack
> > > > > to be left
> > > >
> > > > alone.
> > >
> > > Can we have Segher's Forth version of this instead:
> > >
> > > https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
> > >
> > > That should be equivalent and would make the C code much simpler and
> > > allow dropping most of these patches.
> >
> > That code doesn't work. It causes a "Dictionary space overflow" message
> > to be printed. My code does work.
>
> Maybe you need to increase the available cells to have enough space? I
> don't know much about Forth to understand the code better.

I think Segher's version is closer to what we are trying to achieve -
there is a lot of extra code added for something where the end result
isn't excessively context. Can you find out with the Forth debugger
exactly why Segher's patch is failing?

ATB,

Mark.

[Programmingkid]

On May 1, 2016, at 6:28 AM, Mark Cave-Ayland wrote:
> On 27/04/16 11:14, BALATON Zoltan wrote:
> > On Tue, 26 Apr 2016, Programmingkid wrote:
> > > On Apr 26, 2016, at 4:11 PM, BALATON Zoltan wrote:
> > > > On Tue, 26 Apr 2016, Programmingkid wrote:
> > > > > This patch makes booting Mac OS 9.2 possible. It replaces all
> > > > > occurrences of
> > > > > > r and r> with sub_>r and sub_r>. This allows for the return stack
> > > > > > to be left
> > > > >
> > > > > alone.
> > > >
> > > > Can we have Segher's Forth version of this instead:
> > > >
> > > > https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
> > > >
> > > > That should be equivalent and would make the C code much simpler and
> > > > allow dropping most of these patches.
> > >
> > > That code doesn't work. It causes a "Dictionary space overflow" message
> > > to be printed. My code does work.
> >
> > Maybe you need to increase the available cells to have enough space? I
> > don't know much about Forth to understand the code better.
>
> I think Segher's version is closer to what we are trying to achieve -
> there is a lot of extra code added for something where the end result
> isn't excessively context. Can you find out with the Forth debugger
> exactly why Segher's patch is failing?

I decided to recreate his patch. Here it is below:

Index: forth/lib/vocabulary.fs
===================================================================
--- forth/lib/vocabulary.fs	(revision 1395)
+++ forth/lib/vocabulary.fs	(working copy)
@@ -151,3 +151,46 @@
   ;

true to vocabularies?
+
+VARIABLE max-stack-size
+100 max-stack-size !
+CREATE stash-stack max-stack-size @ CELLS ALLOT \ make a stack that can hold CELL values
+VARIABLE stash-stack-pointer    \ Keeps track of top of stack
+0 stash-stack-pointer !         \ Set initial value to zero
+
+
+\ pushes a value on the stash stack
+: stash-push ( n -- ) 
+    \ check for overflow before pushing value
+    stash-stack-pointer @
+    max-stack-size @ 1-
+    >
+    if ." Warning: stack limit reached" cr EXIT then
+
+    \ ." current stack pointer value = " stash-stack-pointer @ .
+    stash-stack stash-stack-pointer @ CELLS + !  \ calculate address then store value
+    stash-stack-pointer @ 1 +     \ calculate new value for stack pointer
+    stash-stack-pointer !   \ increment the stack pointer
+;   
+
+\ pops a value from the stash stack
+: stash-pop ( -- n )
+    stash-stack-pointer @ 1 -   \ calculate new value for the stack pointer
+    dup ( new-stack-pointer new-stack-pointer )
+    0 ( new-stack-pointer new-stack-pointer 0)
+    < ( new-stack-pointer boolean )
+    if ( new-stack-pointer )
+        drop 0 ( 0 )
+        ." Warning: stack underflow detected" cr 
+    else 
+        ( new-stack-pointer )
+    then
+    stash-stack-pointer !   \ store new value in stack pointer
+    stash-stack-pointer @   \ get the value of stash-stack-pointer
+    CELLS stash-stack + @ \ calculate address then return value
+    \ ." current stack pointer value = " stash-stack-pointer @ .
+;
+
+: >r  state @ IF postpone >r EXIT THEN  stash-push ; IMMEDIATE
+: r>  state @ IF postpone r> EXIT THEN  stash-pop ; IMMEDIATE
+: r@  state @ IF postpone r@ EXIT THEN  stash-pop dup stash-push ; IMMEDIATE

The stash-push and stash-pop words do work. A simple test I did is this:

0 > 100 stash-push  ok
0 > 200 stash-push  ok
0 > 300 stash-push  ok
0 > stash-pop  ok
1 > showstack  ok
300 < 1 > stash-pop  ok
300 200 < 2 > stash-pop  ok
300 200 100 < 3 >

The end result was OpenBIOS not being able to boot Mac OS 9.2. Running the boot script one line at a time does work.

[Segher Boessenkool]

On Sun, May 01, 2016 at 08:27:59PM -0400, Programmingkid wrote:
> > I think Segher's version is closer to what we are trying to achieve -
> > there is a lot of extra code added for something where the end result
> > isn't excessively context. Can you find out with the Forth debugger
> > exactly why Segher's patch is failing?

It's not my patch that is failing, there is just something else than
before failing in the boot script, in a worse way it seems.
> I decided to recreate his patch. Here it is below:
> Index: forth/lib/vocabulary.fs
>
> --- forth/lib/vocabulary.fs	(revision 1395)
> +++ forth/lib/vocabulary.fs	(working copy)
> @@ -151,3 +151,46 @@
>    ;
>
> true to vocabularies?
>
> +VARIABLE max-stack-size
> +100 max-stack-size !

You forgot to make it hex, making it a much smaller number.  Not that
that matters too much here, twenty or so entries is more than most
things need.  Anyway, your Forth code is worse than your C, time to
learn a bit :-)

h# 100 CONSTANT max-stack-size

since it is a constant and not a variable.
> +: stash-push ( n -- )
> \ check for overflow before pushing value
> stash-stack-pointer @
> max-stack-size @ 1-
> >

Just use  >=  .
> if ." Warning: stack limit reached" cr EXIT then

That is not going to end well.  Warning and then ignoring an error
does not help much.  You also forgot to drop the value to push.
Just use  ABORT"  :

stash-stack-pointer ( not a pointer at all, it's an array index)
   max-stack-size >= ABORT" Oh bah, we blew our stack"
> stash-stack-pointer @ 1 +     \ calculate new value for stack pointer
> stash-stack-pointer !   \ increment the stack pointer

1 stash-stack-pointer +!

"1 +" is usually written "1+" as well, fwiw.  It is faster on some
systems, but more importantly, it is more idiomatic, easier to read.
> +: stash-pop ( -- n )
> stash-stack-pointer @ 1 -   \ calculate new value for the stack pointer
> dup ( new-stack-pointer new-stack-pointer )
> 0 ( new-stack-pointer new-stack-pointer 0)
> < ( new-stack-pointer boolean )
> if ( new-stack-pointer )
>    drop 0 ( 0 )
>    ." Warning: stack underflow detected" cr 
> else
>    ( new-stack-pointer )
> then
> stash-stack-pointer !   \ store new value in stack pointer

So, it's fine to decrement it and store it, and only then check it
(also note how to check for negative):

-1 stash-stack-pointer +!
   stash-stack-pointer @ 0<
      ABORT" whoops we drained the stack a bit too much"

Cheers,

Segher

[Segher Boessenkool]

[ Wow, ancient thread -- I'm cleaning up my mailboxes, sorry ]

On Tue, Apr 26, 2016 at 05:53:37PM -0400, Programmingkid wrote:
> > Can we have Segher's Forth version of this instead:
> >
> > https://www.coreboot.org/pipermail/openbios/2016-April/009338.html
> >
> > That should be equivalent and would make the C code much simpler and allow dropping most of these patches.
>
> That code doesn't work. It causes a "Dictionary space overflow" message
> to be printed. My code does work.
>
> Here is my patch I made to test out that code. If you have the ability
> please test out any changes before submitting any patches.
> Index: arch/ppc/ppc.fs
>
> --- arch/ppc/ppc.fs	(revision 1395)
> +++ arch/ppc/ppc.fs	(working copy)
> @@ -66,3 +66,11 @@
>
> \ Set by BootX when booting Mac OS X
>  defer spin
>
> +hex
> +20 cells CREATE stash  VARIABLE #stash  #stash off
> +: stash-push  stash #stash cells + !  1 stash +! ;

1 #stash +!
> +: stash-pop   -1 stash +!  stash #stash cells + @ ;

Here, too.

Segher