* [PATCH] debuginfod-support.c: Use long-lived debuginfod_client @ 2021-04-30 23:57 Aaron Merey 2021-05-04 14:27 ` Tom Tromey 2021-05-06 0:55 ` Frank Ch. Eigler 0 siblings, 2 replies; 10+ messages in thread From: Aaron Merey @ 2021-04-30 23:57 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-04-30 23:57 [PATCH] debuginfod-support.c: Use long-lived debuginfod_client Aaron Merey @ 2021-05-04 14:27 ` Tom Tromey 2021-05-06 0:55 ` Frank Ch. Eigler 1 sibling, 0 replies; 10+ messages in thread From: Tom Tromey @ 2021-05-04 14:27 UTC (permalink / raw) To: Aaron Merey via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-04-30 23:57 [PATCH] debuginfod-support.c: Use long-lived debuginfod_client Aaron Merey 2021-05-04 14:27 ` Tom Tromey @ 2021-05-06 0:55 ` Frank Ch. Eigler 2021-05-06 17:27 ` Aaron Merey 1 sibling, 1 reply; 10+ messages in thread From: Frank Ch. Eigler @ 2021-05-06 0:55 UTC (permalink / raw) To: tom; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 0:55 ` Frank Ch. Eigler @ 2021-05-06 17:27 ` Aaron Merey 2021-05-06 17:39 ` Tom Tromey 2021-05-06 18:03 ` Simon Marchi 0 siblings, 2 replies; 10+ messages in thread From: Aaron Merey @ 2021-05-06 17:27 UTC (permalink / raw) To: Frank Ch. Eigler; +Cc: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 17:27 ` Aaron Merey @ 2021-05-06 17:39 ` Tom Tromey 2021-05-06 18:03 ` Simon Marchi 1 sibling, 0 replies; 10+ messages in thread From: Tom Tromey @ 2021-05-06 17:39 UTC (permalink / raw) To: Aaron Merey via Gdb-patches; +Cc: Frank Ch. Eigler, Aaron Merey, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 17:27 ` Aaron Merey 2021-05-06 17:39 ` Tom Tromey @ 2021-05-06 18:03 ` Simon Marchi 2021-05-06 18:47 ` Frank Ch. Eigler 1 sibling, 1 reply; 10+ messages in thread From: Simon Marchi @ 2021-05-06 18:03 UTC (permalink / raw) To: Aaron Merey, Frank Ch. Eigler; +Cc: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 18:03 ` Simon Marchi @ 2021-05-06 18:47 ` Frank Ch. Eigler 2021-05-06 19:11 ` Simon Marchi 0 siblings, 1 reply; 10+ messages in thread From: Frank Ch. Eigler @ 2021-05-06 18:47 UTC (permalink / raw) To: Simon Marchi; +Cc: Aaron Merey, Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 18:47 ` Frank Ch. Eigler @ 2021-05-06 19:11 ` Simon Marchi 2021-05-06 20:35 ` Aaron Merey 0 siblings, 1 reply; 10+ messages in thread From: Simon Marchi @ 2021-05-06 19:11 UTC (permalink / raw) To: Frank Ch. Eigler; +Cc: Aaron Merey, Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 19:11 ` Simon Marchi @ 2021-05-06 20:35 ` Aaron Merey 2021-05-06 21:45 ` Simon Marchi 0 siblings, 1 reply; 10+ messages in thread From: Aaron Merey @ 2021-05-06 20:35 UTC (permalink / raw) To: simon.marchi; +Cc: fche, tom, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debuginfod-support.c: Use long-lived debuginfod_client 2021-05-06 20:35 ` Aaron Merey @ 2021-05-06 21:45 ` Simon Marchi 0 siblings, 0 replies; 10+ messages in thread From: Simon Marchi @ 2021-05-06 21:45 UTC (permalink / raw) To: Aaron Merey; +Cc: fche, tom, gdb-patches > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-06 21:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-30 23:57 [PATCH] debuginfod-support.c: Use long-lived debuginfod_client Aaron Merey 2021-05-04 14:27 ` Tom Tromey 2021-05-06 0:55 ` Frank Ch. Eigler 2021-05-06 17:27 ` Aaron Merey 2021-05-06 17:39 ` Tom Tromey 2021-05-06 18:03 ` Simon Marchi 2021-05-06 18:47 ` Frank Ch. Eigler 2021-05-06 19:11 ` Simon Marchi 2021-05-06 20:35 ` Aaron Merey 2021-05-06 21:45 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).