This is the mail archive of the
gdb-patches@sourceware.cygnus.com
mailing list for the GDB project.
RFC/WIP: Delete SET_TOP_LEVEL ....
- To: GDB Patches <gdb-patches at sourceware dot cygnus dot com>
- Subject: RFC/WIP: Delete SET_TOP_LEVEL ....
- From: Andrew Cagney <ac131313 at cygnus dot com>
- Date: Fri, 05 Nov 1999 14:57:26 +1100
- Organization: Cygnus Solutions
Hello,
The attatched patch eliminates the macro SET_TOP_LEVEL().
Why do this?
o We delete a complicated macro.
This has to be a good thing :-)
o In the good old days (before the event
loop) control always returned to the
one true top-level (main.c). Now that
there is an event-loop (well several
different event-loops) this concept
(a single top level) no longer applies.
o Recently I created a really nasty bug
with some experimental code:
main called SET_TOP_LEVEL
main called my custom event loop
my event loop SET_TOP_LEVEL
....
my event loop returned
main called foo and foo called error()
Since my event loop thought it was the
top level it called SET_TOP_LEVEL.
Unfortunatly the error() call which jumped
to my top-level occured after my function
had returned ....
I figure eliminating SET_TOP_LEVEL eliminates
the chance of others making that mistake :-)
o Without SET_TOP_LEVEL we're left with just
one function playing with setjmp. Less possible
confusion. As some of the comments point
out, this makes it possible to safely cleanup
that code.
So what's the bad news?
The comment below that I'm adding should provide a good hint:
/* 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. */
It is highly likely that some functions don't clean up after themselves.
In fact the old main() was written with this assumption in mind - it
called do_cleanups() frequently just in case vis:
if (cdarg != NULL)
{
! if (!SET_TOP_LEVEL ())
! {
! cd_command (cdarg, 0);
! }
}
- do_cleanups (ALL_CLEANUPS);
By replacing SET_TOP_LEVEL() with catch_errors(), the do_cleanups() are
also removed (they no longer do anything useful). Since nothing is
cleaning up after soiling code, memory leaks can result :-(
So what can be done?
Following on from the above comment:
o have catch_errors() always
call do_cleanups()
o have catch_errors() check for
leeks, report them, but do nothing
o Have the new main use special wrapped
functions that also always cleanup
o all of the above :-)
o none of the above :-)
Opinions?
Andrew
Fri Nov 5 10:12:27 1999 Andrew Cagney <cagney@b1.cygnus.com>
* target.h, target.c (target_load): Replace macro with a function.
* config/i960/tm-nindy960.h (ADDITIONAL_OPTION_HANDLER): Rewrite
replacing SET_TOP_LEVEL with catch_command_errors.
(nindy_open): Add extern declaration.
* top.h (top_level_val, SET_TOP_LEVEL): Delete.
* defs.h (catch_command_errors_ftype, catch_command_errors): Add
declarations.
* top.c (struct captured_command_args): Declare.
(do_captured_command, catch_command_errors): New functions. Call
the command function via catch_errors.
(catch_errors): Add more comments.
* main.c (struct captured_main_args): Define.
(captured_main): New. Rewrite main. Replace SET_TOP_LEVEL with
calls to catch_command_errors. Delete calls to do_cleanups which
are now handled by catch_errors.
(main): Move code body to captured_main. Call captured_main via
catch_errors.
Index: defs.h
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/defs.h,v
retrieving revision 1.267
diff -p -r1.267 defs.h
*** defs.h 1999/10/18 09:49:38 1.267
--- defs.h 1999/11/05 03:43:11
*************** typedef int return_mask;
*** 847,854 ****
--- 847,867 ----
extern NORETURN void return_to_top_level (enum return_reason) ATTR_NORETURN;
+ /* If CATCH_ERRORS_FTYPE throws an error, catch_errors() returns zero
+ otherwize the result from CATCH_ERRORS_FTYPE is returned. It is
+ probably useful for CATCH_ERRORS_FTYPE to always return a non-zero
+ value. It's unfortunate that, catch_errors() does not return an
+ indication of the exact exception that it caught - quit_flag might
+ help. */
+
typedef int (catch_errors_ftype) (PTR);
extern int catch_errors (catch_errors_ftype *, PTR, char *, return_mask);
+
+ /* Template to catch_errors() that wraps calls to command
+ functions. */
+
+ typedef void (catch_command_errors_ftype) (char *, int);
+ extern int catch_command_errors (catch_command_errors_ftype *func, char *command, int from_tty, return_mask);
extern void warning_begin (void);
Index: main.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/main.c,v
retrieving revision 1.194
diff -p -r1.194 main.c
*** main.c 1999/10/07 23:26:49 1.194
--- main.c 1999/11/05 03:43:16
*************** extern char *external_editor_command;
*** 90,100 ****
#include <sys/cygwin.h> /* for cygwin32_conv_to_posix_path */
#endif
! int
! main (argc, argv)
! int argc;
! char **argv;
{
int count;
static int quiet = 0;
static int batch = 0;
--- 90,107 ----
#include <sys/cygwin.h> /* for cygwin32_conv_to_posix_path */
#endif
! struct captured_main_args
! {
! int argc;
! char **argv;
! };
!
! static int
! captured_main (void *data)
{
+ struct captured_main_args *context = data;
+ int argc = context->argc;
+ char **argv = context->argv;
int count;
static int quiet = 0;
static int batch = 0;
*************** main (argc, argv)
*** 147,158 ****
alloca (4 - i);
#endif
- /* If error() is called from initialization code, just exit */
- if (SET_TOP_LEVEL ())
- {
- exit (1);
- }
-
cmdsize = 1;
cmdarg = (char **) xmalloc (cmdsize * sizeof (*cmdarg));
ncmd = 0;
--- 154,159 ----
*************** main (argc, argv)
*** 593,602 ****
if (!inhibit_gdbinit)
{
! if (!SET_TOP_LEVEL ())
! source_command (homeinit, 0);
}
- do_cleanups (ALL_CLEANUPS);
/* Do stats; no need to do them elsewhere since we'll only
need them if homedir is set. Make sure that they are
--- 594,601 ----
if (!inhibit_gdbinit)
{
! catch_command_errors (source_command, homeinit, 0, RETURN_MASK_ALL);
}
/* Do stats; no need to do them elsewhere since we'll only
need them if homedir is set. Make sure that they are
*************** main (argc, argv)
*** 614,654 ****
/* Now perform all the actions indicated by the arguments. */
if (cdarg != NULL)
{
! if (!SET_TOP_LEVEL ())
! {
! cd_command (cdarg, 0);
! }
}
- do_cleanups (ALL_CLEANUPS);
for (i = 0; i < ndir; i++)
! if (!SET_TOP_LEVEL ())
! directory_command (dirarg[i], 0);
free ((PTR) dirarg);
- do_cleanups (ALL_CLEANUPS);
if (execarg != NULL
&& symarg != NULL
&& STREQ (execarg, symarg))
{
! /* The exec file and the symbol-file are the same. If we can't open
! it, better only print one error message. */
! if (!SET_TOP_LEVEL ())
! {
! exec_file_command (execarg, !batch);
! symbol_file_command (symarg, 0);
! }
}
else
{
if (execarg != NULL)
! if (!SET_TOP_LEVEL ())
! exec_file_command (execarg, !batch);
if (symarg != NULL)
! if (!SET_TOP_LEVEL ())
! symbol_file_command (symarg, 0);
}
- do_cleanups (ALL_CLEANUPS);
/* After the symbol file has been read, print a newline to get us
beyond the copyright line... But errors should still set off
--- 613,642 ----
/* Now perform all the actions indicated by the arguments. */
if (cdarg != NULL)
{
! catch_command_errors (cd_command, cdarg, 0, RETURN_MASK_ALL);
}
for (i = 0; i < ndir; i++)
! catch_command_errors (directory_command, dirarg[i], 0, RETURN_MASK_ALL);
free ((PTR) dirarg);
if (execarg != NULL
&& symarg != NULL
&& STREQ (execarg, symarg))
{
! /* The exec file and the symbol-file are the same. If we can't
! open it, better only print one error message.
! catch_command_errors returns non-zero on success! */
! if (catch_command_errors (exec_file_command, execarg, !batch, RETURN_MASK_ALL))
! catch_command_errors (symbol_file_command, symarg, 0, RETURN_MASK_ALL);
}
else
{
if (execarg != NULL)
! catch_command_errors (exec_file_command, execarg, !batch, RETURN_MASK_ALL);
if (symarg != NULL)
! catch_command_errors (symbol_file_command, symarg, 0, RETURN_MASK_ALL);
}
/* After the symbol file has been read, print a newline to get us
beyond the copyright line... But errors should still set off
*************** main (argc, argv)
*** 661,677 ****
if (corearg != NULL)
{
! if (!SET_TOP_LEVEL ())
! core_file_command (corearg, !batch);
! else if (isdigit (corearg[0]) && !SET_TOP_LEVEL ())
! attach_command (corearg, !batch);
}
- do_cleanups (ALL_CLEANUPS);
if (ttyarg != NULL)
! if (!SET_TOP_LEVEL ())
! tty_command (ttyarg, !batch);
! do_cleanups (ALL_CLEANUPS);
#ifdef ADDITIONAL_OPTION_HANDLER
ADDITIONAL_OPTION_HANDLER;
--- 649,664 ----
if (corearg != NULL)
{
! if (catch_command_errors (core_file_command, corearg, !batch, RETURN_MASK_ALL) == 0)
! {
! /* See if the core file is really a PID. */
! if (isdigit (corearg[0]))
! catch_command_errors (attach_command, corearg, !batch, RETURN_MASK_ALL);
! }
}
if (ttyarg != NULL)
! catch_command_errors (tty_command, ttyarg, !batch, RETURN_MASK_ALL);
#ifdef ADDITIONAL_OPTION_HANDLER
ADDITIONAL_OPTION_HANDLER;
*************** main (argc, argv)
*** 689,702 ****
|| memcmp ((char *) &homebuf, (char *) &cwdbuf, sizeof (struct stat)))
if (!inhibit_gdbinit)
{
! if (!SET_TOP_LEVEL ())
! source_command (gdbinit, 0);
}
- do_cleanups (ALL_CLEANUPS);
for (i = 0; i < ncmd; i++)
{
! if (!SET_TOP_LEVEL ())
{
/* NOTE: I am commenting this out, because it is not clear
where this feature is used. It is very old and
--- 676,690 ----
|| memcmp ((char *) &homebuf, (char *) &cwdbuf, sizeof (struct stat)))
if (!inhibit_gdbinit)
{
! catch_command_errors (source_command, gdbinit, 0, RETURN_MASK_ALL);
}
for (i = 0; i < ncmd; i++)
{
! #if 0
! /* NOTE: cagney/1999-11-03: SET_TOP_LEVEL() was a macro that
! expanded into a call to setjmp(). */
! if (!SET_TOP_LEVEL ()) /* NB: This is #if 0'd out */
{
/* NOTE: I am commenting this out, because it is not clear
where this feature is used. It is very old and
*************** main (argc, argv)
*** 709,714 ****
--- 697,704 ----
source_command (cmdarg[i], !batch);
do_cleanups (ALL_CLEANUPS);
}
+ #endif
+ catch_command_errors (source_command, cmdarg[i], !batch, RETURN_MASK_ALL);
}
free ((PTR) cmdarg);
*************** main (argc, argv)
*** 755,779 ****
The WIN32 Gui calls this main to set up gdb's state, and
has its own command loop. */
#if !defined _WIN32 || defined __GNUC__
while (1)
{
! if (!SET_TOP_LEVEL ())
{
! do_cleanups (ALL_CLEANUPS); /* Do complete cleanup */
! /* GUIs generally have their own command loop, mainloop, or whatever.
! This is a good place to gain control because many error
! conditions will end up here via longjmp(). */
! if (command_loop_hook)
! command_loop_hook ();
! else
! command_loop ();
! quit_command ((char *) 0, instream == stdin);
}
}
/* No exit -- exit is through quit_command. */
#endif
}
/* Don't use *_filtered for printing help. We don't want to prompt
for continue no matter how small the screen or how much we're going
--- 745,779 ----
The WIN32 Gui calls this main to set up gdb's state, and
has its own command loop. */
#if !defined _WIN32 || defined __GNUC__
+ /* GUIs generally have their own command loop, mainloop, or
+ whatever. This is a good place to gain control because many
+ error conditions will end up here via longjmp(). */
+ if (command_loop_hook == NULL)
+ command_loop_hook = command_loop;
while (1)
{
! if (catch_errors ((catch_errors_ftype*) command_loop_hook,
! 0, "", RETURN_MASK_ALL) == 0)
{
! /* Normal exit!?? */
! quit_command (NULL, instream == stdin);
}
}
/* No exit -- exit is through quit_command. */
#endif
+ }
+ int
+ main (int argc, char **argv)
+ {
+ int top_level_val;
+ struct captured_main_args args;
+ args.argc = argc;
+ args.argv = argv;
+ catch_errors (captured_main, &args, "", RETURN_MASK_ALL);
+ return 0;
}
+
/* Don't use *_filtered for printing help. We don't want to prompt
for continue no matter how small the screen or how much we're going
Index: target.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/target.c,v
retrieving revision 1.111
diff -p -r1.111 target.c
*** target.c 1999/10/17 05:59:27 1.111
--- target.c 1999/11/05 03:43:19
*************** target_ignore ()
*** 272,277 ****
--- 272,283 ----
{
}
+ void
+ target_load (char *arg, int from_tty)
+ {
+ (*current_target.to_load) (arg, from_tty);
+ }
+
/* ARGSUSED */
static int
nomemory (memaddr, myaddr, len, write, t)
Index: target.h
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/target.h,v
retrieving revision 1.89
diff -p -r1.89 target.h
*** target.h 1999/11/03 01:59:00 1.89
--- target.h 1999/11/05 03:43:25
*************** print_section_info PARAMS ((struct targe
*** 755,762 ****
not only bring new code into the target process, but also to update
GDB's symbol tables to match. */
! #define target_load(arg, from_tty) \
! (*current_target.to_load) (arg, from_tty)
/* Look up a symbol in the target's symbol table. NAME is the symbol
name. ADDRP is a CORE_ADDR * pointing to where the value of the symbol
--- 755,761 ----
not only bring new code into the target process, but also to update
GDB's symbol tables to match. */
! extern void target_load (char *arg, int from_tty);
/* Look up a symbol in the target's symbol table. NAME is the symbol
name. ADDRP is a CORE_ADDR * pointing to where the value of the symbol
Index: top.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/top.c,v
retrieving revision 2.166
diff -p -r2.166 top.c
*** top.c 1999/10/23 04:09:30 2.166
--- top.c 1999/11/05 03:43:45
*************** return_to_top_level (reason)
*** 530,535 ****
--- 530,554 ----
catch_errors. Note that quit should return to the command line
fairly quickly, even if some further processing is being done. */
+ /* MAYBE: cagney/1999-11-05: catch_errors() in conjunction with
+ error() et.al. could maintain a set of flags that indicate the the
+ current state of each of the longjmp buffers. This would give the
+ longjmp code the chance to detect a longjmp botch (before it gets
+ to longjmperror()). Prior to 1999-11-05 this wasn't possible as
+ 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? */
+
int
catch_errors (func, args, errstring, mask)
catch_errors_ftype *func;
*************** catch_errors (func, args, errstring, mas
*** 567,572 ****
--- 586,599 ----
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;
*************** catch_errors (func, args, errstring, mas
*** 585,590 ****
--- 612,644 ----
}
return val;
}
+
+ struct captured_command_args
+ {
+ catch_command_errors_ftype *command;
+ char *arg;
+ int from_tty;
+ };
+
+ static int
+ do_captured_command (void *data)
+ {
+ struct captured_command_args *context = data;
+ context->command (context->arg, context->from_tty);
+ return 1;
+ }
+
+ int
+ catch_command_errors (catch_command_errors_ftype *command,
+ char *arg, int from_tty, return_mask mask)
+ {
+ struct captured_command_args args;
+ args.command = command;
+ args.arg = arg;
+ args.from_tty = from_tty;
+ return catch_errors (do_captured_command, &args, "", mask);
+ }
+
/* Handler for SIGHUP. */
Index: top.h
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/top.h,v
retrieving revision 2.16
diff -p -r2.16 top.h
*** top.h 1999/09/20 03:22:22 2.16
--- top.h 1999/11/05 03:43:45
*************** extern char gdbinit[];
*** 42,60 ****
#define SIGLONGJMP(buf,val) longjmp(buf,val)
#endif
- /* Temporary variable for SET_TOP_LEVEL. */
-
- int top_level_val;
-
- /* Do a setjmp on error_return and quit_return. catch_errors is
- generally a cleaner way to do this, but main() would look pretty
- ugly if it had to use catch_errors each time. */
-
- #define SET_TOP_LEVEL() \
- (((top_level_val = SIGSETJMP (error_return)) \
- ? (PTR) 0 : (PTR) memcpy (quit_return, error_return, sizeof (SIGJMP_BUF))) \
- , top_level_val)
-
extern SIGJMP_BUF error_return;
extern SIGJMP_BUF quit_return;
--- 42,47 ----
Index: config/i960/tm-nindy960.h
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/config/i960/tm-nindy960.h,v
retrieving revision 1.8
diff -p -r1.8 tm-nindy960.h
*** tm-nindy960.h 1999/08/25 08:56:55 1.8
--- tm-nindy960.h 1999/11/05 03:43:45
*************** extern char *nindy_ttyname; /* Name of s
*** 60,72 ****
/* If specified on the command line, open tty for talking to nindy,
and download the executable file if one was specified. */
! #define ADDITIONAL_OPTION_HANDLER \
! if (!SET_TOP_LEVEL () && nindy_ttyname) { \
! nindy_open (nindy_ttyname, !batch); \
! if (!SET_TOP_LEVEL () && execarg) { \
! target_load (execarg, !batch); \
! } \
! }
/* If configured for i960 target, we take control before main loop
and demand that we configure for a nindy target. */
--- 60,77 ----
/* If specified on the command line, open tty for talking to nindy,
and download the executable file if one was specified. */
! extern void nindy_open (char *name, int from_tty);
! #define ADDITIONAL_OPTION_HANDLER \
! if (nindy_ttyname != NULL) \
! { \
! if (catch_command_errors (nindy_open, nindy_ttyname, \
! !batch, RETURN_MASK_ALL)) \
! { \
! if (execarg != NULL) \
! catch_command_errors (target_load, execarg, !batch, \
! RETURN_MASK_ALL); \
! } \
! }
/* If configured for i960 target, we take control before main loop
and demand that we configure for a nindy target. */