bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

Thanks for such a rapid response!

On Sun, Nov 03, 2024 at 21:46:42 -0500, Stefan Monnier wrote:
> Hi Alan,

> I couldn't properly review your code because I had trouble
> understanding it.  So instead, it's mostly questions.

> > +static Lisp_Object
> > +internal_cc_hb (enum handlertype handlertype,
> > +		Lisp_Object (*bfun) (void), Lisp_Object handlers,
> > +		Lisp_Object (*hfun) (Lisp_Object))
> > +{
> > +  struct handler *c = push_handler (handlers, handlertype);
> > +
> > +  if (handlertype == HANDLER_BIND)
> > +    {
> > +      c->val = Qnil;
> > +      c->bin_handler = hfun;
> > +      c->bytecode_dest = 0;
> > +    }
> > +  if (sys_setjmp (c->jmp))
> > +    {
> > +      Lisp_Object val = handlerlist->val;
> > +      clobbered_eassert (handlerlist == c);
> > +      handlerlist = handlerlist->next;
> > +      return hfun (val);
> > +    }
> > +  else
> > +    {
> > +      Lisp_Object val = bfun ();
> > +      eassert (handlerlist == c);
> > +      handlerlist = c->next;
> > +      return val;
> > +    }
> > +}

> I don't understand the above code: `handler-bind` is not supposed to
> call setjmp/longjmp: when the handler of a `handler-bind` gets called,
> the stack is not unwound.

I started off by just making the HANDLER_BIND case use the same code as
CONDITION_CASE, and got segfaults.  I've tried to find the places where
a longjmp gets called for condition-case, but my brain's tired after a
weekend of heavy coding.

Given HANDLER_BIND doesn't need the setjmp/longjmp mechanism, it would
seem there's no sense in combining the HANDLER_BIND and CONDITION_CASE
cases in internal_cc_hb and friends.  I should just restore the
condition-case functions to what they were, and add separate new ones
for handler-bind.

> So what is the `sys_setjmp` for when `handlertype == HANDLER_BIND`?

After reading your previous paragraph, probably nothing.

[ .... ]

> [ We don't need parens around `h->bin_handler`.  ]

They've gone.

> > +		else
> > +		  call1 (h->val, error);
> > +	        unbind_to (count, Qnil); /* Removes SKIP_CONDITIONS handler.  */
> > +	        pop_handler ();		 /* Discards HANDLER_BIND handler.  */

> These comments don't match my understanding of the code: IIRC the
> `pop_handler` pops the `SKIP_CONDITIONS` handler.

I think I see now you're right.  push_handler doesn't push an entry onto
the binding stack.  I'll amend these comments as soon as I understand
the code.  I think these lines definitely need comments.

> Also there's no reason to presume the HANDLER_BIND handler is at the
> top, so if we wanted to remove it, we'd have to work harder.

This code is difficult to understand.  What is the purpose of the
binding block around the call of the handler function?  I think a
comment here would help.

> > +/* Handle errors signalled by the above function.  */
> > +static Lisp_Object
> > +rel_error_handler (Lisp_Object err)
> > +{
> > +  if (!NILP (Vdebug_on_error))
> > +    {
> > +      call_debugger (err);
> > +      Fthrow (Qtop_level, Qt);
> > +    }
> > +  else
> > +    return err;
> > +}

> I don't understand that.
> AFAICT, this is the only HANDLER_BIND handler you install from
> C code, right?  So this is the thing that motivates all the changes like
> `internal_cc_hb`, etc... so clearly it must do something worthwhile, but
> I don't see what it is.

rel_error_handler is currently the only such handler, yes.  All this
code (once it's debugged) will fix the inconsistency that
condition-case, unwind-protect, catch and throw, ... can all be used
from C code, but handler-bind can't be.

rel_error_handler, in particular, allows the debugger to be called
regardless of any active condition_cases which would otherwise block it.
In the byte compiler, there is such a blocking condition-case set up by
bytecomp--displaying-warnings when byte-compile-debug is nil.  This
mechanism is currently only used in Fload.

> How is this related to `Fprefix_load_file_names`?

Not closely.  I think I should have propsed two separate patches, rather
than the big one I did.  Fprefix_load_file_names is what puts the "While
loading foo.el... " in front of an error message.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).