[PATCH] gdb: Print debuginfod first-use notification

[PATCH] gdb: Print debuginfod first-use notification

[Aaron Merey]

When querying debuginfod servers for the first time, notify the user
that GDB may automatically download debuginfo.  Helps ensure users are
aware that GDB may be relying on the correct operation of remote
debuginfod servers.

In order to determine whether debuginfod is being queried for the
first time, check for the presence of a local debuginfod cache.
If one cannot be found, the notification will be displayed before
performing the first query.

ChangeLog/gdb:

	* debuginfod-support.c (notify_first_use): New function.
---
 gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..3d8667d0367 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -104,6 +104,42 @@ progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
+/* If this is the first time debuginfod is being used on this system, emit
+   a warning message indicating that GDB may automatically download debuginfo.
+
+   To determine whether debuginfod is being used for the first time, check for
+   the presence of a debuginfod cache.  If it cannot be found, then print the
+   warning.  */
+
+static void
+notify_first_use ()
+{
+  std::string cache_var (getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR) ?: "");
+
+  if (access (cache_var.c_str (), F_OK) == 0)
+    return;
+
+  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
+
+  if (!xdg.empty ())
+    {
+      xdg.append ("/debuginfod_client");
+      if (access (xdg.c_str (), F_OK) == 0)
+	return;
+    }
+
+  std::string home (getenv ("HOME") ?: "/");
+  std::string h1 = home + "/.debuginfod_client_cache";
+  std::string h2 = home + "/.cache/debuginfod_client";
+
+  if (access (h1.c_str (), F_OK) == 0 || access (h2.c_str (), F_OK) == 0)
+    return;
+
+  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
+		   getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "(URLs missing!)");
+  return;
+}
+
 static debuginfod_client *
 get_debuginfod_client ()
 {
@@ -114,7 +150,10 @@ get_debuginfod_client ()
       global_client.reset (debuginfod_begin ());
 
       if (global_client != nullptr)
-	debuginfod_set_progressfn (global_client.get (), progressfn);
+	{
+	  notify_first_use ();
+	  debuginfod_set_progressfn (global_client.get (), progressfn);
+	}
     }
 
   return global_client.get ();
-- 
2.31.1

[PING][PATCH] gdb: Print debuginfod first-use notification

[Aaron Merey]

Ping

Thanks,
Aaron

On Wed, May 26, 2021 at 9:27 PM Aaron Merey via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> When querying debuginfod servers for the first time, notify the user
> that GDB may automatically download debuginfo.  Helps ensure users are
> aware that GDB may be relying on the correct operation of remote
> debuginfod servers.
>
> In order to determine whether debuginfod is being queried for the
> first time, check for the presence of a local debuginfod cache.
> If one cannot be found, the notification will be displayed before
> performing the first query.
>
> ChangeLog/gdb:
>
>         * debuginfod-support.c (notify_first_use): New function.
> ---
>  gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..3d8667d0367 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -104,6 +104,42 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>
> +/* If this is the first time debuginfod is being used on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a debuginfod cache.  If it cannot be found, then print the
> +   warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  std::string cache_var (getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR) ?: "");
> +
> +  if (access (cache_var.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
> +
> +  if (!xdg.empty ())
> +    {
> +      xdg.append ("/debuginfod_client");
> +      if (access (xdg.c_str (), F_OK) == 0)
> +       return;
> +    }
> +
> +  std::string home (getenv ("HOME") ?: "/");
> +  std::string h1 = home + "/.debuginfod_client_cache";
> +  std::string h2 = home + "/.cache/debuginfod_client";
> +
> +  if (access (h1.c_str (), F_OK) == 0 || access (h2.c_str (), F_OK) == 0)
> +    return;
> +
> +  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
> +                  getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "(URLs missing!)");
> +  return;
> +}
> +
>  static debuginfod_client *
>  get_debuginfod_client ()
>  {
> @@ -114,7 +150,10 @@ get_debuginfod_client ()
>        global_client.reset (debuginfod_begin ());
>
>        if (global_client != nullptr)
> -       debuginfod_set_progressfn (global_client.get (), progressfn);
> +       {
> +         notify_first_use ();
> +         debuginfod_set_progressfn (global_client.get (), progressfn);
> +       }
>      }
>
>    return global_client.get ();
> --
> 2.31.1
>

[PING**2][PATCH] gdb: Print debuginfod first-use notification

[Aaron Merey]

Ping

Thanks,
Aaron

On Wed, May 26, 2021 at 9:27 PM Aaron Merey via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> When querying debuginfod servers for the first time, notify the user
> that GDB may automatically download debuginfo.  Helps ensure users are
> aware that GDB may be relying on the correct operation of remote
> debuginfod servers.
>
> In order to determine whether debuginfod is being queried for the
> first time, check for the presence of a local debuginfod cache.
> If one cannot be found, the notification will be displayed before
> performing the first query.
>
> ChangeLog/gdb:
>
>         * debuginfod-support.c (notify_first_use): New function.
> ---
>  gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..3d8667d0367 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -104,6 +104,42 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>
> +/* If this is the first time debuginfod is being used on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a debuginfod cache.  If it cannot be found, then print the
> +   warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  std::string cache_var (getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR) ?: "");
> +
> +  if (access (cache_var.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
> +
> +  if (!xdg.empty ())
> +    {
> +      xdg.append ("/debuginfod_client");
> +      if (access (xdg.c_str (), F_OK) == 0)
> +       return;
> +    }
> +
> +  std::string home (getenv ("HOME") ?: "/");
> +  std::string h1 = home + "/.debuginfod_client_cache";
> +  std::string h2 = home + "/.cache/debuginfod_client";
> +
> +  if (access (h1.c_str (), F_OK) == 0 || access (h2.c_str (), F_OK) == 0)
> +    return;
> +
> +  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
> +                  getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "(URLs missing!)");
> +  return;
> +}
> +
>  static debuginfod_client *
>  get_debuginfod_client ()
>  {
> @@ -114,7 +150,10 @@ get_debuginfod_client ()
>        global_client.reset (debuginfod_begin ());
>
>        if (global_client != nullptr)
> -       debuginfod_set_progressfn (global_client.get (), progressfn);
> +       {
> +         notify_first_use ();
> +         debuginfod_set_progressfn (global_client.get (), progressfn);
> +       }
>      }
>
>    return global_client.get ();
> --
> 2.31.1
>

Re: [PATCH] gdb: Print debuginfod first-use notification

[Simon Marchi]

On 2021-05-26 9:27 p.m., Aaron Merey via Gdb-patches wrote:
> When querying debuginfod servers for the first time, notify the user
> that GDB may automatically download debuginfo.  Helps ensure users are
> aware that GDB may be relying on the correct operation of remote
> debuginfod servers.
> 
> In order to determine whether debuginfod is being queried for the
> first time, check for the presence of a local debuginfod cache.
> If one cannot be found, the notification will be displayed before
> performing the first query.
> 
> ChangeLog/gdb:
> 
> 	* debuginfod-support.c (notify_first_use): New function.
> ---
>  gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..3d8667d0367 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -104,6 +104,42 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>  
> +/* If this is the first time debuginfod is being used on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a debuginfod cache.  If it cannot be found, then print the
> +   warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  std::string cache_var (getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR) ?: "");

Avoid the unnecessary copy by using `const char *`.

The use of "x ? : y" is a GNU extension, can we use that?  I don't
remember.  Perhaps it's better to do:

  const char *cache_var = getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR);
  if (cache_var == nullptr)
    return;

Since it's useless to do the access call below with an empty string, we
know it will fail.

> +
> +  if (access (cache_var.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
> +
> +  if (!xdg.empty ())
> +    {
> +      xdg.append ("/debuginfod_client");

Does debuginfod use XDG_CACHE_HOME regardless of the platform to find a
cache dir?  I think it would be much better if we could ask the
debuginfod library for the path to the local cache, so we don't bake-in
the location where we think debuginfod puts it (which could change in
the future).

Simon

[PATCH v2] gdb: Print debuginfod first-use notification

[Aaron Merey]

Hi Simon,

On Wed, Jul 14, 2021 at 9:02 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> The use of "x ? : y" is a GNU extension, can we use that?  I don't
> remember.  Perhaps it's better to do: 
>
>   const char *cache_var = getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR);
>   if (cache_var == nullptr)
>     return;
>
> Since it's useless to do the access call below with an empty string, we
> know it will fail.

Fixed.

> > + 
> > +  if (access (cache_var.c_str (), F_OK) == 0)
> > +    return;
> > + 
> > +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
> > + 
> > +  if (!xdg.empty ()) 
> > +    {
> > +      xdg.append ("/debuginfod_client");
>
> Does debuginfod use XDG_CACHE_HOME regardless of the platform to find a
> cache dir?  I think it would be much better if we could ask the 
> debuginfod library for the path to the local cache, so we don't bake-in
> the location where we think debuginfod puts it (which could change in
> the future).

To avoid this problem I changed the heuristic used to determine first use.
The presence of a 'debuginfod-used-p' file in the GDB config directory will
signal to GDB that the first use notice has already been printed. If the
file does not exist when the first query is about to happen then the file
is created and the notice is printed.

Aaron


From 0ee110b17624b187b6dfb55d4145756da582d8e3 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Tue, 20 Jul 2021 23:36:59 -0400
Subject: gdb: Print debuginfod first-use notification

When querying debuginfod servers for the first time, notify the user
that GDB may automatically download debuginfo.  Helps ensure users are
aware that GDB may be relying on the correct operation of remote
debuginfod servers.

In order to determine whether debuginfod is being queried for the
first time, check for the presence of a 'debuginfod-used-p' file
in the gdb config directory.  If one cannot be found, the notification
will be displayed before performing the first query.
---
 gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..2de703cc138 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -22,6 +22,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
 #include "gdbsupport/gdb_optional.h"
+#include "gdbsupport/pathstuff.h"
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -104,6 +105,41 @@ progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
+/* If this is the first time that GDB is using debuginfod on this system, emit
+   a warning message indicating that GDB may automatically download debuginfo.
+
+   To determine whether debuginfod is being used for the first time, check for
+   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
+   it is not found, then try to create the file and print the warning.  */
+
+static void
+notify_first_use ()
+{
+  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  if (urls == nullptr)
+    return;
+
+  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
+  if (access (used_p.c_str (), F_OK) == 0)
+    return;
+
+  std::string cache = get_standard_config_dir ();
+  if (cache.empty ())
+    {
+      warning (_("Cannot determine GDB config directory."));
+    }
+  else if (!mkdir_recursive (cache.c_str ())
+	   || open (used_p.c_str (), O_CREAT, 0) < 0)
+    {
+      warning (_("Cannot create debuginfod config file: %s"),
+	       safe_strerror (errno));
+    }
+
+  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
+		   urls);
+  return;
+}
+
 static debuginfod_client *
 get_debuginfod_client ()
 {
@@ -114,7 +150,10 @@ get_debuginfod_client ()
       global_client.reset (debuginfod_begin ());
 
       if (global_client != nullptr)
-	debuginfod_set_progressfn (global_client.get (), progressfn);
+	{
+	  notify_first_use ();
+	  debuginfod_set_progressfn (global_client.get (), progressfn);
+	}
     }
 
   return global_client.get ();
-- 
2.31.1

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Lancelot SIX]

On Wed, Jul 21, 2021 at 12:20:12AM -0400, Aaron Merey via Gdb-patches wrote:
> Hi Simon,
> 
> On Wed, Jul 14, 2021 at 9:02 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> > The use of "x ? : y" is a GNU extension, can we use that?  I don't
> > remember.  Perhaps it's better to do: 
> >
> >   const char *cache_var = getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR);
> >   if (cache_var == nullptr)
> >     return;
> >
> > Since it's useless to do the access call below with an empty string, we
> > know it will fail.
> 
> Fixed.
> 
> > > + 
> > > +  if (access (cache_var.c_str (), F_OK) == 0)
> > > +    return;
> > > + 
> > > +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");
> > > + 
> > > +  if (!xdg.empty ()) 
> > > +    {
> > > +      xdg.append ("/debuginfod_client");
> >
> > Does debuginfod use XDG_CACHE_HOME regardless of the platform to find a
> > cache dir?  I think it would be much better if we could ask the 
> > debuginfod library for the path to the local cache, so we don't bake-in
> > the location where we think debuginfod puts it (which could change in
> > the future).
> 
> To avoid this problem I changed the heuristic used to determine first use.
> The presence of a 'debuginfod-used-p' file in the GDB config directory will
> signal to GDB that the first use notice has already been printed. If the
> file does not exist when the first query is about to happen then the file
> is created and the notice is printed.
> 
> Aaron
> 
> 
> From 0ee110b17624b187b6dfb55d4145756da582d8e3 Mon Sep 17 00:00:00 2001
> From: Aaron Merey <amerey@redhat.com>
> Date: Tue, 20 Jul 2021 23:36:59 -0400
> Subject: gdb: Print debuginfod first-use notification
> 
> When querying debuginfod servers for the first time, notify the user
> that GDB may automatically download debuginfo.  Helps ensure users are
> aware that GDB may be relying on the correct operation of remote
> debuginfod servers.
> 
> In order to determine whether debuginfod is being queried for the
> first time, check for the presence of a 'debuginfod-used-p' file
> in the gdb config directory.  If one cannot be found, the notification
> will be displayed before performing the first query.
> ---
>  gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..2de703cc138 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -22,6 +22,7 @@
>  #include "gdbsupport/scoped_fd.h"
>  #include "debuginfod-support.h"
>  #include "gdbsupport/gdb_optional.h"
> +#include "gdbsupport/pathstuff.h"
>  
>  #ifndef HAVE_LIBDEBUGINFOD
>  scoped_fd
> @@ -104,6 +105,41 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>  
> +/* If this is the first time that GDB is using debuginfod on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
> +   it is not found, then try to create the file and print the warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> +  if (urls == nullptr)
> +    return;
> +
> +  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
> +  if (access (used_p.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string cache = get_standard_config_dir ();
> +  if (cache.empty ())
> +    {
> +      warning (_("Cannot determine GDB config directory."));
> +    }
> +  else if (!mkdir_recursive (cache.c_str ())
> +	   || open (used_p.c_str (), O_CREAT, 0) < 0)
> +    {
> +      warning (_("Cannot create debuginfod config file: %s"),
> +	       safe_strerror (errno));
> +    }
> +
> +  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
> +		   urls);

Hi,

I am not entirely sure where GDB stands with regards to
internationalization, but since you use _() above in the patch, I guess this
string could also qualify for translation and should be wrapped with _().

Best,
Lancelot.

> +  return;
> +}
> +
>  static debuginfod_client *
>  get_debuginfod_client ()
>  {
> @@ -114,7 +150,10 @@ get_debuginfod_client ()
>        global_client.reset (debuginfod_begin ());
>  
>        if (global_client != nullptr)
> -	debuginfod_set_progressfn (global_client.get (), progressfn);
> +	{
> +	  notify_first_use ();
> +	  debuginfod_set_progressfn (global_client.get (), progressfn);
> +	}
>      }
>  
>    return global_client.get ();
> -- 
> 2.31.1
> 

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Simon Marchi]

> @@ -104,6 +105,41 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>  
> +/* If this is the first time that GDB is using debuginfod on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
> +   it is not found, then try to create the file and print the warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> +  if (urls == nullptr)
> +    return;
> +
> +  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
> +  if (access (used_p.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string cache = get_standard_config_dir ();

cache -> config_dir (the cache dir is something else).

> +  if (cache.empty ())
> +    {
> +      warning (_("Cannot determine GDB config directory."));
> +    }

Remove braces for single statements.

> +  else if (!mkdir_recursive (cache.c_str ())
> +	   || open (used_p.c_str (), O_CREAT, 0) < 0)

This leaks the fd.  Not dramatic, but since open fds are a limited
resource, let's avoid leaving it open.

Use gdb_open_cloexec and store the result in a scoped_fd (not sure why
gdb_open_cloexec doesn't return a scoped_fd already, I'll look into it).

Simon

Re: [PATCH] gdb: Print debuginfod first-use notification

[Keith Seitz]

On 5/26/21 6:27 PM, Aaron Merey via Gdb-patches wrote:
> When querying debuginfod servers for the first time, notify the user
> that GDB may automatically download debuginfo.  Helps ensure users are
> aware that GDB may be relying on the correct operation of remote
> debuginfod servers.
> 
> In order to determine whether debuginfod is being queried for the
> first time, check for the presence of a local debuginfod cache.
> If one cannot be found, the notification will be displayed before
> performing the first query.

I don't mean to jump into the fray so late here, but I have one request and
one question regarding this.

First, please include a copy of the proposed output in the commit log.
It renders trivial the understanding of the intended change.

This proposed change seems suboptimal for GDB. IIUC the change comes as
a consequence of a Fedora CR proposal related to debuginfod. There,
developers worried about usage problems when debuginfod ran into, e.g.,
network issues. Users would have no clue why tools might be become
"suddenly" unresponsive.

While I share those concerns, I think we can do better than a simple
warning in GDB, given that GDB is an interactive tool.

Would it be more user-friendly to ask the user the first time GDB queries
debuginfod to download debuginfo?

Something like:

   This GDB is configured to auto-download debuginfo from
   https://debuginfod.elfutils.org. Continue? (y or n) 

Given that this query would only happen once, I don't think it would
necessarily be that intrusive. [If it was considered too disruptive,
we could add a setting to circumvent that query.]

Opinions?
Keith

Re: [PATCH] gdb: Print debuginfod first-use notification

[Aaron Merey]

Hi Keith,

On Thu, Jul 22, 2021 at 1:07 PM Keith Seitz <keiths@redhat.com> wrote:
> I don't mean to jump into the fray so late here, but I have one request and
> one question regarding this.
>
> First, please include a copy of the proposed output in the commit log.
> It renders trivial the understanding of the intended change.

Agreed.

> This proposed change seems suboptimal for GDB. IIUC the change comes as
> a consequence of a Fedora CR proposal related to debuginfod. There,
> developers worried about usage problems when debuginfod ran into, e.g.,
> network issues. Users would have no clue why tools might be become
> "suddenly" unresponsive.
>
> While I share those concerns, I think we can do better than a simple
> warning in GDB, given that GDB is an interactive tool.
>
> Would it be more user-friendly to ask the user the first time GDB queries
> debuginfod to download debuginfo?
>
> Something like:
>
>    This GDB is configured to auto-download debuginfo from
>    https://debuginfod.elfutils.org. Continue? (y or n)
>
> Given that this query would only happen once, I don't think it would
> necessarily be that intrusive. [If it was considered too disruptive,
> we could add a setting to circumvent that query.]
>
> Opinions?

If a user responds with 'no' we could print a message recommending they
unset $DEBUGINFOD_URLS and then exit GDB. Otherwise we may have to introduce
GDB-specific ways of enabling and disabling debuginfod. Although this
could be pretty straightforward with something like a 'set debuginfod on/off'
command, I do worry that this complicates GDB's debuginfod functionality
without much payoff.

Part of the client-side appeal of debuginfod is that it is extremely simple
to enable and configure via $DEBUGINFOD_URLS. If GDB is built with
debuginfod and the user has set $DEBUGINFOD_URLS and they see the
non-interactive first use notice mentioning that debuginfod is enabled,
I think it is safe to assume that the user is sufficiently informed and
that they had ample opportunity to disable debuginfod if they wished.

Aaron

Re: [PATCH] gdb: Print debuginfod first-use notification

[Frank Ch. Eigler]

Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:

> [...]
> This proposed change seems suboptimal for GDB. IIUC the change comes as
> a consequence of a Fedora CR proposal related to debuginfod. There,
> developers worried about usage problems when debuginfod ran into, e.g.,
> network issues. Users would have no clue why tools might be become
> "suddenly" unresponsive.
> [...]

Yeah, plus gdb's eager processing of all the debuginfo of a given binary
at startup can lead to very noticeable delays for a modern GUI program.
OTOH, we're adding additional debuginfod client control env vars, in
order to limit download sizes/times.

https://sourceware.org/bugzilla/show_bug.cgi?id=27982

- FChE

[PATCH v2] gdb: Print debuginfod first-use notification

[Aaron Merey]

Hi Simon,

I realized I never updated the debuginfod first-use notice patch[1]
with your feedback. I've included the revised patch below.

Keith also asked a good question[2] about whether or not to add 
interactivity to the first-use notice. Do you have any thoughts on this?

Thanks,
Aaron

[1] https://sourceware.org/pipermail/gdb-patches/2021-July/181038.html
[2] https://sourceware.org/pipermail/gdb-patches/2021-July/181081.html

From ddf5bb4b95b797351609625b8c69874132358ac4 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Tue, 28 Sep 2021 20:11:20 -0400
Subject: gdb: Print debuginfod first-use notification

When querying debuginfod servers for the first time, print the following
message to notify the user that GDB may automatically download debuginfo.

This GDB is configured to auto-download debuginfo from:
http://localhost:54321

This helps ensure users are aware that GDB may be relying on the correct
operation of remote debuginfod servers.

In order to determine whether debuginfod is being queried for the first
time, check for the presence of a 'debuginfod-used-p' file in the gdb
config directory.  If one cannot be found, the notification will be printed
before performing the first query.
---
 gdb/debuginfod-support.c | 42 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..6e88f8d53ef 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -22,6 +22,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
 #include "gdbsupport/gdb_optional.h"
+#include "gdbsupport/pathstuff.h"
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -104,6 +105,42 @@ progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
+/* If this is the first time that GDB is using debuginfod on this system, emit
+   a warning message indicating that GDB may automatically download debuginfo.
+
+   To determine whether debuginfod is being used for the first time, check for
+   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
+   it is not found, then try to create the file and print the warning.  */
+
+static void
+notify_first_use ()
+{
+  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  if (urls == nullptr)
+    return;
+
+  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
+  if (access (used_p.c_str (), F_OK) == 0)
+    return;
+
+  std::string config_dir = get_standard_config_dir ();
+  if (config_dir.empty ())
+    warning (_("Cannot determine GDB config directory."));
+
+  if (!mkdir_recursive (config_dir.c_str ()))
+    warning (_("Cannot create GDB config directory: %s"),
+	     safe_strerror (errno));
+
+  scoped_fd fd = (scoped_fd) gdb_open_cloexec (used_p.c_str (), O_CREAT, 0);
+  if (fd.get () < 0)
+    warning (_("Cannot create debuginfod config file: %s"),
+	     safe_strerror (errno));
+
+  printf_filtered (_("\nThis GDB is configured to auto-download " \
+		   "debuginfo from:\n%s\n\n"), urls);
+  return;
+}
+
 static debuginfod_client *
 get_debuginfod_client ()
 {
@@ -114,7 +151,10 @@ get_debuginfod_client ()
       global_client.reset (debuginfod_begin ());
 
       if (global_client != nullptr)
-	debuginfod_set_progressfn (global_client.get (), progressfn);
+	{
+	  notify_first_use ();
+	  debuginfod_set_progressfn (global_client.get (), progressfn);
+	}
     }
 
   return global_client.get ();
-- 
2.31.1

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Keith Seitz]

On 9/28/21 6:09 PM, Aaron Merey wrote:
> I realized I never updated the debuginfod first-use notice patch[1]
> with your feedback. I've included the revised patch below.

I took a "quick" look over the previous thread (thank you for linking it!)
and Simon's comments. AFAICT, all comments/recommendations have been addressed.

Although I will say, I am not a fan of:

> +  scoped_fd fd = (scoped_fd) gdb_open_cloexec (used_p.c_str (), O_CREAT, 0);

The more canonical way this is done in gdb is

  scoped_fd fd (gdb_open_cloexec (...));

YMMV

> Keith also asked a good question[2] about whether or not to add 
> interactivity to the first-use notice. Do you have any thoughts on this?

While I would like something more "interactive," I am not going
to stand in the way of progress (for which perfect is the proverbial enemy).

I recommend this patch be approved.

Keith

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Simon Marchi]

On 2021-09-28 21:09, Aaron Merey wrote:
> Hi Simon,
> 
> I realized I never updated the debuginfod first-use notice patch[1]
> with your feedback. I've included the revised patch below.
> 
> Keith also asked a good question[2] about whether or not to add 
> interactivity to the first-use notice. Do you have any thoughts on this?

I don't have a strong opinion on this.  If I install GDB from my distro
and it is pre-configured to fetch files from a remote debuginfo server,
it would indeed be nice for it to ask for permission first.  For
privacy reasons and those concerned with potential internet
consumption.

You would have to decide what to do if the first use of GDB is
non-interactive.  Do you default to yes or no?  In any case, it would be
bad because the user might never see the message.  So the choices should
probably be

 - yes (and remember)
 - yes (for this session)
 - no (and remember)
 - no (for this session)

And the default in case of non-interactivity would be "no (for this
session)".

An alternative: I still think it would be nice for the user to be able
to switch debuginfod usage on and off (and maybe even servers) using a
GDB setting (rather than changing env vars).  So let's say we had a "set
debuginfod <state>".  The default could be "ask".  That would make GDB
ask "Would you like to use debuginfod for this session?", to which you
could reply y or n.  If the terminal is non-interactive and the state is
"ask", then we assume "no".

We would also say "To make this setting permanent / avoid this
question", add "set debuginfod on/off" to your .gdbinit".  Then, we
wouldn't need the debuginfod-used-p file at all I think.  If the first
use of GDB on a system is non-interactive, the user would still see it
the next time.  I would personally put "set debuginfod on" in the
.gdbinit I carry everywhere, because I am fine with using it.  If I
exceptionally wanted to disable it, I would use `-iex "set debuginfod
off"`, something like that.

Comments on the current patch:

> @@ -104,6 +105,42 @@ progressfn (debuginfod_client *c, long cur, long total)
>    return 0;
>  }
>  
> +/* If this is the first time that GDB is using debuginfod on this system, emit
> +   a warning message indicating that GDB may automatically download debuginfo.
> +
> +   To determine whether debuginfod is being used for the first time, check for
> +   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
> +   it is not found, then try to create the file and print the warning.  */
> +
> +static void
> +notify_first_use ()
> +{
> +  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> +  if (urls == nullptr)
> +    return;
> +
> +  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
> +  if (access (used_p.c_str (), F_OK) == 0)
> +    return;
> +
> +  std::string config_dir = get_standard_config_dir ();
> +  if (config_dir.empty ())
> +    warning (_("Cannot determine GDB config directory."));
> +
> +  if (!mkdir_recursive (config_dir.c_str ()))
> +    warning (_("Cannot create GDB config directory: %s"),
> +	     safe_strerror (errno));
> +
> +  scoped_fd fd = (scoped_fd) gdb_open_cloexec (used_p.c_str (), O_CREAT, 0);
> +  if (fd.get () < 0)
> +    warning (_("Cannot create debuginfod config file: %s"),
> +	     safe_strerror (errno));

This is not really a "config file" (despite being in the config
directory).  I'd include the filename in the message to be more precise
about what file couldn't be created:

  "Cannot create %s: %s", used_p.c_str (), safe_strerror (errno)

And maybe same for the "Cannot create GDB config directory" above?  If
the user gets the path, it will be easier to correct the situation.

> +
> +  printf_filtered (_("\nThis GDB is configured to auto-download " \
> +		   "debuginfo from:\n%s\n\n"), urls);
> +  return;

Remove this return.

Simon

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Simon Marchi]

On 2021-09-29 13:48, Keith Seitz wrote:
> On 9/28/21 6:09 PM, Aaron Merey wrote:
>> I realized I never updated the debuginfod first-use notice patch[1]
>> with your feedback. I've included the revised patch below.
> 
> I took a "quick" look over the previous thread (thank you for linking it!)
> and Simon's comments. AFAICT, all comments/recommendations have been addressed.
> 
> Although I will say, I am not a fan of:
> 
>> +  scoped_fd fd = (scoped_fd) gdb_open_cloexec (used_p.c_str (), O_CREAT, 0);
> 
> The more canonical way this is done in gdb is
> 
>   scoped_fd fd (gdb_open_cloexec (...));

That reminds me I need to get back to this series:

https://sourceware.org/pipermail/gdb-patches/2021-July/181085.html

>> Keith also asked a good question[2] about whether or not to add 
>> interactivity to the first-use notice. Do you have any thoughts on this?
> 
> While I would like something more "interactive," I am not going
> to stand in the way of progress (for which perfect is the proverbial enemy).
> 
> I recommend this patch be approved.

Can you please check my suggestions in my other message?  One of them
is about adding a setting to control debuginfod usage, and would make
adding this debuginfod-used-p file unnecessary.  While it's always
possible to revert the patch, I think we should decide now which path we
want to take.

Simon

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Aaron Merey]

Hi Simon,

On Thu, Sep 30, 2021 at 11:41 AM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> I don't have a strong opinion on this.  If I install GDB from my distro
> and it is pre-configured to fetch files from a remote debuginfo server,
> it would indeed be nice for it to ask for permission first.  For
> privacy reasons and those concerned with potential internet
> consumption.
>
> You would have to decide what to do if the first use of GDB is
> non-interactive.  Do you default to yes or no?  In any case, it would be
> bad because the user might never see the message.  So the choices should
> probably be
>
>  - yes (and remember)
>  - yes (for this session)
>  - no (and remember)
>  - no (for this session)
>
> And the default in case of non-interactivity would be "no (for this
> session)".
>
> An alternative: I still think it would be nice for the user to be able
> to switch debuginfod usage on and off (and maybe even servers) using a
> GDB setting (rather than changing env vars).  So let's say we had a "set
> debuginfod <state>".  The default could be "ask".  That would make GDB
> ask "Would you like to use debuginfod for this session?", to which you
> could reply y or n.  If the terminal is non-interactive and the state is
> "ask", then we assume "no".
>
> We would also say "To make this setting permanent / avoid this
> question", add "set debuginfod on/off" to your .gdbinit".  Then, we
> wouldn't need the debuginfod-used-p file at all I think.  If the first
> use of GDB on a system is non-interactive, the user would still see it
> the next time.  I would personally put "set debuginfod on" in the
> .gdbinit I carry everywhere, because I am fine with using it.  If I
> exceptionally wanted to disable it, I would use `-iex "set debuginfod
> off"`, something like that.

Personally I'd rather stick to using just the environment variable in
order to control debuginfod.  I mentioned in my reply to Keith's original
suggestion that part of debuginfod's appeal is the ability to easily
configure and enable it across a whole system via $DEBUGINFOD_URLS.

If the user has this env var set and they get a (non-interactive) first
use notice in gdb I think it's safe to assume that they are sufficiently
informed and that they had enough opportunity to disable debuginfod if
they preferred.  So I don't think that we have much to gain by adding an
extra layer of control via 'set debuginfod on/off/ask'.

However if the consensus is that gdb would benefit from this then I'm
happy to add it.

> > +  scoped_fd fd = (scoped_fd) gdb_open_cloexec (used_p.c_str (), O_CREAT, 0);
> > +  if (fd.get () < 0)
> > +    warning (_("Cannot create debuginfod config file: %s"),
> > +          safe_strerror (errno));
>
> This is not really a "config file" (despite being in the config
> directory).  I'd include the filename in the message to be more precise
> about what file couldn't be created:
>
>   "Cannot create %s: %s", used_p.c_str (), safe_strerror (errno)
>
> And maybe same for the "Cannot create GDB config directory" above?  If
> the user gets the path, it will be easier to correct the situation.
>
> > +
> > +  printf_filtered (_("\nThis GDB is configured to auto-download " \
> > +                "debuginfo from:\n%s\n\n"), urls);
> > +  return;
>
> Remove this return.

Done.

Thanks,
Aaron

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Keith Seitz]

Hi, Simon,

Thank you for adding some input to this discussion. Hopefully we
can all come to an agreement and get this work completed.

On 9/30/21 8:41 AM, Simon Marchi wrote:
> On 2021-09-28 21:09, Aaron Merey wrote:
>>
>> Keith also asked a good question[2] about whether or not to add 
>> interactivity to the first-use notice. Do you have any thoughts on this?
> 
> I don't have a strong opinion on this.  If I install GDB from my distro
> and it is pre-configured to fetch files from a remote debuginfo server,
> it would indeed be nice for it to ask for permission first.  For
> privacy reasons and those concerned with potential internet
> consumption.

Agreed, but I have a slightly stronger-than-neutral opinion on this.
That's why I asked. :-)

> You would have to decide what to do if the first use of GDB is
> non-interactive.  Do you default to yes or no?  In any case, it would be
> bad because the user might never see the message.  So the choices should
> probably be
> 
>  - yes (and remember)
>  - yes (for this session)
>  - no (and remember)
>  - no (for this session)
> 
> And the default in case of non-interactivity would be "no (for this
> session)".

I've thought about how I think this should behave, but I keep flip-flopping
between default to however GDB is configured interactively and "no".
But I think as long as there is a switch to override (a la "set breakpoint
pending on/off"), this default choice isn't quite as consequential as
I'd otherwise fear.

> An alternative: I still think it would be nice for the user to be able
> to switch debuginfod usage on and off (and maybe even servers) using a
> GDB setting (rather than changing env vars).  So let's say we had a "set
> debuginfod <state>".  The default could be "ask".  That would make GDB
> ask "Would you like to use debuginfod for this session?", to which you
> could reply y or n.  If the terminal is non-interactive and the state is
> "ask", then we assume "no".

This is exactly what I was thinking of. There is an argument to be
made that the "user experience" be maintained across the toolchain,
but GDB is really a rather different beast than, say, objdump, readelf,
and other non-interactive tools.

> We would also say "To make this setting permanent / avoid this
> question", add "set debuginfod on/off" to your .gdbinit".  Then, we
> wouldn't need the debuginfod-used-p file at all I think.  If the first
> use of GDB on a system is non-interactive, the user would still see it
> the next time.  I would personally put "set debuginfod on" in the
> .gdbinit I carry everywhere, because I am fine with using it.  If I
> exceptionally wanted to disable it, I would use `-iex "set debuginfod
> off"`, something like that.

That echoes my thinking on this as well. -iex to the rescue for anyone
wishing to change the "default" behavior.

If I may attempt to codify what the "ideal" interface/feature set
might include...

* set/show debuginfod on/off/ask
  turns on/off debuginfod support
  defaults to "ask" (interactive); "off" (non-interactive)
  . on = always use debuginfod
  . off = never use debuginfod;
  . ask = ask once per-session whether to enable debuginfod
    ("ask" can be overidden by manually setting this option in .gdbinit
     or -iex "set debuginfod on/off")
    When this question is asked, GDB should print the helpful
    "To make this setting permanent, add ...".

* set/show debuginfod urls URL1[:URL2:...URLN]
  Set/show the server list used by debuginfod for debuginfo downloads
  defaults to env(DEBUGINFOD_URLS)

These also have the added benefit of being able to use "with" to temporarily
override the current settings, which I can convince myself might be very
convenient for some use cases.

WDYT? If we can arrive at agreement, I will help Aaron get this hammered
out.

Keith

Re: [PATCH v2] gdb: Print debuginfod first-use notification

[Simon Marchi]

On 2021-09-30 13:03, Aaron Merey wrote:
> Hi Simon,
> 
> On Thu, Sep 30, 2021 at 11:41 AM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>> I don't have a strong opinion on this.  If I install GDB from my distro
>> and it is pre-configured to fetch files from a remote debuginfo server,
>> it would indeed be nice for it to ask for permission first.  For
>> privacy reasons and those concerned with potential internet
>> consumption.
>>
>> You would have to decide what to do if the first use of GDB is
>> non-interactive.  Do you default to yes or no?  In any case, it would be
>> bad because the user might never see the message.  So the choices should
>> probably be
>>
>>  - yes (and remember)
>>  - yes (for this session)
>>  - no (and remember)
>>  - no (for this session)
>>
>> And the default in case of non-interactivity would be "no (for this
>> session)".
>>
>> An alternative: I still think it would be nice for the user to be able
>> to switch debuginfod usage on and off (and maybe even servers) using a
>> GDB setting (rather than changing env vars).  So let's say we had a "set
>> debuginfod <state>".  The default could be "ask".  That would make GDB
>> ask "Would you like to use debuginfod for this session?", to which you
>> could reply y or n.  If the terminal is non-interactive and the state is
>> "ask", then we assume "no".
>>
>> We would also say "To make this setting permanent / avoid this
>> question", add "set debuginfod on/off" to your .gdbinit".  Then, we
>> wouldn't need the debuginfod-used-p file at all I think.  If the first
>> use of GDB on a system is non-interactive, the user would still see it
>> the next time.  I would personally put "set debuginfod on" in the
>> .gdbinit I carry everywhere, because I am fine with using it.  If I
>> exceptionally wanted to disable it, I would use `-iex "set debuginfod
>> off"`, something like that.
> 
> Personally I'd rather stick to using just the environment variable in
> order to control debuginfod.  I mentioned in my reply to Keith's original
> suggestion that part of debuginfod's appeal is the ability to easily
> configure and enable it across a whole system via $DEBUGINFOD_URLS.

The "set debuginfod urls" value could be populated at startup with the
content of the DEBUGINFOD_URLS env var.  That would make it "just work"
in GDB by setting DEBUGINFOD_URLS.

GDB users are used to configuring GDB using settings.  I can't think of
anything in GDB that is configurable _only_ using an environment
variable.  The index-cache default location, for example, is chosen
based on the XDG_CACHE_HOME env var, but the user can override it using
a setting after that.

> If the user has this env var set and they get a (non-interactive) first
> use notice in gdb I think it's safe to assume that they are sufficiently
> informed and that they had enough opportunity to disable debuginfod if
> they preferred.  So I don't think that we have much to gain by adding an
> extra layer of control via 'set debuginfod on/off/ask'.

I don't agree.  The user may not see the output of the non-interactive
execution.  For example, a script/tool that extracts info from a core
dump using GDB, a GDB GUI front-end, etc.  So they might never see it.

Simon