Message ID | 53271C09.5000709@mentor.com |
---|---|
State | Superseded |
Headers | show |
On 03/17/2014 04:00 PM, Hui Zhu wrote: > The most of the pid_to_exec_file target_ops method for some platforms will > allocate memory for exec_file and add them to cleanup. Which platforms?
On 03/17/2014 04:12 PM, Pedro Alves wrote: > On 03/17/2014 04:00 PM, Hui Zhu wrote: > >> The most of the pid_to_exec_file target_ops method for some platforms will >> allocate memory for exec_file and add them to cleanup. > > Which platforms? OK, I see several do that, including linux-nat.c. IMO, that ends up being a silly interface. The current interface is documented as: /* Attempts to find the pathname of the executable file that was run to create a specified process. The process PID must be stopped when this operation is used. If the executable file cannot be determined, NULL is returned. Else, a pointer to a character string containing the pathname is returned. This string should be copied into a buffer by the client if the string will not be immediately used, or if it must persist. */ #define target_pid_to_exec_file(pid) \ (current_target.to_pid_to_exec_file) (¤t_target, pid) The "This string should be copied into a buffer by the client if the string will not be immediately used, or if it must persist." part hints that the implementation is supposed to return a pointer to a static buffer, like e.g., target_pid_to_str, paddress, and friends, etc. Either we make target_pid_to_exec_file return a pointer to a malloc buffer that the caller is responsible for xfree'ing (and adjust the interface comments in target.h) or we make targets indeed return a pointer to a static buffer, as the current method's description hints at. Returning a malloced buffer, and installing a cleanup like that is a silly interface, IMO. Note that GDB used to have more random memory-release cleanups installed like this, but we've removed most, I believe. Although it's really not harmful to install a cleanup that just releases memory later at any random time, OTOH, it potentially makes debugging nasty cleanup issues harder, so we've moved away from doing that, and we now have that warning. Bummer that we don't have a test that caught this. :-(
--- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -2337,6 +2337,7 @@ attach_command_post_wait (char *args, in char *exec_file; char *full_exec_path = NULL; struct inferior *inferior; + struct cleanup *back_to = make_cleanup (null_cleanup, NULL); inferior = current_inferior (); inferior->control.stop_soon = NO_STOP_QUIETLY; @@ -2421,6 +2422,12 @@ attach_command_post_wait (char *args, in if (deprecated_attach_hook) deprecated_attach_hook (); } + + /* The pid_to_exec_file target_ops method for some platforms will + allocate memory for exec_file and add them to cleanup. + Release them in here because function attach_command will be call + by function captured_main too. */ + do_cleanups (back_to); } struct attach_command_continuation_args