debuginfod-support.c: Replace globals with user_data

Message ID CAJDtP-QeNpAvKwhnqBj0xYBpkPvP_4fvUBeFWWp_kFcFvZPXxQ@mail.gmail.com
State New
Headers show
Series
  • debuginfod-support.c: Replace globals with user_data
Related show

Commit Message

Hannes Domani via Gdb-patches July 31, 2020, 10:44 p.m.

Comments

Simon Marchi Aug. 1, 2020, 2:44 p.m. | #1
> 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
Hannes Domani via Gdb-patches Aug. 12, 2020, 9:39 p.m. | #2
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
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)
Simon Marchi Aug. 12, 2020, 9:45 p.m. | #3
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
Hannes Domani via Gdb-patches Aug. 13, 2020, 9:54 p.m. | #4
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

Patch

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