zevv / zForth

zForth: tiny, embeddable, flexible, compact Forth scripting language for embedded systems
MIT License
355 stars 48 forks source link

include triggers ZF_ABORT_BUSY #21

Closed tius2000 closed 3 years ago

tius2000 commented 3 years ago

Dear @zevv,

I just started playing with zForth, seems like big fun! However, while implementing include. I ran into a problem. When calling include interactively I get an error ZF_ABORT_BUSY. It looks like _zfeval is not reentrant. Am I doing something wrong?

With kind regards, Matthias

zevv commented 3 years ago

HI Matthias,

I'm not exactly sure what you are trying to do - is your include implementation any different from the example implementation in src/linux/main.c? I recommend to take a peek at that implementation first: system call ZF_SYSCALL_USER + 2 implements this, and calls the include() function from the same source file.

tius2000 commented 3 years ago

I think my implantation matches the example implementation in src/linux/main.c. I also run a simple test with the linux sample file (with minimal changes to compile in under win32):

E:\zForth-master\src\linux>zforth
: .       1 sys ;

42 .
42

: include 130 sys ;

include ../../forth/core.zf
../../forth/core.zf:1: unknown error (12)

It seems to me, that using include with _zfeval triggers an error ZF_ABORT_BUSY (12).

zevv commented 3 years ago

Right, this is embarrassing: there were some changes merged in early may that simply broke include; It seems I actually never ran these and executed the (pretty limited) tests, so I never noticed.

The culprit seems to be commit 19c6c6b9069e73e1cb6b949daafa496b4bcd7903, I think we need to revert this for now. maybe @rleigh-lumiradx can shed some light on this?

rleigh-lumiradx commented 3 years ago

Yes, the commit you mentioned had a specific purpose: to prevent re-entrancy and corrupting the state of a currently-running program by starting to evaluate another when already busy. However, I didn't consider the case for include, and this may well be a special-case where we want to permit re-entrancy?

If re-entrancy is a requirement for include to work, then I think my change is clearly not the correct behaviour. Perhaps we need to either

zevv commented 3 years ago

Right; thanks for jumping in @rleigh-lumiradx.

My proposal would be to just drop 19c6c6b for now, if we ever need to protect against reentrency we can overthink the proper solution by then.

Maybe I should spend some time whipping up some CI and unit tests as well...

rleigh-lumiradx commented 3 years ago

That sounds perfectly reasonable.

tius2000 commented 3 years ago

Thanks to both of you!

zevv commented 3 years ago

22 is on the way to get merged