Message ID | 20190216205810.1253-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 45492 invoked by alias); 16 Feb 2019 20:58:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 45481 invoked by uid 89); 16 Feb 2019 20:58:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=3400, gdb.c, UD:gdb.c, sk:get_thr X-HELO: mailsec105.isp.belgacom.be Received: from mailsec105.isp.belgacom.be (HELO mailsec105.isp.belgacom.be) (195.238.20.101) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 16 Feb 2019 20:58:38 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1550350718; x=1581886718; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=uu4+KM8LRXu6bahS4ocR0gVcxsfJs8EOBCi48LAd8UQ=; b=wGRzbsuUCu8Lda/KIkKHaqqxj1uJZjQ7fGswhFFGIC0U+6rmhRVbM3qY PKj13EFuMvjYlONgfLzJRea5jY5+/w==; Received: from 147.122-130-109.adsl-dyn.isp.belgacom.be (HELO md.home) ([109.130.122.147]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 16 Feb 2019 21:58:35 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFA] Fix regcache leak, and avoid possible regcache access after detach. Date: Sat, 16 Feb 2019 21:58:10 +0100 Message-Id: <20190216205810.1253-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
Feb. 16, 2019, 8:58 p.m. UTC
Valgrind reports leaks like the below in various tests, e.g. gdb.threads/attach-slow-waitpid.exp, gdb.ada/task_switch_in_core.exp, ... Fix the leak by clearing the regcache when detaching from an inferior. Note that these leaks are 'created' when GDB exits, when the regcache::current_regcache is destroyed : the elements of the forward_list are pointers, and the 'pointed to' memory is not deleted by the forward_list destructor. Nevertheless, fixing this leak is good as it makes a bunch of tests 'leak clean'. Also, it seems strange to keep a register cache for a process from which GDB detached : it is not clear if this cache is still valid after detach. And effectively, when clearing only the regcache, (and not the frame cache), then the frame cache was still 'pointing' at this regcache and was used when switching to the child process in the test gdb.threads/watchpoint-fork.exp, which seems strange. So, we solve the leak and avoid possible accesses to the regcache and frame cache of the detached inferior, by clearing both the regcache and the frame cache. Tested on debian/amd64, natively and under Valgrind. ==27679== VALGRIND_GDB_ERROR_BEGIN ==27679== 1,123 (72 direct, 1,051 indirect) bytes in 1 blocks are definitely lost in loss record 2,942 of 3,400 ==27679== at 0x4C2C4CC: operator new(unsigned long) (vg_replace_malloc.c:344) ==27679== by 0x5CDF71: get_thread_arch_aspace_regcache(ptid_t, gdbarch*, address_space*) (regcache.c:330) ==27679== by 0x5CE12A: get_thread_regcache (regcache.c:366) ==27679== by 0x5CE12A: get_current_regcache() (regcache.c:372) ==27679== by 0x4FF63D: post_create_inferior(target_ops*, int) (infcmd.c:452) ==27679== by 0x43AF62: core_target_open(char const*, int) (corelow.c:458) ==27679== by 0x408B68: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:1892) ==27679== by 0x66B399: execute_command(char const*, int) (top.c:630) ==27679== by 0x4B4D2B: command_handler(char const*) (event-top.c:583) ==27679== by 0x4B505C: command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) (event-top.c:770) ==27679== by 0x4B3EA3: gdb_rl_callback_handler(char*) (event-top.c:213) ==27679== by 0x4E68AC2: rl_callback_read_char (callback.c:283) ==27679== by 0x4B3DDD: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175) ==27679== by 0x4B3E48: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192) ==27679== by 0x4B43EF: stdin_event_handler(int, void*) (event-top.c:511) ==27679== by 0x4B31BC: gdb_wait_for_event(int) (event-loop.c:859) ==27679== by 0x4B32F3: gdb_do_one_event() [clone .part.4] (event-loop.c:347) ==27679== by 0x4B3484: gdb_do_one_event (common-exceptions.h:219) ==27679== by 0x4B3484: start_event_loop() (event-loop.c:371) ==27679== by 0x549967: captured_command_loop() (main.c:330) ==27679== by 0x54A95C: captured_main (main.c:1176) ==27679== by 0x54A95C: gdb_main(captured_main_args*) (main.c:1192) ==27679== by 0x297517: main (gdb.c:32) gdb/ChangeLog 2019-02-16 Philippe Waroquiers <philippe.waroquiers@skynet.be> * target.c (target_detach): Clear the regcache and the frame cache. --- gdb/target.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
Comments
On Sat, 16 Feb 2019 21:58:10 +0100 Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote: > Valgrind reports leaks like the below in various tests, > e.g. gdb.threads/attach-slow-waitpid.exp, gdb.ada/task_switch_in_core.exp, ... > > Fix the leak by clearing the regcache when detaching from an inferior. > Note that these leaks are 'created' when GDB exits, > when the regcache::current_regcache is destroyed : the elements > of the forward_list are pointers, and the 'pointed to' memory is not > deleted by the forward_list destructor. > > Nevertheless, fixing this leak is good as it makes a bunch of > tests 'leak clean'. > > Also, it seems strange to keep a register cache for a process from > which GDB detached : it is not clear if this cache is still valid > after detach. And effectively, when clearing only the regcache, > (and not the frame cache), then the frame cache was still 'pointing' > at this regcache and was used when switching to the child process > in the test gdb.threads/watchpoint-fork.exp, which seems strange. > > So, we solve the leak and avoid possible accesses to the regcache > and frame cache of the detached inferior, by clearing both the > regcache and the frame cache. > > Tested on debian/amd64, natively and under Valgrind. Before committing, could you also test against native-gdbserver? I.e... make check RUNTESTFLAGS="--target_board=native-gdbserver" (I'd like the detach code in remote.c to be exercised as well. I've run into some subtle bugs in this area of remote.c in the past.) My remaining comments consist of a few nits... > diff --git a/gdb/target.c b/gdb/target.c > index 116510e8cb..0f10628590 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2010,6 +2010,8 @@ target_preopen (int from_tty) > void > target_detach (inferior *inf, int from_tty) > { > + ptid_t pid_ptid = ptid_t (inf->pid); > + Please move this declaration / initialization to the line just before where it's used. Since it's only used once, you could also just pass ptid_t (inf->pid) directly to registers_changed_ptid() and get rid of the declaration entirely. > /* As long as some to_detach implementations rely on the current_inferior > (either directly, or indirectly, like through target_gdbarch or by > reading memory), INF needs to be the current inferior. When that > @@ -2029,6 +2031,15 @@ target_detach (inferior *inf, int from_tty) > prepare_for_detach (); > > current_top_target ()->detach (inf, from_tty); > + > + /* After we have detached, clear the register cache for this inferior. */ > + registers_changed_ptid (pid_ptid); > + > + /* We have to ensure we have no frame cached left. Normally, s/cached/cache/ > + registers_changed_ptid (pid_ptid) calls reinit_frame_cache when > + inferior_ptid matches pid_ptid, but in our case, it does not > + call it, as inferior_pid has been reset. */ s/inferior_pid/inferior_ptid/ Thanks for that comment though; I was wondering why reinit_frame_cache needed to be called. > + reinit_frame_cache (); > } Okay with those changes, assuming that all goes well with the native-gdbserver testing.
On Tue, 2019-02-19 at 16:44 -0700, Kevin Buettner wrote: > Okay with those changes, assuming that all goes well with the > native-gdbserver testing. Thanks for the review. I have just pushed the patch, after doing the suggested fixes, successfully tested with native gdbserver, and waited for the branching just to be sure. Philippe
diff --git a/gdb/target.c b/gdb/target.c index 116510e8cb..0f10628590 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2010,6 +2010,8 @@ target_preopen (int from_tty) void target_detach (inferior *inf, int from_tty) { + ptid_t pid_ptid = ptid_t (inf->pid); + /* As long as some to_detach implementations rely on the current_inferior (either directly, or indirectly, like through target_gdbarch or by reading memory), INF needs to be the current inferior. When that @@ -2029,6 +2031,15 @@ target_detach (inferior *inf, int from_tty) prepare_for_detach (); current_top_target ()->detach (inf, from_tty); + + /* After we have detached, clear the register cache for this inferior. */ + registers_changed_ptid (pid_ptid); + + /* We have to ensure we have no frame cached left. Normally, + registers_changed_ptid (pid_ptid) calls reinit_frame_cache when + inferior_ptid matches pid_ptid, but in our case, it does not + call it, as inferior_pid has been reset. */ + reinit_frame_cache (); } void