From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id FfCDMg9TlGDrLQAAWB0awg (envelope-from ) for ; Thu, 06 May 2021 16:35:27 -0400 Received: by simark.ca (Postfix, from userid 112) id C25041F11C; Thu, 6 May 2021 16:35:27 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id DE0491E01F for ; Thu, 6 May 2021 16:35:26 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 39108383800C; Thu, 6 May 2021 20:35:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 39108383800C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1620333326; bh=UWhKxoSs2gRdVi4QJUKuRopYrOrasjxrudtKDJ32fq0=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=jce8Su/jICMr9Dv0xjohKqA1PRWWoSxqn35byiFHNDNJUqudxz5YNoQ39pyL4IeE6 EEHeMXZIqW4eKxrihyLyTSkjQ/T4IzS1ysauIxSa+y2sKUOAkhCN5dAi0hOsZGMuCd DLsd2laqOKmB3ep3AXnh6rqQh4+3aSruaaSnDQMA= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id B62D83838002 for ; Thu, 6 May 2021 20:35:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B62D83838002 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-336-hxAiqn_RNee-fERCBycDkA-1; Thu, 06 May 2021 16:35:20 -0400 X-MC-Unique: hxAiqn_RNee-fERCBycDkA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 001B41009619; Thu, 6 May 2021 20:35:19 +0000 (UTC) Received: from localhost.localdomain.com (ovpn-117-186.rdu2.redhat.com [10.10.117.186]) by smtp.corp.redhat.com (Postfix) with ESMTP id 70C56196A1; Thu, 6 May 2021 20:35:19 +0000 (UTC) To: simon.marchi@polymtl.ca Subject: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client Date: Thu, 6 May 2021 16:35:13 -0400 Message-Id: <20210506203513.525551-1-amerey@redhat.com> In-Reply-To: References: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Aaron Merey via Gdb-patches Reply-To: Aaron Merey Cc: fche@redhat.com, tom@tromey.com, gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On Thu, May 6, 2021 at 3:12 PM Simon Marchi wrote: > > On 2021-05-06 2:47 p.m., Frank Ch. Eigler wrote:> Hi - > > > >> I think it would still be a good idea to properly close the client > >> at exit.  Not because the memory leak is a concern, but because we > >> shouldn't assume what closing a debuginfod client (current or > >> future) does or does not do, so we shouldn't assume that we can get > >> away without calling debuginfo_end. > > > > If it's not hard to do, sure .... but wouldn't that necessitate > > exposing the pointer from function static scope again? > > I don't think so, just make the static variable a unique pointer (the > type that you are removing in this patch).  The destructor of the > unique_ptr will be called at program exit, closing the client cleanly. > That variable can be declared inside debuginfod_init, like Tom > suggested.  Something like: > > static debuginfo_client * > debuginfod_init () > { >   static debuginfod_client_up global_client; > >   if (global_client == nullptr) >     global_client.reset (debuginfod_begin ()); > >   return global_client.get (); > } > > In that case, the debuginfod_init function should probably be renamed to > something like get_debuginfod_client, something like that. > > >> A debuginfod client could eventually maintain some temporary files > >> that need to be deleted when closing in order not to litter /tmp, > >> some cache files need to be flushed, etc. > > > > I suspect that if we were to do any of those things in the future, > > we'd keep things clean by techniques like holding onto fd's to > > unlinked files, so process exit is enough.  (We could make a > > commitment in the docs.) > > That sounds like a good idea, this way even if the user of the client > crashes, there won't be any leftovers.  However, committing to that in > the documentation just sounds like an unnecessary way to paint yourself > in a corner. Even if we ensure that debuginfod_end() properly closes libdebuginfod internals, I'd still like to use the debuginfod_client unique_ptr the way Simon described. This design is more extensible if in the future we need to safely close other debuginfod-related resources on the gdb-side at program exit. I've included the updated patch below. Aaron --- Instead of initializing a new debuginfod_client for each query, store the first initialized client for the remainder of the GDB session and use it for every debuginfod query. In conjunction with upcoming changes to libdebuginfod, using one client for all queries will avoid latency caused by unneccesarily setting up TCP connections multiple times. Tested on Fedora 33 x86_64. gdb/ChangeLog: * debuginfod-support.c (debuginfod_init): Remove. (get_debuginfod_client): New function. --- gdb/debuginfod-support.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 9778e2e4cfe..2d626e335a0 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -72,6 +72,7 @@ static int progressfn (debuginfod_client *c, long cur, long total) { user_data *data = static_cast (debuginfod_get_user_data (c)); + gdb_assert (data != nullptr); if (check_quit_flag ()) { @@ -103,15 +104,20 @@ progressfn (debuginfod_client *c, long cur, long total) return 0; } -static debuginfod_client_up -debuginfod_init () +static debuginfod_client * +get_debuginfod_client () { - debuginfod_client_up c (debuginfod_begin ()); + static debuginfod_client_up global_client; - if (c != nullptr) - debuginfod_set_progressfn (c.get (), progressfn); + if (global_client == nullptr) + { + global_client.reset (debuginfod_begin ()); + + if (global_client != nullptr) + debuginfod_set_progressfn (global_client.get (), progressfn); + } - return c; + return global_client.get (); } /* See debuginfod-support.h */ @@ -126,19 +132,20 @@ debuginfod_source_query (const unsigned char *build_id, if (urls_env_var == NULL || urls_env_var[0] == '\0') return scoped_fd (-ENOSYS); - debuginfod_client_up c = debuginfod_init (); + debuginfod_client *c = get_debuginfod_client (); if (c == nullptr) return scoped_fd (-ENOMEM); user_data data ("source file", srcpath); - debuginfod_set_user_data (c.get (), &data); - scoped_fd fd (debuginfod_find_source (c.get (), + debuginfod_set_user_data (c, &data); + scoped_fd fd (debuginfod_find_source (c, build_id, build_id_len, srcpath, nullptr)); + debuginfod_set_user_data (c, nullptr); /* TODO: Add 'set debug debuginfod' command to control when error messages are shown. */ if (fd.get () < 0 && fd.get () != -ENOENT) @@ -164,7 +171,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id, if (urls_env_var == NULL || urls_env_var[0] == '\0') return scoped_fd (-ENOSYS); - debuginfod_client_up c = debuginfod_init (); + debuginfod_client *c = get_debuginfod_client (); if (c == nullptr) return scoped_fd (-ENOMEM); @@ -172,9 +179,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id, char *dname = nullptr; user_data data ("separate debug info for", filename); - debuginfod_set_user_data (c.get (), &data); - scoped_fd fd (debuginfod_find_debuginfo (c.get (), build_id, build_id_len, + debuginfod_set_user_data (c, &data); + scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname)); + debuginfod_set_user_data (c, nullptr); if (fd.get () < 0 && fd.get () != -ENOENT) printf_filtered (_("Download failed: %s. Continuing without debug info for %ps.\n"), -- 2.30.2