| 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
ps -e | grep a.out 28886 pts/12 00:00:00 a.out gdb -p 28886 Loaded symbols for /lib64/ld-linux-x86-64.so.2 0x0000003b0ccbc970 in __nanosleep_nocancel () from /lib64/libc.so.6 ../../binutils-gdb/gdb/cleanups.c:265: internal-warning: restore_my_cleanups has found a stale cleanup A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) The backtrace of this issue: (gdb) bt #0 0x0000003b0cc35c39 in raise () from /lib64/libc.so.6 #1 0x0000003b0cc37348 in abort () from /lib64/libc.so.6 #2 0x00000000006fdd38 in dump_core () at ../../binutils-gdb/gdb/utils.c:589 #3 0x00000000006fe039 in internal_vproblem (problem=0xcbdc90 <internal_warning_problem>, file=0x8b0c10 "s' failed.", line=265, fmt=0x8b0c38 "nutils-gdb/gdb/cleanups.c", ap=0x7fff803e3ed8) at ../../binutils-gdb/gdb/utils.c:748 #4 0x00000000006fe19c in internal_vwarning (file=0x8b0c10 "s' failed.", line=265, fmt=0x8b0c38 "nutils-gdb/gdb/cleanups.c", ap=0x7fff803e3ed8) at ../../binutils-gdb/gdb/utils.c:799 #5 0x00000000006fe246 in internal_warning (file=0x8b0c10 "s' failed.", line=265, string=0x8b0c38 "nutils-gdb/gdb/cleanups.c") at ../../binutils-gdb/gdb/utils.c:809 #6 0x000000000057da3d in restore_my_cleanups (pmy_chain=0xcba518 <cleanup_chain>, chain=0x14ec030) at ../../binutils-gdb/gdb/cleanups.c:265 #7 0x000000000057da67 in restore_cleanups (chain=0x14ec030) at ../../binutils-gdb/gdb/cleanups.c:276 #8 0x00000000005f0dc7 in catcher_pop () at ../../binutils-gdb/gdb/exceptions.c:116 #9 0x00000000005f0e62 in exceptions_state_mc (action=CATCH_ITER) at ../../binutils-gdb/gdb/exceptions.c:142 #10 0x00000000005f0fe6 in exceptions_state_mc (action=CATCH_ITER) at ../../binutils-gdb/gdb/exceptions.c:203 #11 0x00000000005f18ed in catch_command_errors ( command=0x5d5fb8 <attach_command_continuation_free_args+18>, arg=0x7fff803e525b "2914", from_tty=1, mask=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:549 ---Type <return> to continue, or q <return> to quit--- #12 0x00000000005f5ed7 in captured_main (data=0x7fff803e4280) at ../../binutils-gdb/gdb/main.c:968 #13 0x00000000005f180b in catch_errors (func=0x5f501c <VEC_cmdarg_s_safe_push+43>, func_args=0x7fff803e4280, errstring=0x8cf2e4 "/local/bin", mask=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:522 #14 0x00000000005f6180 in gdb_main (args=0x7fff803e4280) at ../../binutils-gdb/gdb/main.c:1061 #15 0x000000000045d087 in main (argc=3, argv=0x7fff803e4388) at ../../binutils-gdb/gdb/gdb.c:33 This is a new issue. It is introduced by commit https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=8bc2fe488957946d2cdccda3ce8d4f39e4003ea0 It removed the discard_cleanups (back_to) inside attach_command. Then restore_my_cleanups will throw a internal_warning. 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. So I add a null cleanup to attach_command_post_wait to fix this issue. It is tested and and passed regression test in x86_64-linux. Thanks, Hui 2014-03-17 Hui Zhu <hui@codesourcery.com> * infcmd.c (attach_command_post_wait): Add null_cleanup.