* [PATCH] debuginfod-support.c: Replace globals with user_data @ 2020-07-31 22:44 Aaron Merey 2020-08-01 14:44 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Aaron Merey @ 2020-07-31 22:44 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1 bytes --] [-- Attachment #2: 0001-debuginfod-support.c-Replace-globals-with-user_data.patch --] [-- Type: text/x-patch, Size: 5023 bytes --] From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001 From: Aaron Merey <amerey@redhat.com> Date: Fri, 31 Jul 2020 18:25:48 -0400 Subject: [PATCH] debuginfod-support.c: Replace globals with user_data Store query information in user_data struct instead of global variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE. gdb/ChangeLog: * Makefile.in: Add DEBUGINFOD_CFLAGS to INTERNAL_CFLAGS_BASE. * debuginfod-support.c: Replace global vars with struct user_data. --- gdb/ChangeLog | 5 +++++ gdb/Makefile.in | 8 ++++++-- gdb/debuginfod-support.c | 40 ++++++++++++++++++++++++---------------- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 9e718361a6..6e44e74f40 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2020-07-31 Aaron Merey <amerey@redhat.com> + + * Makefile.in: Add DEBUGINFOD_CFLAGS to INTERNAL_CFLAGS_BASE. + * debuginfod-support.c: Replace global vars with struct user_data. + 2020-07-30 Simon Marchi <simon.marchi@polymtl.ca> PR ada/26318 diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 0d6d8137b5..783949d054 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -214,6 +214,9 @@ GDB_WERROR_CFLAGS = $(WERROR_CFLAGS) PTHREAD_CFLAGS = @PTHREAD_CFLAGS@ PTHREAD_LIBS = @PTHREAD_LIBS@ +DEBUGINFOD_CFLAGS = @DEBUGINFOD_CFLAGS@ +DEBUGINFOD_LIBS = @DEBUGINFOD_LIBS@ + RDYNAMIC = @RDYNAMIC@ # Where is the INTL library? Typically in ../intl. @@ -598,7 +601,8 @@ INTERNAL_CFLAGS_BASE = \ $(GDB_CFLAGS) $(OPCODES_CFLAGS) $(READLINE_CFLAGS) $(ZLIBINC) \ $(BFD_CFLAGS) $(INCLUDE_CFLAGS) $(LIBDECNUMBER_CFLAGS) \ $(INTL_CFLAGS) $(INCGNU) $(INCSUPPORT) $(ENABLE_CFLAGS) \ - $(INTERNAL_CPPFLAGS) $(SRCHIGH_CFLAGS) $(TOP_CFLAGS) $(PTHREAD_CFLAGS) + $(INTERNAL_CPPFLAGS) $(SRCHIGH_CFLAGS) $(TOP_CFLAGS) $(PTHREAD_CFLAGS) \ + $(DEBUGINFOD_CFLAGS) INTERNAL_WARN_CFLAGS = $(INTERNAL_CFLAGS_BASE) $(GDB_WARN_CFLAGS) INTERNAL_CFLAGS = $(INTERNAL_WARN_CFLAGS) $(GDB_WERROR_CFLAGS) @@ -624,7 +628,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) \ $(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \ $(WIN32LIBS) $(LIBGNU) $(LIBICONV) \ $(LIBMPFR) $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) \ - @DEBUGINFOD_LIBS@ + $(DEBUGINFOD_LIBS) CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) $(CTF_DEPS) \ $(OPCODES) $(INTL_DEPS) $(LIBIBERTY) $(CONFIG_DEPS) $(LIBGNU) \ $(LIBSUPPORT) diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index f4a227b040..9325bf8651 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id, #else #include <elfutils/debuginfod.h> -/* TODO: Use debuginfod API extensions instead of these globals. */ -static std::string desc; -static std::string fname; -static bool has_printed; +struct user_data +{ + std::string desc; + std::string fname; + bool has_printed; + + user_data (std::string desc, std::string fname, bool has_printed) + : desc (desc), fname (fname), has_printed (has_printed) + { } +}; static int progressfn (debuginfod_client *c, long cur, long total) { + struct user_data *data = static_cast<user_data*> (debuginfod_get_user_data (c)); + if (check_quit_flag ()) { printf_filtered ("Cancelling download of %s %ps...\n", - desc.c_str (), - styled_string (file_name_style.style (), fname.c_str ())); + data->desc.c_str (), + styled_string (file_name_style.style (), + data->fname.c_str ())); return 1; } - if (!has_printed && total != 0) + if (! data->has_printed && total != 0) { /* Print this message only once. */ - has_printed = true; + data->has_printed = true; printf_filtered ("Downloading %s %ps...\n", - desc.c_str (), - styled_string (file_name_style.style (), fname.c_str ())); + data->desc.c_str (), + styled_string (file_name_style.style (), + data->fname.c_str ())); } return 0; @@ -98,10 +108,9 @@ debuginfod_source_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - desc = std::string ("source file"); - fname = std::string (srcpath); - has_printed = false; + struct user_data data ("source file", srcpath, false); + debuginfod_set_user_data (c, &data); scoped_fd fd (debuginfod_find_source (c, build_id, build_id_len, @@ -136,11 +145,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - desc = std::string ("separate debug info for"); - fname = std::string (filename); - has_printed = false; char *dname = nullptr; + struct user_data data ("separate debug info for", filename, false); + debuginfod_set_user_data (c, &data); scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname)); if (fd.get () < 0 && fd.get () != -ENOENT) -- 2.25.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] debuginfod-support.c: Replace globals with user_data 2020-07-31 22:44 [PATCH] debuginfod-support.c: Replace globals with user_data Aaron Merey @ 2020-08-01 14:44 ` Simon Marchi 2020-08-12 21:39 ` Aaron Merey 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2020-08-01 14:44 UTC (permalink / raw) To: Aaron Merey, gdb-patches > From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001 > From: Aaron Merey <amerey@redhat.com> > Date: Fri, 31 Jul 2020 18:25:48 -0400 > Subject: [PATCH] debuginfod-support.c: Replace globals with user_data > > Store query information in user_data struct instead of global > variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE. The DEBUGINFOD_CFLAGS change looks good, but it's unrelated to the topic of this patch, please make it its own patch, with its own justification. > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > index f4a227b040..9325bf8651 100644 > --- a/gdb/debuginfod-support.c > +++ b/gdb/debuginfod-support.c > @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > #else > #include <elfutils/debuginfod.h> > > -/* TODO: Use debuginfod API extensions instead of these globals. */ > -static std::string desc; > -static std::string fname; > -static bool has_printed; > +struct user_data > +{ > + std::string desc; > + std::string fname; > + bool has_printed; > + > + user_data (std::string desc, std::string fname, bool has_printed) > + : desc (desc), fname (fname), has_printed (has_printed) > + { } Put the constructor before the fields. Make the fields const if they are not intended to be changed once the object has been created. Using `std::string` causes unnecessary copying to happen. Just use `const char *` (`const char * const` for the fields) or gdb::string_view, if you want to be more generic (but all callers are `const char *`, so it's not really necessary). That's assuming that the lifetime of those strings will always be longer than the lifetime of the debuginfo client. I think that's the case, since the debug info client is always created and destroyed in the same function. > +}; > > static int > progressfn (debuginfod_client *c, long cur, long total) > { > + struct user_data *data = static_cast<user_data*> (debuginfod_get_user_data (c)); This line is too long, just skip the `struct` keyword and you'll be fine. Also, space before `*`. > + > if (check_quit_flag ()) > { > printf_filtered ("Cancelling download of %s %ps...\n", > - desc.c_str (), > - styled_string (file_name_style.style (), fname.c_str ())); > + data->desc.c_str (), > + styled_string (file_name_style.style (), > + data->fname.c_str ())); > return 1; > } > > - if (!has_printed && total != 0) > + if (! data->has_printed && total != 0) Remove space after `!`. Simon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] debuginfod-support.c: Replace globals with user_data 2020-08-01 14:44 ` Simon Marchi @ 2020-08-12 21:39 ` Aaron Merey 2020-08-12 21:45 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Aaron Merey @ 2020-08-12 21:39 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 2936 bytes --] On Sat, Aug 1, 2020 at 10:45 AM Simon Marchi <simark@simark.ca> wrote: > > > From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001 > > From: Aaron Merey <amerey@redhat.com> > > Date: Fri, 31 Jul 2020 18:25:48 -0400 > > Subject: [PATCH] debuginfod-support.c: Replace globals with user_data > > > > Store query information in user_data struct instead of global > > variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE. > > The DEBUGINFOD_CFLAGS change looks good, but it's unrelated to the topic of this patch, > please make it its own patch, with its own justification. > > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > > index f4a227b040..9325bf8651 100644 > > --- a/gdb/debuginfod-support.c > > +++ b/gdb/debuginfod-support.c > > @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > > #else > > #include <elfutils/debuginfod.h> > > > > -/* TODO: Use debuginfod API extensions instead of these globals. */ > > -static std::string desc; > > -static std::string fname; > > -static bool has_printed; > > +struct user_data > > +{ > > + std::string desc; > > + std::string fname; > > + bool has_printed; > > + > > + user_data (std::string desc, std::string fname, bool has_printed) > > + : desc (desc), fname (fname), has_printed (has_printed) > > + { } > > Put the constructor before the fields. Make the fields const if they are not intended > to be changed once the object has been created. > > Using `std::string` causes unnecessary copying to happen. Just use `const char *` > (`const char * const` for the fields) or gdb::string_view, if you want to be more > generic (but all callers are `const char *`, so it's not really necessary). That's > assuming that the lifetime of those strings will always be longer than the lifetime > of the debuginfo client. I think that's the case, since the debug info client is > always created and destroyed in the same function. > > > +}; > > > > static int > > progressfn (debuginfod_client *c, long cur, long total) > > { > > + struct user_data *data = static_cast<user_data*> (debuginfod_get_user_data (c)); > > This line is too long, just skip the `struct` keyword and you'll be fine. Also, space > before `*`. > > > + > > if (check_quit_flag ()) > > { > > printf_filtered ("Cancelling download of %s %ps...\n", > > - desc.c_str (), > > - styled_string (file_name_style.style (), fname.c_str ())); > > + data->desc.c_str (), > > + styled_string (file_name_style.style (), > > + data->fname.c_str ())); > > return 1; > > } > > > > - if (!has_printed && total != 0) > > + if (! data->has_printed && total != 0) > > Remove space after `!`. Hi Simon, Thanks for the review, I've updated the patch and moved the Makefile.in change to a separate patch. Aaron [-- Attachment #2: 0001-debuginfod-support.c-Replace-globals-with-user_data.patch --] [-- Type: text/x-patch, Size: 3422 bytes --] From 922179058218a82f9b74e328249e67244adae3fc Mon Sep 17 00:00:00 2001 From: Aaron Merey <amerey@redhat.com> Date: Wed, 12 Aug 2020 17:23:42 -0400 Subject: [PATCH 2/2] debuginfod-support.c: Replace globals with user_data Store query information in user_data struct instead of global variables. gdb/ChangeLog: * debuginfod-support.c: Replace global variables with user_data. --- gdb/ChangeLog | 4 ++++ gdb/debuginfod-support.c | 38 ++++++++++++++++++++++---------------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 0e035e233f..ad54c49864 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2020-08-12 Aaron Merey <amerey@redhat.com> + + * debuginfod-support.c: Replace global variables with user_data. + 2020-08-12 Aaron Merey <amerey@redhat.com> * Makefile.in (DEBUGINFOD_CFLAGS, DEBUGINFOD_LIBS): New variables. diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index f4a227b040..3f51aaaf43 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -43,29 +43,37 @@ debuginfod_debuginfo_query (const unsigned char *build_id, #else #include <elfutils/debuginfod.h> -/* TODO: Use debuginfod API extensions instead of these globals. */ -static std::string desc; -static std::string fname; -static bool has_printed; +struct user_data +{ + user_data (const char *desc, const char *fname, bool has_printed) + : desc (desc), fname (fname), has_printed (has_printed) + { } + + const char * const desc; + const char * const fname; + bool has_printed; +}; static int progressfn (debuginfod_client *c, long cur, long total) { + user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c)); + if (check_quit_flag ()) { printf_filtered ("Cancelling download of %s %ps...\n", - desc.c_str (), - styled_string (file_name_style.style (), fname.c_str ())); + data->desc, + styled_string (file_name_style.style (), data->fname)); return 1; } - if (!has_printed && total != 0) + if (!data->has_printed && total != 0) { /* Print this message only once. */ - has_printed = true; + data->has_printed = true; printf_filtered ("Downloading %s %ps...\n", - desc.c_str (), - styled_string (file_name_style.style (), fname.c_str ())); + data->desc, + styled_string (file_name_style.style (), data->fname)); } return 0; @@ -98,10 +106,9 @@ debuginfod_source_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - desc = std::string ("source file"); - fname = std::string (srcpath); - has_printed = false; + struct user_data data ("source file", srcpath, false); + debuginfod_set_user_data (c, &data); scoped_fd fd (debuginfod_find_source (c, build_id, build_id_len, @@ -136,11 +143,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - desc = std::string ("separate debug info for"); - fname = std::string (filename); - has_printed = false; char *dname = nullptr; + struct user_data data ("separate debug info for", filename, false); + debuginfod_set_user_data (c, &data); scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname)); if (fd.get () < 0 && fd.get () != -ENOENT) -- 2.25.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] debuginfod-support.c: Replace globals with user_data 2020-08-12 21:39 ` Aaron Merey @ 2020-08-12 21:45 ` Simon Marchi 2020-08-13 21:54 ` Aaron Merey 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2020-08-12 21:45 UTC (permalink / raw) To: Aaron Merey; +Cc: gdb-patches On 2020-08-12 5:39 p.m., Aaron Merey wrote: > Hi Simon, > > Thanks for the review, I've updated the patch and moved the Makefile.in > change to a separate patch. > > Aaron > Hi Aaron, Thanks, just a few minor things, the patch LGTM with those fixed. > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > index f4a227b040..3f51aaaf43 100644 > --- a/gdb/debuginfod-support.c > +++ b/gdb/debuginfod-support.c > @@ -43,29 +43,37 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > #else > #include <elfutils/debuginfod.h> > > -/* TODO: Use debuginfod API extensions instead of these globals. */ > -static std::string desc; > -static std::string fname; > -static bool has_printed; > +struct user_data > +{ > + user_data (const char *desc, const char *fname, bool has_printed) > + : desc (desc), fname (fname), has_printed (has_printed) There's no need for the `has_printed` parameter. Just initialize the `has_printed` field to false below. > + { } > + > + const char * const desc; > + const char * const fname; > + bool has_printed; > +}; > > static int > progressfn (debuginfod_client *c, long cur, long total) > { > + user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c)); > + > if (check_quit_flag ()) > { > printf_filtered ("Cancelling download of %s %ps...\n", > - desc.c_str (), > - styled_string (file_name_style.style (), fname.c_str ())); > + data->desc, > + styled_string (file_name_style.style (), data->fname)); > return 1; > } > > - if (!has_printed && total != 0) > + if (!data->has_printed && total != 0) > { > /* Print this message only once. */ > - has_printed = true; > + data->has_printed = true; > printf_filtered ("Downloading %s %ps...\n", > - desc.c_str (), > - styled_string (file_name_style.style (), fname.c_str ())); > + data->desc, > + styled_string (file_name_style.style (), data->fname)); > } > > return 0; > @@ -98,10 +106,9 @@ debuginfod_source_query (const unsigned char *build_id, > if (c == nullptr) > return scoped_fd (-ENOMEM); > > - desc = std::string ("source file"); > - fname = std::string (srcpath); > - has_printed = false; > + struct user_data data ("source file", srcpath, false); Omit the `struct` keyword when referring to the new type. > > + debuginfod_set_user_data (c, &data); > scoped_fd fd (debuginfod_find_source (c, > build_id, > build_id_len, > @@ -136,11 +143,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > if (c == nullptr) > return scoped_fd (-ENOMEM); > > - desc = std::string ("separate debug info for"); > - fname = std::string (filename); > - has_printed = false; > char *dname = nullptr; > + struct user_data data ("separate debug info for", filename, false); Same here. Simon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] debuginfod-support.c: Replace globals with user_data 2020-08-12 21:45 ` Simon Marchi @ 2020-08-13 21:54 ` Aaron Merey 0 siblings, 0 replies; 5+ messages in thread From: Aaron Merey @ 2020-08-13 21:54 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Wed, Aug 12, 2020 at 5:45 PM Simon Marchi <simark@simark.ca> wrote: > Hi Aaron, > > Thanks, just a few minor things, the patch LGTM with those fixed. > > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > > index f4a227b040..3f51aaaf43 100644 > > --- a/gdb/debuginfod-support.c > > +++ b/gdb/debuginfod-support.c > > @@ -43,29 +43,37 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > > #else > > #include <elfutils/debuginfod.h> > > > > -/* TODO: Use debuginfod API extensions instead of these globals. */ > > -static std::string desc; > > -static std::string fname; > > -static bool has_printed; > > +struct user_data > > +{ > > + user_data (const char *desc, const char *fname, bool has_printed) > > + : desc (desc), fname (fname), has_printed (has_printed) > > There's no need for the `has_printed` parameter. Just initialize the `has_printed` field to false below. > > > + { } > > + > > + const char * const desc; > > + const char * const fname; > > + bool has_printed; > > +}; > > > > static int > > progressfn (debuginfod_client *c, long cur, long total) > > { > > + user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c)); > > + > > if (check_quit_flag ()) > > { > > printf_filtered ("Cancelling download of %s %ps...\n", > > - desc.c_str (), > > - styled_string (file_name_style.style (), fname.c_str ())); > > + data->desc, > > + styled_string (file_name_style.style (), data->fname)); > > return 1; > > } > > > > - if (!has_printed && total != 0) > > + if (!data->has_printed && total != 0) > > { > > /* Print this message only once. */ > > - has_printed = true; > > + data->has_printed = true; > > printf_filtered ("Downloading %s %ps...\n", > > - desc.c_str (), > > - styled_string (file_name_style.style (), fname.c_str ())); > > + data->desc, > > + styled_string (file_name_style.style (), data->fname)); > > } > > > > return 0; > > @@ -98,10 +106,9 @@ debuginfod_source_query (const unsigned char *build_id, > > if (c == nullptr) > > return scoped_fd (-ENOMEM); > > > > - desc = std::string ("source file"); > > - fname = std::string (srcpath); > > - has_printed = false; > > + struct user_data data ("source file", srcpath, false); > > Omit the `struct` keyword when referring to the new type. > > > > > + debuginfod_set_user_data (c, &data); > > scoped_fd fd (debuginfod_find_source (c, > > build_id, > > build_id_len, > > @@ -136,11 +143,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > > if (c == nullptr) > > return scoped_fd (-ENOMEM); > > > > - desc = std::string ("separate debug info for"); > > - fname = std::string (filename); > > - has_printed = false; > > char *dname = nullptr; > > + struct user_data data ("separate debug info for", filename, false); > > Same here. Thanks Simon, I pushed the patch with these changes. Aaron ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-13 21:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-31 22:44 [PATCH] debuginfod-support.c: Replace globals with user_data Aaron Merey 2020-08-01 14:44 ` Simon Marchi 2020-08-12 21:39 ` Aaron Merey 2020-08-12 21:45 ` Simon Marchi 2020-08-13 21:54 ` Aaron Merey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox