[PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Aaron Merey]

Apologies, resending this patch with a proper subject.

Aaron

From 54fa19c47f9beca1eaabd0f758333a9445084c0c Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Fri, 30 Apr 2021 18:58:36 -0400
Subject: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

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): Use one client for
	all queries.
	(struct debuginfod_client_deleter): Remove.
---
 gdb/debuginfod-support.c | 41 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 9778e2e4cfe..357cd3f6d01 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -55,23 +55,13 @@ struct user_data
   gdb::optional<ui_out::progress_meter> meter;
 };
 
-/* Deleter for a debuginfod_client.  */
-
-struct debuginfod_client_deleter
-{
-  void operator() (debuginfod_client *c)
-  {
-    debuginfod_end (c);
-  }
-};
-
-using debuginfod_client_up
-  = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
+static debuginfod_client *global_client = nullptr;
 
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
   user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
+  gdb_assert (data != nullptr);
 
   if (check_quit_flag ())
     {
@@ -103,15 +93,18 @@ progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
-static debuginfod_client_up
+static debuginfod_client *
 debuginfod_init ()
 {
-  debuginfod_client_up c (debuginfod_begin ());
+  if (global_client == nullptr)
+    {
+      global_client = debuginfod_begin ();
 
-  if (c != nullptr)
-    debuginfod_set_progressfn (c.get (), progressfn);
+      if (global_client != nullptr)
+	debuginfod_set_progressfn (global_client, progressfn);
+    }
 
-  return c;
+  return global_client;
 }
 
 /* See debuginfod-support.h  */
@@ -126,19 +119,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 = debuginfod_init ();
 
   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 +158,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 = debuginfod_init ();
 
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
@@ -172,9 +166,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

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Instead of initializing a new debuginfod_client for each query, store
Aaron> the first initialized client for the remainder of the GDB session and
Aaron> use it for every debuginfod query.

Will this still work with existing versions of the library?

Aaron> +static debuginfod_client *global_client = nullptr;

Will there ever be a need to finalize this object?  Like, shut it down
cleanly in some way?

Aaron> -static debuginfod_client_up
Aaron> +static debuginfod_client *
Aaron>  debuginfod_init ()
Aaron>  {
Aaron> -  debuginfod_client_up c (debuginfod_begin ());
Aaron> +  if (global_client == nullptr)

If global_client were defined here, rather than globally, then that
would help enforce the rule that the only way to get a
debuginfod_client* is to call debuginfod_init.

Other than this nit, the patch looks good to me.

thanks,
Tom

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Frank Ch. Eigler]

Hi -

> Aaron> Instead of initializing a new debuginfod_client for each query,
> Aaron> store the first initialized client for the remainder of the GDB
> Aaron> session and use it for every debuginfod query.

> Will this still work with existing versions of the library?

Yes.

> Aaron> +static debuginfod_client *global_client = nullptr;

> Will there ever be a need to finalize this object?  Like, shut it down
> cleanly in some way?

Nope (except perhaps if running gdb under valgrind?).


- FChE

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Aaron Merey]

On Wed, May 5, 2021 at 8:56 PM Frank Ch. Eigler via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> > Will there ever be a need to finalize this object?  Like, shut it down
> > cleanly in some way?
>
> Nope (except perhaps if running gdb under valgrind?).

Running gdb under valgrind appears to work properly and no
debuginfod-related leaks are reported.

If there aren't any other concerns then I will merge this patch with
Tom's suggestion to define global_client in debuginfod_init() included.

Aaron

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> If there aren't any other concerns then I will merge this patch with
Aaron> Tom's suggestion to define global_client in debuginfod_init() included.

Sounds good, thank you.

Tom

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Simon Marchi]

On 2021-05-06 1:27 p.m., Aaron Merey via Gdb-patches wrote:
> On Wed, May 5, 2021 at 8:56 PM Frank Ch. Eigler via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>>> Will there ever be a need to finalize this object?  Like, shut it down
>>> cleanly in some way?
>>
>> Nope (except perhaps if running gdb under valgrind?).
> 
> Running gdb under valgrind appears to work properly and no
> debuginfod-related leaks are reported.
> 
> If there aren't any other concerns then I will merge this patch with
> Tom's suggestion to define global_client in debuginfod_init() included.

That might be because Valgrind does not show the memory that is still
allocated at program exit that is still reachable by a global variable.

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.   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.

Simon

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Frank Ch. Eigler]

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?


> 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.)


- FChE

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Simon Marchi]

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.

Simon

[PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Aaron Merey]

On Thu, May 6, 2021 at 3:12 PM Simon Marchi <simon.marchi@polymtl.ca> 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<user_data *> (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

Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Simon Marchi]

> 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.

LGTM, thanks!

Simon