Re: Curious "buffer_full" line within host_execute

[David Malcolm]

On Fri, 2014-10-10 at 17:51 -0400, David Malcolm wrote:
> On Fri, 2014-10-10 at 12:26 -0400, David Malcolm wrote:
> > On Fri, 2014-10-10 at 08:03 -0400, David Malcolm wrote:
> > > [replying on the mailing list, with Ben's permission to quote his
> > > initial off-list msg]
> > > 
> > > On Fri, 2014-10-10 at 10:34 +1100, Ben Elliston wrote:
> > > > Hi David,
> > > > 
> > > > You're right that it looks like it should be full_buffer, not
> > > > buffer_full.  What happens if you fix that?
> > > 
> > > Hacking in a "-d" into runtest's invocation of expect, I see this in the
> > > log (the 1st time through):
> > > 
> > >   expect: does "" (spawn_id exp0) match glob pattern "buffer_full"? no
> > > 
> > > i.e. it does indeed treat "buffer_full" as a glob pattern, rather than a
> > > keyword.
> > > 
> > > Fixing the spelling to "full_buffer" doesn't fix the issue (log with -d
> > > attached)
> > > 
> > > However, moving the "expect_before" line to *after* the "spawn"
> > > invocation does: is the "expect_before" line too early?
> > > 
> > > My current belief here is that it's picking up the wrong spawn:
> > > "spawn_id exp0" doesn't seem to be the right thing for it to be
> > > listening on (am I right in thinking that's the "default" stdin/stout,
> > > rather than the not-yet-spawned program?)
> > > 
> > > Have been attempting to come up with a more minimal reproducer, but
> > > failing so far - some kind of heisenbug timing needed to expose this
> > > issue, perhaps?  Though I reliably hit it with my current setup, with:
> > >   make check-gcc check-jit -j2
> > > 
> > > [my copy of "Exploring Expect" is due for delivery at some point today,
> > > so may have more conceptual grounding in this in a day
> > > or so].
> > 
> > I haven't been able to come up with a more minimal reproducer, but for
> > reference, here's the patch I'm applying locally that fixes the problem.
> > 
> >     * lib/dejagnu.exp (host_execute): Move the expect_before to
> >     directly after the "spawn", and use "full_buffer", which is an
> >     expect keyword rather than "buffer_full", which isn't.
> 
> Sorry about this; it turns out *not* to fix my problem; when I remove
> all my debug prints, I start getting buffer_full errors. Will try to
> debug further.

Having spent the weekend reading "Exploring Expect" and experimenting
(newbie caveats apply!) I now think there are four issues here:

(A) the typo of the "full_buffer" keyword as "buffer_full".

(B) the placement of the "expect_before" *before* the "spawn".  From my
reading of the bottom of p241 of "Exploring Expect" AIUI, this means
that in the expect_before that "spawn_id" is searched for first in local
scope... it doesn't find it, then in global scope... it may or may not
find it, and associates the event with an unrelated spawn (which in my
case has been closed, hence a fatal error).

(C) "expect_before" may have too high a priority.  As noted in the patch
above I tried placing the "expect_before" directly after the "spawn".
This worked somewhat better: it's associated the full_buffer with the
correct spawn_id that way, but I started get numerous full_buffer
errors.  I think the issue there was that doing it with *_before* is
incorrect: if the spawned process buffers stdout, and writes, say 4096
bytes of data containing results, that's going to be bigger than
Expect's default match_max buffer size of 2000 bytes, and hence we
immediately have a full_buffer event.  Having it as "expect_before"
means that event is handled before any of the "PASSED" glob parsing, so
despite the buffer containing lots of meaningful results, the
prioritization of full_buffer means it simply errors out.  If we want to
fail if the buffer overflows, it may make sense to instead have:

   expect_after full_buffer { error "Buffer full" }

directly after the "spawn" (seems to work for my testsuite), or simply
to list it at the *end* of the the main "expect" statement, so that it
has lowest priority of the available matches.

(D) Zombies and checking the exit code.   There's no "wait" at the end
of host_execute, and spawn_id is a local within host_execute.  Hence
there doesn't seem to be a good way to reap the spawned process, and for
the caller to verify any constraints it wants to impose (in my case, I
want to check for a clean exit with an exit code of 0).  I *think* this
can be achieved by adding something like this to the top of
host_execute:

   upvar host_execute_spawn_id spawn_id

so that the spawn_id is written into the calling scope as
host_execute_spawn_id and can be "wait"-ed on and checked there.  That
said, I'm an expect neophyte and this is untested (the line above is
based on p244 of "Exploring Expect").

FWIW, for now, I'm attempting to fix my testsuite by including a patched
copy of host_execute, and simply removing the bogus line:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5531d1df2ab9f50c8be72271c6c74ed3757c79db


Hope this is helpful
Dave