This is the mail archive of the
gdb-patches@sourceware.cygnus.com
mailing list for the GDB project.
Re: Patch: catch_errors() bug fixes and speedups
Looks great. Thanks Nick, for taking the time to fix this.
It can certainly go in, unless Andrew has some objections.
Elena
nsd@cygnus.com writes:
> The appended patch is an improved version of the "catch_errors() cleanup
> chain restore" patch I posted on 11-Feb.
>
> It fixes these bugs:
>
> 1. If a quit happened within catch_errors(RETURN_MASK_ERROR) or vice
> versa, the cleanup chain got lost.
>
> 2. A catch_errors(RETURN_MASK_ERROR) within a
> catch_errors(RETURN_MASK_QUIT) or vice versa could have resulted in a
> garbled jump buffer.
>
> 3. Some comments were out of date.
>
> It also improves efficiency by avoiding a few memcpy()s.
>
> Comments welcome,
>
> Nick Duffek
> nsd@cygnus.com
>
> [patch follows]
>
> Index: top.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/top.c,v
> retrieving revision 1.2
> diff -u -r1.2 top.c
> --- top.c 2000/02/09 03:28:18 1.2
> +++ top.c 2000/02/22 19:59:14
> @@ -482,8 +482,8 @@
> PARAMS ((void)) ATTR_NORETURN;
>
>
> -/* Generally one should use catch_errors rather than manipulating these
> - directly. The exception is main(). */
> +/* One should use catch_errors rather than manipulating these
> + directly. */
> #if defined(HAVE_SIGSETJMP)
> #define SIGJMP_BUF sigjmp_buf
> #define SIGSETJMP(buf) sigsetjmp(buf, 1)
> @@ -494,13 +494,10 @@
> #define SIGLONGJMP(buf,val) longjmp(buf,val)
> #endif
>
> -/* Where to go for return_to_top_level (RETURN_ERROR). */
> -static SIGJMP_BUF error_return;
> -/* Where to go for return_to_top_level (RETURN_QUIT). */
> -static SIGJMP_BUF quit_return;
> +/* Where to go for return_to_top_level. */
> +static SIGJMP_BUF *catch_return;
>
> -/* Return for reason REASON. This generally gets back to the command
> - loop, but can be caught via catch_errors. */
> +/* Return for reason REASON to the nearest containing catch_errors(). */
>
> NORETURN void
> return_to_top_level (reason)
> @@ -531,8 +528,11 @@
> break;
> }
>
> - (NORETURN void) SIGLONGJMP
> - (reason == RETURN_ERROR ? error_return : quit_return, 1);
> + /* Jump to the containing catch_errors() call, communicating REASON
> + to that call via setjmp's return value. Note that REASON can't
> + be zero, by definition in defs.h. */
> +
> + (NORETURN void) SIGLONGJMP (*catch_return, (int)reason);
> }
>
> /* Call FUNC with arg ARGS, catching any errors. If there is no
> @@ -562,13 +562,6 @@
> code also randomly used a SET_TOP_LEVEL macro that directly
> initialize the longjmp buffers. */
>
> -/* MAYBE: cagney/1999-11-05: Since the SET_TOP_LEVEL macro has been
> - eliminated it is now possible to use the stack to directly store
> - each longjmp buffer. The global code would just need to update a
> - pointer (onto the stack - ulgh!?) indicating the current longjmp
> - buffers. It would certainly improve the performance of the longjmp
> - code since the memcpy's would be eliminated. */
> -
> /* MAYBE: cagney/1999-11-05: Should the catch_erros and cleanups code
> be consolidated into a single file instead of being distributed
> between utils.c and top.c? */
> @@ -580,61 +573,89 @@
> char *errstring;
> return_mask mask;
> {
> - SIGJMP_BUF saved_error;
> - SIGJMP_BUF saved_quit;
> - SIGJMP_BUF tmp_jmp;
> + SIGJMP_BUF *saved_catch;
> + SIGJMP_BUF catch;
> int val;
> struct cleanup *saved_cleanup_chain;
> char *saved_error_pre_print;
> char *saved_quit_pre_print;
>
> - saved_cleanup_chain = save_cleanups ();
> + /* Return value from SIGSETJMP(): enum return_reason if error or
> + quit caught, 0 otherwise. */
> + int caught;
> +
> + /* Override error/quit messages during FUNC. */
> +
> saved_error_pre_print = error_pre_print;
> saved_quit_pre_print = quit_pre_print;
>
> if (mask & RETURN_MASK_ERROR)
> - {
> - memcpy ((char *) saved_error, (char *) error_return, sizeof (SIGJMP_BUF));
> - error_pre_print = errstring;
> - }
> + error_pre_print = errstring;
> if (mask & RETURN_MASK_QUIT)
> - {
> - memcpy (saved_quit, quit_return, sizeof (SIGJMP_BUF));
> - quit_pre_print = errstring;
> - }
> -
> - if (SIGSETJMP (tmp_jmp) == 0)
> - {
> - if (mask & RETURN_MASK_ERROR)
> - memcpy (error_return, tmp_jmp, sizeof (SIGJMP_BUF));
> - if (mask & RETURN_MASK_QUIT)
> - memcpy (quit_return, tmp_jmp, sizeof (SIGJMP_BUF));
> - val = (*func) (args);
> - /* FIXME: cagney/1999-11-05: A correct FUNC implementaton will
> - clean things up (restoring the cleanup chain) to the state
> - they were just prior to the call. Technically, this means
> - that the below restore_cleanups call is redundant.
> - Unfortunatly, many FUNC's are not that well behaved.
> - restore_cleanups should either be replaced with a do_cleanups
> - call (to cover the problem) or an assertion check to detect
> - bad FUNCs code. */
> - }
> - else
> - val = 0;
> + quit_pre_print = errstring;
> +
> + /* Prevent error/quit during FUNC from calling cleanups established
> + prior to here. */
>
> + saved_cleanup_chain = save_cleanups ();
> +
> + /* Call FUNC, catching error/quit events. */
> +
> + saved_catch = catch_return;
> + catch_return = &catch;
> + caught = SIGSETJMP (catch);
> + if (!caught)
> + val = (*func) (args);
> + catch_return = saved_catch;
> +
> + /* FIXME: cagney/1999-11-05: A correct FUNC implementaton will
> + clean things up (restoring the cleanup chain) to the state they
> + were just prior to the call. Unfortunatly, many FUNC's are not
> + that well behaved. This could be fixed by adding either a
> + do_cleanups call (to cover the problem) or an assertion check to
> + detect bad FUNCs code. */
> +
> + /* Restore the cleanup chain and error/quit messages to their
> + original states. */
> +
> restore_cleanups (saved_cleanup_chain);
>
> - if (mask & RETURN_MASK_ERROR)
> - {
> - memcpy (error_return, saved_error, sizeof (SIGJMP_BUF));
> - error_pre_print = saved_error_pre_print;
> - }
> if (mask & RETURN_MASK_QUIT)
> - {
> - memcpy (quit_return, saved_quit, sizeof (SIGJMP_BUF));
> - quit_pre_print = saved_quit_pre_print;
> - }
> - return val;
> + quit_pre_print = saved_quit_pre_print;
> + if (mask & RETURN_MASK_ERROR)
> + error_pre_print = saved_error_pre_print;
> +
> + /* Return normally if no error/quit event occurred. */
> +
> + if (!caught)
> + return val;
> +
> + /* If the caller didn't request that the event be caught, relay the
> + event to the next containing catch_errors(). */
> +
> + if (!(mask & RETURN_MASK (caught)))
> + return_to_top_level (caught);
> +
> + /* Tell the caller that an event was caught.
> +
> + FIXME: nsd/2000-02-22: When MASK is RETURN_MASK_ALL, the caller
> + can't tell what type of event occurred.
> +
> + A possible fix is to add a new interface, catch_event(), that
> + returns enum return_reason after catching an error or a quit.
> +
> + When returning normally, i.e. without catching an error or a
> + quit, catch_event() could return RETURN_NORMAL, which would be
> + added to enum return_reason. FUNC would return information
> + exclusively via ARGS.
> +
> + Alternatively, normal catch_event() could return FUNC's return
> + value. The caller would need to be aware of potential overlap
> + with enum return_reason, which could be publicly restricted to
> + negative values to simplify return value processing in FUNC and
> + in the caller. */
> +
> + return 0;
> }
>
> struct captured_command_args
> Index: defs.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/defs.h,v
> retrieving revision 1.2
> diff -u -r1.2 defs.h
> --- defs.h 2000/02/10 20:06:32 1.2
> +++ defs.h 2000/02/22 19:59:14
> @@ -801,21 +801,24 @@
>
> extern NORETURN void nomem (long) ATTR_NORETURN;
>
> -/* Reasons for calling return_to_top_level. */
> +/* Reasons for calling return_to_top_level. Note: enum value 0 is
> + reserved for internal use as the return value from an initial
> + setjmp(). */
>
> enum return_reason
> {
> /* User interrupt. */
> - RETURN_QUIT,
> + RETURN_QUIT = 1,
> /* Any other error. */
> RETURN_ERROR
> };
>
> #define ALL_CLEANUPS ((struct cleanup *)0)
>
> -#define RETURN_MASK_QUIT (1 << (int)RETURN_QUIT)
> -#define RETURN_MASK_ERROR (1 << (int)RETURN_ERROR)
> -#define RETURN_MASK_ALL (RETURN_MASK_QUIT | RETURN_MASK_ERROR)
> +#define RETURN_MASK(reason) (1 << (int)(reason))
> +#define RETURN_MASK_QUIT RETURN_MASK (RETURN_QUIT)
> +#define RETURN_MASK_ERROR RETURN_MASK (RETURN_ERROR)
> +#define RETURN_MASK_ALL (RETURN_MASK_QUIT | RETURN_MASK_ERROR)
> typedef int return_mask;
>
> extern NORETURN void return_to_top_level (enum return_reason) ATTR_NORETURN;