TinyScheme

On Saturday 19 January 2008 14:49, Ray Lehtiniemi wrote:

> under guile, i get the expected infinite stream of "foo!!".  using
> tinyscheme 1.38 i get 4 "foo!!", which also appears incorrect.

upon further investigation, the change from 1.38 behavior was introduced by 
"Fix for bug 1589711" on Fri Feb 23 23:04:02 2007.

the new code does not abort the (loop), thus fixing the reported bug.  
however, it does not seem to process the (load) properly.  what happens is 
that it loops continuously calling file_push(), but never processing the 
loaded file and never calling file_pop().  this continues until you run out 
of file descriptors, then it dies.

replace foo.scm with this version to see this more clearly:

# cat foo.scm
(load "bar.scm")
(let ()
  (load "bar.scm"))
(let loop ()
  (load "bar.scm"))
(let loop ()
  (write "xxx")
  (load "bar.scm")
  (loop))


ray

>
> hi
>
> i'm evaluating tinyscheme and have noticed a problem.  the following
> failure
> happens using the latest CVS.

Ray, thanks for the bug report.  I'm looking at the code now.

Tom

On Monday 21 January 2008 12:49, Tom Breton (Tehom) wrote:
> > hi
> >
> > i'm evaluating tinyscheme and have noticed a problem.  the following
> > failure
> > happens using the latest CVS.
>
> Ray, thanks for the bug report.  I'm looking at the code now.
>
> Tom

great, thanks tom!

i also noticed that this test case exposes a bug in file_push().  it fails to 
check file_i against MAXFIL, thus corrupting the struct scheme if enough 
files are opened.  something like this patch might be good to add...

commit 78959c8863f1425e17aa11f48857f320fc147335
Author: Ray Lehtiniemi <rayl@...>
Date:   Sat Jan 19 23:46:23 2008 -0700

    Check for load_stack[] overflow.
    
    Signed-off-by: Ray Lehtiniemi <rayl@...>

diff --git a/scheme.c b/scheme.c
index eb1ea16..93ab68a 100644
--- a/scheme.c
+++ b/scheme.c
@@ -1323,7 +1323,11 @@ static void finalize_cell(scheme *sc, pointer a) {
 /* ========== Routines for Reading ========== */
 
 static int file_push(scheme *sc, const char *fname) {
-  FILE *fin=fopen(fname,"r");
+  FILE *fin = NULL;
+
+  if (sc->file_i == MAXFIL-1)
+     return 0;
+  fin=fopen(fname,"r");
   if(fin!=0) {
     sc->file_i++;
     sc->load_stack[sc->file_i].kind=port_file|port_input;

Thanks for the patch, Ray.  I committed it just now.

The load problem is a timing problem.  TS does get around to eval'ing
bar.scm, it just doesn't do it until the form that called `load' has
finished.  This demonstrates it:

(let loop ((n 0))
  (write "xxx")
  (load "bar.scm")
  (if (< n 8) (loop (+ 1 n))))

I'm going to look at it further.  I'm afraid that the fix may require
splitting the top-level case OP_T0LVL, ie adding another operator in
op-defines.h  That's more adventurous than I would prefer in a project
like this, but making it load right is important.

        Tom

On Mon, 2008-01-21 at 17:15 -0500, Tom Breton (Tehom) wrote:

> I'm going to look at it further.  I'm afraid that the fix may require
> splitting the top-level case OP_T0LVL, ie adding another operator in
> op-defines.h  That's more adventurous than I would prefer in a project
> like this, but making it load right is important.

That will almost certainly be required. While you are there, check that
OP_T0LVL isn't resetting the eval stack under the (mistaken) assumption
that LOAD is being run from top level. At one point there was an error
of this form in the logic of the machine. It may have subsequently been
fixed, or I may be mis-recollecting where it was.

However, note that there is an inherent ambiguity about what LOAD means
when it is called from any place that is not top level. Specifically:
the LOAD will internally execute a REPL loop, and it is not obvious what
environment that REPL loop runs in? In particular, does it's environment
(if not specified explicitly as an argument to LOAD) include any
non-globals that were bound in the procedure calling LOAD?

I don't know the answer, and the standard does not take a position.


shap

On Monday 21 January 2008 15:34, Jonathan S. Shapiro wrote:
> On Mon, 2008-01-21 at 17:15 -0500, Tom Breton (Tehom) wrote:
> > I'm going to look at it further.  I'm afraid that the fix may require
> > splitting the top-level case OP_T0LVL, ie adding another operator in
> > op-defines.h  That's more adventurous than I would prefer in a project
> > like this, but making it load right is important.
>
> That will almost certainly be required. While you are there, check that
> OP_T0LVL isn't resetting the eval stack under the (mistaken) assumption
> that LOAD is being run from top level. At one point there was an error
> of this form in the logic of the machine. It may have subsequently been
> fixed, or I may be mis-recollecting where it was.

i believe this may in fact be happening.  the OP_T0LVL handler will 
dump_stack_reset(), which appeared (to my uneducated eyes) to be a big part 
of the problem.


i had tried splitting up the OP_T0LVL into OP_T0ALVL and OP_T0BLVL, but i 
don;t really know what i'm doing.... i think i managed to get it to read the 
first form out of the file by saving an OP_READ_INTERNAL on the stack, but 
then it returned back to the top level and i couldn;t figure out how to make 
it keep going :-(



> However, note that there is an inherent ambiguity about what LOAD means
> when it is called from any place that is not top level. Specifically:
> the LOAD will internally execute a REPL loop, and it is not obvious what
> environment that REPL loop runs in? In particular, does it's environment
> (if not specified explicitly as an argument to LOAD) include any
> non-globals that were bound in the procedure calling LOAD?
>
> I don't know the answer, and the standard does not take a position.
>
>
> shap
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> Tinyscheme-issues mailing list
> Tinyscheme-issues@...
> https://lists.sourceforge.net/lists/listinfo/tinyscheme-issues

On Mon, 2008-01-21 at 15:47 -0700, Ray Lehtiniemi wrote:
> > That will almost certainly be required. While you are there, check that
> > OP_T0LVL isn't resetting the eval stack under the (mistaken) assumption
> > that LOAD is being run from top level. At one point there was an error
> > of this form in the logic of the machine. It may have subsequently been
> > fixed, or I may be mis-recollecting where it was.
> 
> i believe this may in fact be happening.  the OP_T0LVL handler will 
> dump_stack_reset(), which appeared (to my uneducated eyes) to be a big part 
> of the problem.

Yes. That is precisely the issue that I remembered.

> i had tried splitting up the OP_T0LVL into OP_T0ALVL and OP_T0BLVL, but i 
> don;t really know what i'm doing.... i think i managed to get it to read the 
> first form out of the file by saving an OP_READ_INTERNAL on the stack, but 
> then it returned back to the top level and i couldn;t figure out how to make 
> it keep going :-(

Yes. The problem will get even messier if you contrive to have C call
back into Scheme, because you will really want to kick off OP_T0LVL
there as well.

I had this one fixed at one point, but then I decided that I really
wanted bignum support. This led me into the GC system and the error
stream problem, and then somewhere along the way I realized that PRINT
would probably fail if GC was triggered from inside it (which became
possible when I introduced string streams).

This in turn is part of why I started down the preamble rat-hole,
because a correct implementation of PRINT is necessarily recursive, and
getting it all to work under GC looked VERY difficult.

Pretty soon I was doing a ground-up rewrite, because once I started to
tackle the GC issue I found some more issues. I will send a note in a
moment outlining how to improve on the current situation in a simple way
that I did not understand at that time.

shap

On Monday 21 January 2008 16:03, Jonathan S. Shapiro wrote:

> Pretty soon I was doing a ground-up rewrite

hmm.  so, cutting right to the chase here... is the current codebase stable 
enough to be used for simple scripting of firmware self-tests and upgrades in 
an embedded device?  so far, i've been getting a real happy feeling while 
playing with the code, and i hope i'm not overlooking something important...

thanks!
ray

On Mon, 2008-01-21 at 20:13 -0700, Ray Lehtiniemi wrote:
> On Monday 21 January 2008 16:03, Jonathan S. Shapiro wrote:
> 
> > Pretty soon I was doing a ground-up rewrite
> 
> hmm.  so, cutting right to the chase here... is the current codebase stable 
> enough to be used for simple scripting of firmware self-tests and upgrades in 
> an embedded device?  so far, i've been getting a real happy feeling while 
> playing with the code, and i hope i'm not overlooking something important...

I think so. What I ran into was a case of cascading improvements. I
decided to add two "simple" things to the interpreter to increase
standard compliance: BigNums and string-streams. Both ran afoul of the
assumptions of the implementation.

The two existing bits of code that I would inspect with aggressive
distrust are the implementation of PRINT and the top-level LOAD logic.
You already ran in to the latter case.

shap

> On Mon, 2008-01-21 at 17:15 -0500, Tom Breton (Tehom) wrote:
>
>> I'm going to look at it further.  I'm afraid that the fix may require
>> splitting the top-level case OP_T0LVL, ie adding another operator in
>> op-defines.h  That's more adventurous than I would prefer in a project
>> like this, but making it load right is important.

I have some semblance of an answer now.  The hairiest problem is that
inchar can be called after it has encountered EOF but before the read
operators have reacted, for instance within "token" after "skipspace"
encounters EOF.
This can cause input to wrongly try to resume reading the first file while
still parsing the second, and if it reads past the end of that too, it
will call file_pop much too early.

Also, file_pop has no way to communicate that it has popped past the first
file, since setting sc->file_i to -1 causes problems (in fact, segfaults).
 Several pieces of code, for instance some contingent calls to OP_QUIT,
try to distinguish true quit from EOF on load, but lacking the actual
information, they guess on the basis of whether loadport is the same as
inport.

And as Ray showed, load wasn't causing evals.  It appeared to be, but only
because the real top-level loop *outside* of load handled those evals.

These problems somewhat cancelled each other, but caused the subtle bugs
reported.

My code is in a bit of a messy state right now, but I have Ray's example
and my own doing the right thing.  I'll clean it up and commit it.

Basically what I'm doing is:

Change the logic of OP_T0LVL so that each call to load has its own read
loop which ends when the file has been read, and interactive top level is
different.

Add to scheme a flag (top_EOF) indicating whether it has seen EOF for the
topmost file.

Make all the direct and indirect calls to inchar check for EOF and react
accordingly.


> That will almost certainly be required. While you are there, check that
> OP_T0LVL isn't resetting the eval stack under the (mistaken) assumption
> that LOAD is being run from top level. At one point there was an error
> of this form in the logic of the machine. It may have subsequently been
> fixed, or I may be mis-recollecting where it was.

Yes, it was, and since I have to change OP_T0LVL anyways, I'll fix it.

> However, note that there is an inherent ambiguity about what LOAD means
> when it is called from any place that is not top level. Specifically:
> the LOAD will internally execute a REPL loop, and it is not obvious what
> environment that REPL loop runs in? In particular, does it's environment
> (if not specified explicitly as an argument to LOAD) include any
> non-globals that were bound in the procedure calling LOAD?
>
> I don't know the answer, and the standard does not take a position.

I don't know, but what makes sense to me is to use the environment load
was called in.

Tom

On Mon, 2008-01-21 at 22:40 -0500, Tom Breton (Tehom) wrote:
> > However, note that there is an inherent ambiguity about what LOAD means
> > when it is called from any place that is not top level. Specifically:
> > the LOAD will internally execute a REPL loop, and it is not obvious what
> > environment that REPL loop runs in? In particular, does it's environment
> > (if not specified explicitly as an argument to LOAD) include any
> > non-globals that were bound in the procedure calling LOAD?
> >
> > I don't know the answer, and the standard does not take a position.
> 
> I don't know, but what makes sense to me is to use the environment load
> was called in.

Actually, I think that this is exactly the wrong thing to do. In the
absence of specification, LOAD should be called with the TOP-LEVEL
environment, not the current lexical environment.

Rationale:

The in-procedure case introduces misleading complexity. Calling load
form a procedure is generally a mistake. The cases we want to consider
are of the form:

  (load "a.scm")
  (load "b.scm") ; must be able to reference results of a.scm
  (if (test) (load "c.scm) #f)

If you see a load within a procedure of the form

  (load "x.scm" (current-environment))

well, then, the user said "current lexical env" and fuck 'em if they
can't take a joke. But in the absence of an argument I claim that the
local ENV must NOT be captured "by magic". If you accept this, then the
two credible rewritings are:

  (load "x.scm") => (load "x.scm" (r5rs-environment))
  (load "x.scm") => (load "x.scm" (top-level-environment))

I claim that the second is the correct interpretation, because otherwise
we must conclude that

  (load "a.scm") (load "b.scm") =>
    (load "a.scm" (r5rs-environment))
    (load "b.scm" (r5rs-environment))

and it would follow generally that later loads cannot use the results of
earlier loads -- which is obviously not what we intend.


shap

On Monday 21 January 2008 20:37, Jonathan S. Shapiro wrote:

> I think so. What I ran into was a case of cascading improvements. I
> decided to add two "simple" things to the interpreter to increase
> standard compliance: BigNums and string-streams. Both ran afoul of the
> assumptions of the implementation.

ok, that makes me feel better.


> The two existing bits of code that I would inspect with aggressive
> distrust are the implementation of PRINT and the top-level LOAD logic.
> You already ran in to the latter case.

heh!  and my next question was going to be about OP_P0LIST :-)

if i run aubrey jaffer's test.scm, it seems to work.... unless i run it under 
valgrind.  while running the "tmp1" results checker for SECTION(6 10 3), 
valgrind indicates that the first "if (is_vector(sc->args))" under OP_P0LIST 
is producing a segfault.  it _seems_ to work fine, though, even under 
electricfence, so i'm not sure how seriously i should take this... i've used 
valgrind with mixed results in the past.

ray

On Monday 21 January 2008 20:40, Tom Breton (Tehom) wrote:

> My code is in a bit of a messy state right now, but I have Ray's example
> and my own doing the right thing.  I'll clean it up and commit it.

sweet, looking forward to testing it!

will this also fix the problem where you (usually) have to hit Ctrl-D _twice_ 
to exit the top level reader?

thanks
ray

On Mon, 2008-01-21 at 21:41 -0700, Ray Lehtiniemi wrote:

> > The two existing bits of code that I would inspect with aggressive
> > distrust are the implementation of PRINT and the top-level LOAD logic.
> > You already ran in to the latter case.
> 
> heh!  and my next question was going to be about OP_P0LIST :-)

Ray:

Offhand, that one doesn't stir any memories, but I'm dragging my memory
back from about four years ago, so don't take my memory faults too
seriously. :-)

shap

While fixing this bug, I notice that TS does file loading in an
unneccessarily error-prone way.

inchar is just the wrong place to pop to the next enclosing file
(file_pop).  After it does so, inchar is always at risk for reading
the wrong file.

ISTM all inchar should do on EOF is stop trying to read until
something else deals with the situation.  ISTM the right place to
control resuming reading the next enclosing file is the file-eval
loop, OP_T0LVL.

So I'd like to:

Remove (so soon!) my new flag top_EOF and replace it with a new value
in enum scheme_port_kind, port_saw_EOF, that marks a port as having
reached its end.

Add a set/clear/check interface.

Let inchar first check that flag.

Let inchar, on EOF, just set that flag and no longer file_pop.

It falls out that the flag is clear on port initialization.

Let the file-eval loop (OP_T0LVL) check whether this is true of
current loadport and if so, call file_pop.  Furthermore, on
finding EOF at top level (sc->file_i == 0), call OP_QUIT

Remove the code that this replaces.

	Tom Breton (Tehom)

I was able to keep OP_T0LVL as a single operation, but I'm not sure
whether it makes sense to.  It does two semi-related things, loop (one
loop that functions as either REPL loop or file read-eval loop) and
reset potential bad situations the user may have gotten trapped in,
such as restoring the global environment.

Historically one op had to do all of it because there existed only a
top-level loop, but that's no longer the case.

ISTM it's more coherent to have one that does REPL and situation-reset
and one that does file read-eval (call it OP_FILE_TPLVL ?)

One tiny hazard is that scheme_load_file has to distinguish which loop
to start, but it does already know whether it's launching a REPL

The added flexibility might make sense.  For instance, C might want to
launch a REPL that didn't use stdin.

Any objections to separating the two?

	Tom Breton (Tehom)

On Tue, 2008-01-22 at 22:49 -0500, Tom Breton (Tehom) wrote:
> While fixing this bug, I notice that TS does file loading in an
> unneccessarily error-prone way.
> 
> inchar is just the wrong place to pop to the next enclosing file
> (file_pop).  After it does so, inchar is always at risk for reading
> the wrong file.
> 
> ISTM all inchar should do on EOF is stop trying to read until
> something else deals with the situation.  ISTM the right place to
> control resuming reading the next enclosing file is the file-eval
> loop, OP_T0LVL.

I concur, but I think this isn't the whole story. What I think we need
is a low-level version of IN-CHAR that is able to return either a char
or the universally unique EOF object. Once EOF is reached, subsequent
calls to PRIM-IN-CHAR on the same stream will return EOF.

In a scheme having exceptions one could use exceptions, but within the
interpreter my proposal above is probably simpler.

> Remove (so soon!) my new flag top_EOF and replace it with a new value
> in enum scheme_port_kind, port_saw_EOF, that marks a port as having
> reached its end.

I think you are saying that a port which has reached EOF "becomes" (in
the smalltelk sense) an "EOF port" internally to the implementation?
That is: the port type changes "in place", and any associated external
file descriptor gets closed when EOF is encountered.

> Add a set/clear/check interface.

Why is this desirable?

> Let inchar first check that flag.

Ultimately, IN-CHAR needs to dispatch on port type to decide what to do,
so I'm not convinced that a check is required here.



shap

On Tue, 2008-01-22 at 23:07 -0500, Tom Breton (Tehom) wrote:
> I was able to keep OP_T0LVL as a single operation, but I'm not sure
> whether it makes sense to.  It does two semi-related things, loop (one
> loop that functions as either REPL loop or file read-eval loop) and
> reset potential bad situations the user may have gotten trapped in,
> such as restoring the global environment.
> 
> Historically one op had to do all of it because there existed only a
> top-level loop, but that's no longer the case.

When I was screwing around with adding exceptions (which, note, are
coming in R6RS anyway, I concluded that the "right" strategy was for
eval_cycle to introduce a CATCH context on the eval stack and then
simply run a normal REPL loop. If errors RAISE, then the funny cases in
OP_T0LVL all go away, and the REPL case becomes completely regular.

shap

> On Tue, 2008-01-22 at 22:49 -0500, Tom Breton (Tehom) wrote:
>> While fixing this bug, I notice that TS does file loading in an
>> unneccessarily error-prone way.
>>
>> inchar is just the wrong place to pop to the next enclosing file
>> (file_pop).  After it does so, inchar is always at risk for reading
>> the wrong file.
>>
>> ISTM all inchar should do on EOF is stop trying to read until
>> something else deals with the situation.  ISTM the right place to
>> control resuming reading the next enclosing file is the file-eval
>> loop, OP_T0LVL.
>
> I concur, but I think this isn't the whole story. What I think we need
> is a low-level version of IN-CHAR that is able to return either a char
> or the universally unique EOF object. Once EOF is reached, subsequent
> calls to PRIM-IN-CHAR on the same stream will return EOF.

OK, that's basically what I meant by "stop trying to read".

> In a scheme having exceptions one could use exceptions, but within the
> interpreter my proposal above is probably simpler.

I agree.

>> Remove (so soon!) my new flag top_EOF and replace it with a new value
>> in enum scheme_port_kind, port_saw_EOF, that marks a port as having
>> reached its end.
>
> I think you are saying that a port which has reached EOF "becomes" (in
> the smalltelk sense) an "EOF port" internally to the implementation?
> That is: the port type changes "in place", and any associated external
> file descriptor gets closed when EOF is encountered.

Almost. scheme_port_kind is a bunch of bitflags, not a set of types, so
this doesn't change the port's type.  It just signals that there is no
more to read, mostly for the load loop's benefit.

>> Add a set/clear/check interface.
>
> Why is this desirable?

Because otherwise functions that should not be concerned with port's
internals have to navigate thru cell/port/file port fields.

Just to be clear, I'm talking about a C interface along the lines of
is_mark, setmark, clrmark, not a Scheme interface to this.


>> Let inchar first check that flag.
>
> Ultimately, IN-CHAR needs to dispatch on port type to decide what to do,
> so I'm not convinced that a check is required here.

See above, scheme_port_kind is flags.

That reminds me of a related concern:  basic_inchar assumes that every
port that's not a file is a string, which is not true.  AFAICT nothing
actually passes a freed port to basic_inchar, so no bugs have bitten us on
that count, but it's still a bug waiting to happen.  I'd like to make it
explictly check for port_string and give it a default case.  I'm not sure
what to do for the port_srfi6 flag because I'm not familiar with it.

        Tom

On Wed, 2008-01-23 at 13:57 -0500, Tom Breton (Tehom) wrote:
> Almost. scheme_port_kind is a bunch of bitflags, not a set of types, so
> this doesn't change the port's type.  It just signals that there is no
> more to read, mostly for the load loop's benefit.

Agreed. I remember while introducing string streams concluding that
scheme_port_kind wasn't very well thought out.

It is not clear in hindsight how many of my issues were real, and how
many merely reflected differences in taste about implementing such
things.

> That reminds me of a related concern:  basic_inchar assumes that every
> port that's not a file is a string, which is not true.  AFAICT nothing
> actually passes a freed port to basic_inchar, so no bugs have bitten us on
> that count, but it's still a bug waiting to happen.  I'd like to make it
> explictly check for port_string and give it a default case.  I'm not sure
> what to do for the port_srfi6 flag because I'm not familiar with it.

Two possible solutions suggest themselves:

1. Introduce a new distinguished value for malformed arguments.

2. Implement RAISE.

On the subject of RAISE, this isn't at all difficult. What I did on the
in-progress version was to introduce a new type of framus that could sit
on the eval stack. The opcode was TRY and it's sole argument was a thunk
to which the raised object would be passed. On normal return, the
returned value simply bypasses the TRY frame and returns in the normal
way. Whenever an exception is raised, you walk up the eval stack looking
for the nearest enclosing OP_TRY and change it to OP_RAISED, then reset
the eval stack appropriately.

Or some such. That's not going to be right on the details, but it will
give you the idea.

> While fixing this bug, I notice that TS does file loading in an
> unneccessarily error-prone way.

OK, that's done.  A few notes:

We were writing a second prompt in file_pop because of top loop oddness. 
No longer needed and in fact confusing now that we do it right, it's gone.

Didn't add the set/clear/check because I would have only used it in one
place.

Definitely terminates on a single ^D now.

It now reports the result of a form containing a load, generally <#EOF>
from the load itself.  That's correct, but it may look funny because it
didn't before.

Tom

On Thursday 24 January 2008 18:45, Tom Breton (Tehom) wrote:
>
> OK, that's done.

works great.  thanks for the speedy fixes!

ray