Re: Patch: catch_errors() cleanup chain restore

[Elena Zannoni <ezannoni at cygnus dot com>]

Andrew Cagney writes:
 > nsd@cygnus.com wrote:
 > > 
 > > Hi,
 > > 
 > > If a quit happens within catch_errors(RETURN_MASK_ERROR), the cleanup
 > > chain gets lost.
 > > 
 > > This is the offending sequence of events:
 > >   1. catch_errors() calls save_cleanups(), which zeros the cleanup chain.
 > >   2. catch_errors() redirects the error longjmp target to a location that
 > >      will call restore_cleanups(), but doesn't redirect the quit longjmp
 > >      target similarly when RETURN_MASK_ERROR is specified.
 > >   3. A subsequent quit event longjmps back to top level, skipping past
 > >      the restore_cleanups() call in catch_errors().
 > > 
 > > An error within catch_errors(RETURN_MASK_QUIT) would have the same effect,
 > > but there aren't any such calls in gdb at the moment.
 > > 
 > > It's easy to fix by:
 > >   1. unconditionally redirecting both the error and quit longjmp targets;
 > >   2. calling the function argument;
 > >   3. restoring the cleanup chain;
 > >   4. calling return_to_top_level() if appropriate.
 > > 
 > > I've appended a patch to do that.  Comments welcome,
 > > 
 > > Nick Duffek
 > > nsd@cygnus.com
 > 
 > Um, I think you've just pointed to a far nastier bug:
 > 
 > Consider the stack:
 > 
 > 	catch_errors (MASK_ALL)
 > 		error_return = quit_return = (1)
 > 	...
 > 	-> catch_errors (MASK_ERRORS)
 > 		saved_error = error_return /* (1) */
 > 		error_return = (2)
 > 	...
 > 	-> catch_errors (MASK_QUIT)
 > 		saved_quit = quit_return /* (1) */
 > 		quit_return = (3)
 > 	...
 > 	-> error ()
 > 	-> return_to_top_level (ERROR)
 > 		
 > where (1), (2), and (3) denote setjmp points.  After the long jmp we end
 > up with:
 > 
 > 	catch_errors (MASK_ALL)
 > 
 > 	...
 > 
 > with:
 > 
 > 	error_return == (1) and quit_return == (3)
 > 
 > A subsequent call to quit() would land GDB in the middle of (3) a popped
 > stack :-(
 > 
 > Can anyone confirm this?
 > 

It seems right. That's a bug.

 > 	Andrew
 > 
 > PS: Goto's result in bad kama :-)
 > 

I would prefer if this patch wasn't applied as is, we can solve Nick's
immediate problem by adding another catch_errors(..RETURN_MASK_QUIT)
call on top of the original catch_errors(..RETURN_MASK_ALL) where the
problem occurred, and do something meaningful with the return values.

[BTW, for the curious, this is in a (the?) call to
print_only_stack_frame_stub()]

Elena