Re: Interrupting globs (Re: Something rotten in tar completion)

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

Bart Schaefer wrote:
> On Dec 7,  5:07pm, Peter Stephenson wrote:
> }
> } Other issues:
> } 
> } Mikael needs to ^C three times to get back to the command line ---
> } apparently related to the code from Sven I took at its word.
> 
> (Sven is Oberon?  If that's true, I'd entirely forgotten it.)

I'm pretty sure I didn't make that up.
 
> } I agree we might want to look at some always blocks.  BTW, I didn't
> } actually think through the handling of "always" blocks for this patch,
> } so it might want looking at.
> 
> In particular with respect to always-blocks, there should be some way
> for TRY_BLOCK_ERROR to clear interrupts as well as (maybe even instead
> of?  TRY_BLOCK_INTERRUPT?) internal errors.  A trap that returns zero
> only clears interrupt if there isn't another trap in a deeper scope.

Yes, probably.  I realised we'd better not do that with bits in the shell
code as the documentation suggests setting TRY_BLOCK_ERROR to 0 clears
errors.  We'd want clearing interrupts to be a separate explicit action.

> Maybe TRY_BLOCK_ERROR does still clear both in the patch, I didn't
> parse it that carefully.

The current code causes ERRFLAG_INT to propagate and only updates
ERRFLAG_ERROR owing to TRY_BLOCK_ERROR.  I suppose that's correct for
the basic operation, i.e. unless we do some something special as above
interrupts propagate.

However, I think it definitely needs to clear ERRFLAG_INT for the
duration of the always clause, or the code won't execute at all, even if
it restores it later.  The patch below does this and notes the other issue.

> } It's also just occurred to me I may have introduced some rare but
> } entirely possible read-modify-write races because we set the ERRFLAG_INT
> } bit in interrupts and set the other and clear both bits separately in
> } the main shell.  I guess it would be better to queue interrupts whenever
> } we add or remove a single bit of errflag
> 
> I don't think single bits are a problem because it's always done with
> bitwise-or.  If anything, we'd need to queue any time we clear the
> interrupt bit, so that a signal that comes in while we're clearing it
> gets treated as if arrived after clearing.  In practice I don't think
> it makes that much difference.

I'm worried about the various occurrences of this:

	    /* Keep any user interrupt error status */
	    errflag = oef | (errflag & ERRFLAG_INT);

which can cause

- read errflag to determine ERRFLAG_INT bit; find it's not set
- interrupt sets ERRFLAG_INT
- errflag gets set to value without ERRFLAG_INT.

It's a narrow race, but I think it's a real one.

pws

diff --git a/Src/loop.c b/Src/loop.c
index 69805ea..9b0a8d7 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -643,7 +643,7 @@ exectry(Estate state, int do_exec)
 {
     Wordcode end, always;
     int endval;
-    int save_retflag, save_breaks, save_contflag;
+    int save_retflag, save_breaks, save_contflag, try_interrupt;
     zlong save_try_errflag, save_try_tryflag;
 
     end = state->pc + WC_TRY_SKIP(state->pc[-1]);
@@ -671,8 +671,10 @@ exectry(Estate state, int do_exec)
 
     /* The always clause. */
     save_try_errflag = try_errflag;
-    try_errflag = (zlong)errflag;
-    errflag &= ~ERRFLAG_ERROR;
+    try_errflag = (zlong)(errflag & ERRFLAG_ERROR);
+    try_interrupt = errflag & ERRFLAG_INT;
+    /* We need to reset all errors to allow the block to execute */
+    errflag = 0;
     save_retflag = retflag;
     retflag = 0;
     save_breaks = breaks;
@@ -687,6 +689,17 @@ exectry(Estate state, int do_exec)
 	errflag |= ERRFLAG_ERROR;
     else
 	errflag &= ~ERRFLAG_ERROR;
+    /*
+     * TODO: currently, we always restore the interrupt
+     * error status.  We should have a way of clearing it.
+     * Doing this with try_errflag (the shell variable TRY_BLOCK_ERROR)
+     * is probably not a good idea since currently that's documented
+     * such that setting it to 0 clears errors, and we don't want
+     * to clear interrupts as a side effect.  So it probably needs
+     * a different variable.
+     */
+    if (try_interrupt)
+	errflag |= ERRFLAG_INT;
     try_errflag = save_try_errflag;
     if (!retflag)
 	retflag = save_retflag;