* [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query
@ 2023-02-27 19:42 Aaron Merey
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
` (7 more replies)
0 siblings, 8 replies; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
Add new function debuginfod_section_query. This function queries
debuginfod servers for an individual ELF/DWARF section associated with
a given build-id.
Also check for libdebuginfod version >= 0.188 at configure time.
debuginfod_section_query simply returns -ENOSYS if this condition
is not met.
---
config/debuginfod.m4 | 27 +++++++++++
gdb/config.in | 3 ++
gdb/configure | 102 ++++++++++++++++++++++++++++++++++++++-
gdb/configure.ac | 2 +-
gdb/debuginfod-support.c | 55 +++++++++++++++++++++
gdb/debuginfod-support.h | 24 +++++++++
6 files changed, 211 insertions(+), 2 deletions(-)
diff --git a/config/debuginfod.m4 b/config/debuginfod.m4
index 2c1bfbdb544..203a799719d 100644
--- a/config/debuginfod.m4
+++ b/config/debuginfod.m4
@@ -26,3 +26,30 @@ else
AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
fi
])
+
+AC_DEFUN([AC_DEBUGINFOD_SECTION],
+[
+# Handle optional debuginfod support as well as optional section downloading support
+AC_ARG_WITH([debuginfod],
+ AC_HELP_STRING([--with-debuginfod], [Enable debuginfo lookups with debuginfod (auto/yes/no)]),
+ [], [with_debuginfod=auto])
+AC_MSG_CHECKING([whether to use debuginfod])
+AC_MSG_RESULT([$with_debuginfod])
+
+if test "x$with_debuginfod" != xno; then
+ PKG_CHECK_MODULES([DEBUGINFOD], [libdebuginfod >= 0.188],
+ [AC_DEFINE([HAVE_DEBUGINFOD_FIND_SECTION], [1],
+ [Define to 1 if debuginfod section downloading is supported.])],
+ [AC_MSG_WARN([libdebuginfod is missing or some features may be unavailable.])])
+
+ PKG_CHECK_MODULES([DEBUGINFOD], [libdebuginfod >= 0.179],
+ [AC_DEFINE([HAVE_LIBDEBUGINFOD], [1], [Define to 1 if debuginfod is enabled.])],
+ [if test "x$with_debuginfod" = xyes; then
+ AC_MSG_ERROR(["--with-debuginfod was given, but libdebuginfod is missing or unusable."])
+ else
+ AC_MSG_WARN([libdebuginfod is missing or unusable; some features may be unavailable.])
+ fi])
+else
+ AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
+fi
+])
diff --git a/gdb/config.in b/gdb/config.in
index a7da88b92d7..157acd46c7c 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -99,6 +99,9 @@
/* define if the compiler supports basic C++11 syntax */
#undef HAVE_CXX11
+/* Define to 1 if debuginfod section downloading is supported. */
+#undef HAVE_DEBUGINFOD_FIND_SECTION
+
/* Define to 1 if you have the declaration of `ADDR_NO_RANDOMIZE', and to 0 if
you don't. */
#undef HAVE_DECL_ADDR_NO_RANDOMIZE
diff --git a/gdb/configure b/gdb/configure
index 017ec05e4b7..338460c07bd 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -18349,7 +18349,7 @@ esac
# Handle optional debuginfod support
-# Handle optional debuginfod support
+# Handle optional debuginfod support as well as optional section downloading support
# Check whether --with-debuginfod was given.
if test "${with_debuginfod+set}" = set; then :
@@ -18365,6 +18365,106 @@ $as_echo "$with_debuginfod" >&6; }
if test "x$with_debuginfod" != xno; then
+pkg_failed=no
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libdebuginfod >= 0.188" >&5
+$as_echo_n "checking for libdebuginfod >= 0.188... " >&6; }
+
+if test -n "$DEBUGINFOD_CFLAGS"; then
+ pkg_cv_DEBUGINFOD_CFLAGS="$DEBUGINFOD_CFLAGS"
+ elif test -n "$PKG_CONFIG"; then
+ if test -n "$PKG_CONFIG" && \
+ { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libdebuginfod >= 0.188\""; } >&5
+ ($PKG_CONFIG --exists --print-errors "libdebuginfod >= 0.188") 2>&5
+ ac_status=$?
+ $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+ test $ac_status = 0; }; then
+ pkg_cv_DEBUGINFOD_CFLAGS=`$PKG_CONFIG --cflags "libdebuginfod >= 0.188" 2>/dev/null`
+ test "x$?" != "x0" && pkg_failed=yes
+else
+ pkg_failed=yes
+fi
+ else
+ pkg_failed=untried
+fi
+if test -n "$DEBUGINFOD_LIBS"; then
+ pkg_cv_DEBUGINFOD_LIBS="$DEBUGINFOD_LIBS"
+ elif test -n "$PKG_CONFIG"; then
+ if test -n "$PKG_CONFIG" && \
+ { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libdebuginfod >= 0.188\""; } >&5
+ ($PKG_CONFIG --exists --print-errors "libdebuginfod >= 0.188") 2>&5
+ ac_status=$?
+ $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+ test $ac_status = 0; }; then
+ pkg_cv_DEBUGINFOD_LIBS=`$PKG_CONFIG --libs "libdebuginfod >= 0.188" 2>/dev/null`
+ test "x$?" != "x0" && pkg_failed=yes
+else
+ pkg_failed=yes
+fi
+ else
+ pkg_failed=untried
+fi
+
+if test $pkg_failed = no; then
+ pkg_save_LDFLAGS="$LDFLAGS"
+ LDFLAGS="$LDFLAGS $pkg_cv_DEBUGINFOD_LIBS"
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+
+else
+ pkg_failed=yes
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+ LDFLAGS=$pkg_save_LDFLAGS
+fi
+
+
+
+if test $pkg_failed = yes; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+
+if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then
+ _pkg_short_errors_supported=yes
+else
+ _pkg_short_errors_supported=no
+fi
+ if test $_pkg_short_errors_supported = yes; then
+ DEBUGINFOD_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "libdebuginfod >= 0.188" 2>&1`
+ else
+ DEBUGINFOD_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "libdebuginfod >= 0.188" 2>&1`
+ fi
+ # Put the nasty error message in config.log where it belongs
+ echo "$DEBUGINFOD_PKG_ERRORS" >&5
+
+ { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: libdebuginfod is missing or some features may be unavailable." >&5
+$as_echo "$as_me: WARNING: libdebuginfod is missing or some features may be unavailable." >&2;}
+elif test $pkg_failed = untried; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+ { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: libdebuginfod is missing or some features may be unavailable." >&5
+$as_echo "$as_me: WARNING: libdebuginfod is missing or some features may be unavailable." >&2;}
+else
+ DEBUGINFOD_CFLAGS=$pkg_cv_DEBUGINFOD_CFLAGS
+ DEBUGINFOD_LIBS=$pkg_cv_DEBUGINFOD_LIBS
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+
+$as_echo "#define HAVE_DEBUGINFOD_FIND_SECTION 1" >>confdefs.h
+
+fi
+
+
pkg_failed=no
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libdebuginfod >= 0.179" >&5
$as_echo_n "checking for libdebuginfod >= 0.179... " >&6; }
diff --git a/gdb/configure.ac b/gdb/configure.ac
index fb43cd10d6c..499802bb4c9 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -342,7 +342,7 @@ case $host_os in
esac
# Handle optional debuginfod support
-AC_DEBUGINFOD
+AC_DEBUGINFOD_SECTION
# Libunwind support for ia64.
AC_ARG_WITH(libunwind-ia64,
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 04d254a1601..f909742362f 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -80,6 +80,15 @@ debuginfod_exec_query (const unsigned char *build_id,
return scoped_fd (-ENOSYS);
}
+scoped_fd
+debuginfod_section_query (const unsigned char *build_id,
+ int build_id_len,
+ const char *filename,
+ const char *section_name,
+ gdb::unique_xmalloc_ptr<char> *destname)
+{
+ return scoped_fd (-ENOSYS);
+}
#define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
#else
@@ -388,6 +397,52 @@ debuginfod_exec_query (const unsigned char *build_id,
return fd;
}
+
+/* See debuginfod-support.h */
+
+scoped_fd
+debuginfod_section_query (const unsigned char *build_id,
+ int build_id_len,
+ const char *filename,
+ const char *section_name,
+ gdb::unique_xmalloc_ptr<char> *destname)
+{
+#if !defined (HAVE_DEBUGINFOD_FIND_SECTION)
+ return scoped_fd (-ENOSYS);
+#else
+
+ if (!debuginfod_is_enabled ())
+ return scoped_fd (-ENOSYS);
+
+ debuginfod_client *c = get_debuginfod_client ();
+
+ if (c == nullptr)
+ return scoped_fd (-ENOMEM);
+
+ char *dname = nullptr;
+ std::string desc = std::string ("section ") + section_name + " for";
+ user_data data (desc.c_str (), filename);
+
+ debuginfod_set_user_data (c, &data);
+ gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
+ if (target_supports_terminal_ours ())
+ {
+ term_state.emplace ();
+ target_terminal::ours ();
+ }
+
+ scoped_fd fd (debuginfod_find_section (c, build_id, build_id_len,
+ section_name, &dname));
+ debuginfod_set_user_data (c, nullptr);
+ print_outcome (data, fd.get ());
+
+ if (fd.get () >= 0 && destname != nullptr)
+ destname->reset (dname);
+
+ return fd;
+#endif /* HAVE_DEBUGINFOD_FIND_SECTION */
+}
+
#endif
/* Set callback for "set debuginfod enabled". */
diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
index 633600a79da..9701e3b4685 100644
--- a/gdb/debuginfod-support.h
+++ b/gdb/debuginfod-support.h
@@ -81,4 +81,28 @@ extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
const char *filename,
gdb::unique_xmalloc_ptr<char>
*destname);
+
+/* Query debuginfod servers for the binary contents of a ELF/DWARF section
+ from a file matching BUILD_ID. BUILD_ID can be given as a binary blob
+ or a null-terminated string. If given as a binary blob, BUILD_ID_LEN
+ should be the number of bytes. If given as a null-terminated string,
+ BUILD_ID_LEN should be 0.
+
+ FILENAME should be the name or path associated with the file matching
+ BUILD_ID. It is used for printing messages to the user.
+
+ SECTION_NAME should be the name of an ELF/DWARF section.
+
+ If the file is successfully retrieved, return a file descriptor and store
+ the file's local path in DESTNAME. If unsuccessful, print an error message
+ and return a negative errno. If GDB is not built with debuginfod or
+ libdebuginfod does not support section queries, this function returns
+ -ENOSYS. */
+
+extern scoped_fd debuginfod_section_query (const unsigned char *build_id,
+ int build_id_len,
+ const char *filename,
+ const char *section_name,
+ gdb::unique_xmalloc_ptr<char>
+ *destname);
#endif /* DEBUGINFOD_SUPPORT_H */
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled'
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-02-27 19:54 ` Eli Zaretskii
2023-05-24 9:31 ` Andrew Burgess
2023-02-27 19:42 ` [PATCH 3/7] gdb/debuginfod: disable pagination during downloads Aaron Merey
` (6 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
'set debuginfod enabled lazy' turns on debuginfod downloading like
'set debuginfod enabled on' but also enables ELF/DWARFs section
downloading via debuginfod_section_query.
If support for debuginfod section queries was not found at configure
time, 'set debuginfod enabled lazy' will print an error message
indicating the missing support and default to 'set debuginfod enabled on'.
Also update the help text and gdb.texinfo section for 'set debuginfod enabled'
with information on the lazy setting.
---
gdb/debuginfod-support.c | 20 +++++++++++++++++---
gdb/doc/gdb.texinfo | 9 +++++++--
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index f909742362f..12025fcf0c0 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -34,12 +34,14 @@ static cmd_list_element *show_debuginfod_prefix_list;
static const char debuginfod_on[] = "on";
static const char debuginfod_off[] = "off";
static const char debuginfod_ask[] = "ask";
+static const char debuginfod_lazy[] = "lazy";
static const char *debuginfod_enabled_enum[] =
{
debuginfod_on,
debuginfod_off,
debuginfod_ask,
+ debuginfod_lazy,
nullptr
};
@@ -411,7 +413,7 @@ debuginfod_section_query (const unsigned char *build_id,
return scoped_fd (-ENOSYS);
#else
- if (!debuginfod_is_enabled ())
+ if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
return scoped_fd (-ENOSYS);
debuginfod_client *c = get_debuginfod_client ();
@@ -451,6 +453,14 @@ static void
set_debuginfod_enabled (const char *value)
{
#if defined(HAVE_LIBDEBUGINFOD)
+#if !defined(HAVE_DEBUGINFOD_FIND_SECTION)
+ if (value == debuginfod_lazy)
+ {
+ debuginfod_enabled = debuginfod_on;
+ error (_("Support for lazy downloading is not compiled into GDB. " \
+"Defaulting to \"on\"."));
+ }
+#endif
debuginfod_enabled = value;
#else
/* Disabling debuginfod when gdb is not built with it is a no-op. */
@@ -550,8 +560,12 @@ _initialize_debuginfod ()
_("Set whether to use debuginfod."),
_("Show whether to use debuginfod."),
_("\
-When on, enable the use of debuginfod to download missing debug info and\n\
-source files."),
+When set to \"on\", enable the use of debuginfod to download missing\n\
+debug info and source files. \"off\" disables the use of debuginfod.\n\
+When set to \"ask\", a prompt may ask whether to enable or disable\n\
+debuginfod. When set to \"lazy\", debug info downloading will be\n\
+deferred until it is required. GDB may also download components of\n\
+debug info instead of entire files." ),
set_debuginfod_enabled,
get_debuginfod_enabled,
show_debuginfod_enabled,
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c1ca45521ea..ac0636e3115 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -48757,10 +48757,15 @@ debug info or source files. By default, @code{debuginfod enabled} is set to
attempting to perform the next query. By default, @code{debuginfod enabled}
is set to @code{ask} for interactive sessions.
+@item set debuginfod enabled lazy
+@value{GDBN} will attempt to defer downloading entire debug info files until
+necessary. @value{GDBN} may instead download individual components of the
+debug info files such as @code{.gdb_index}.
+
@kindex show debuginfod enabled
@item show debuginfod enabled
-Display whether @code{debuginfod enabled} is set to @code{on}, @code{off} or
-@code{ask}.
+Display whether @code{debuginfod enabled} is set to @code{on}, @code{off},
+@code{ask} or @code{lazy}.
@kindex set debuginfod urls
@cindex configure debuginfod URLs
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/7] gdb/debuginfod: disable pagination during downloads
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-03-03 21:33 ` Tom Tromey
2023-05-24 9:38 ` Andrew Burgess
2023-02-27 19:42 ` [PATCH 4/7] gdb/ui-file: Add newline tracking Aaron Merey
` (5 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
Disable pagination during downloads in order to avoid inconvenient
continue prompts "--Type <RET> for more, q to quit...".
For more discussion on this issue see the following thread
https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html
---
gdb/debuginfod-support.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 12025fcf0c0..f4969e94b0a 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -292,6 +292,9 @@ debuginfod_source_query (const unsigned char *build_id,
const char *srcpath,
gdb::unique_xmalloc_ptr<char> *destname)
{
+ scoped_restore save_count_lines_printed
+ = make_scoped_restore (&pagination_enabled, false);
+
if (!debuginfod_is_enabled ())
return scoped_fd (-ENOSYS);
@@ -333,6 +336,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
const char *filename,
gdb::unique_xmalloc_ptr<char> *destname)
{
+ scoped_restore save_count_lines_printed
+ = make_scoped_restore (&pagination_enabled, false);
+
if (!debuginfod_is_enabled ())
return scoped_fd (-ENOSYS);
@@ -371,6 +377,9 @@ debuginfod_exec_query (const unsigned char *build_id,
const char *filename,
gdb::unique_xmalloc_ptr<char> *destname)
{
+ scoped_restore save_count_lines_printed
+ = make_scoped_restore (&pagination_enabled, false);
+
if (!debuginfod_is_enabled ())
return scoped_fd (-ENOSYS);
@@ -412,6 +421,8 @@ debuginfod_section_query (const unsigned char *build_id,
#if !defined (HAVE_DEBUGINFOD_FIND_SECTION)
return scoped_fd (-ENOSYS);
#else
+ scoped_restore save_count_lines_printed
+ = make_scoped_restore (&pagination_enabled, false);
if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
return scoped_fd (-ENOSYS);
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/7] gdb/ui-file: Add newline tracking
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
2023-02-27 19:42 ` [PATCH 3/7] gdb/debuginfod: disable pagination during downloads Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-03-07 19:33 ` Tom Tromey
2023-02-27 19:42 ` [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading Aaron Merey
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
Add a new field line_state to ui_file's that indicates whether
the character most recently written to the ui_file's stream is
a newline.
The purpose of this is to help debuginfod progress messages
always print on their own line. Until now they have always
done so without any special intervention. But with the addition
of deferred debuginfo downloading it is possible for progress
messages to occur during the printing of frame information.
Without any newline tracking in this case, we can end up with
poorly formatted output:
(gdb) backtrace
[...]
#8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (Downloading separate debug info for /lib64/libpython3.11.so.1.0
function_cache=0x561221b224d0, state=<optimized out>...
With newline tracking, the progress message writer can detect
whether the message should start with a newline. This ensures
that all progress messages print on their own line:
(gdb) backtrace
[...]
#8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (
Downloading separate debug info for /lib64/libpython3.11.so.1.0
function_cache=0x561221b224d0, state=<optimized out>...
---
gdb/cli-out.c | 18 +++++++++++++++---
gdb/mi/mi-console.c | 4 ++++
gdb/tui/tui-file.c | 4 ++++
gdb/ui-file.c | 31 +++++++++++++++++++++++++++++++
gdb/ui-file.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index fdfd0f7f0cf..1639f885cb9 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -307,6 +307,10 @@ cli_ui_out::do_progress_notify (const std::string &msg,
if (info.state == progress_update::START)
{
+ /* MSG should print on its own line. */
+ if (stream->get_line_state () == ui_file::LINE_NOT_AT_START)
+ gdb_printf (stream, "\n");
+
if (stream->isatty ()
&& current_ui->input_interactive_p ()
&& chars_per_line >= MIN_CHARS_PER_LINE)
@@ -410,10 +414,18 @@ void
cli_ui_out::do_progress_end ()
{
struct ui_file *stream = m_streams.back ();
- m_progress_info.pop_back ();
- if (stream->isatty ())
- clear_current_line ();
+ cli_progress_info &info (m_progress_info.back ());
+
+ if (info.state != progress_update::START && stream->isatty ())
+ {
+ clear_current_line ();
+
+ if (stream->get_line_state () != ui_file::LINE_UNDEFINED)
+ stream->set_line_state (ui_file::LINE_AT_START);
+ }
+
+ m_progress_info.pop_back ();
}
/* local functions */
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index e0b5648c2df..60f2a823b47 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -46,6 +46,8 @@ mi_console_file::write (const char *buf, long length_buf)
buffer. */
if (strchr (m_buffer.c_str () + prev_size, '\n') != NULL)
this->flush ();
+
+ set_line_state_from_buf (m_buffer.c_str () + prev_size, length_buf);
}
void
@@ -63,6 +65,8 @@ mi_console_file::write_async_safe (const char *buf, long length_buf)
char nl = '\n';
m_raw->write_async_safe (&nl, 1);
+
+ set_line_state (LINE_AT_START);
}
void
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index c1619d5ace6..c36ce739497 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -28,6 +28,8 @@ tui_file::puts (const char *linebuffer)
tui_puts (linebuffer);
if (!m_buffered)
tui_refresh_cmd_win ();
+
+ set_line_state_from_buf (linebuffer);
}
void
@@ -36,6 +38,8 @@ tui_file::write (const char *buf, long length_buf)
tui_write (buf, length_buf);
if (!m_buffered)
tui_refresh_cmd_win ();
+
+ set_line_state_from_buf (buf, length_buf);
}
void
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index e0814fe2b2d..3f7a55ed252 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -31,6 +31,7 @@
null_file null_stream;
ui_file::ui_file ()
+ : m_line_state (LINE_UNDEFINED)
{}
ui_file::~ui_file ()
@@ -159,6 +160,30 @@ ui_file::printchar (int c, int quoter, bool async_safe)
this->write (buf, out);
}
+/* See ui-file.h. */
+
+void
+ui_file::set_line_state_from_buf (const char *buf)
+{
+ const char *last_nl = strrchr (buf, '\n');
+
+ if (last_nl != nullptr && *++last_nl == '\0')
+ m_line_state = LINE_AT_START;
+ else
+ m_line_state = LINE_NOT_AT_START;
+}
+
+/* See ui-file.h. */
+
+void
+ui_file::set_line_state_from_buf (const char *buf, long length_buf)
+{
+ if (buf[length_buf - 1] == '\n')
+ m_line_state = LINE_AT_START;
+ else
+ m_line_state = LINE_NOT_AT_START;
+}
+
\f
void
@@ -311,6 +336,8 @@ stdio_file::write (const char *buf, long length_buf)
{
/* Nothing. */
}
+
+ set_line_state_from_buf (buf, length_buf);
}
void
@@ -323,6 +350,8 @@ stdio_file::write_async_safe (const char *buf, long length_buf)
{
/* Nothing. */
}
+
+ set_line_state_from_buf (buf, length_buf);
}
void
@@ -339,6 +368,8 @@ stdio_file::puts (const char *linebuffer)
{
/* Nothing. */
}
+
+ set_line_state_from_buf (linebuffer);
}
bool
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index de24620e247..b85c37d81c7 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -132,11 +132,40 @@ class ui_file
this->puts (str);
}
+ enum line_state
+ {
+ /* Line state is not being tracked. */
+ LINE_UNDEFINED,
+
+ /* The last character written to this ui_file is not a newline. */
+ LINE_NOT_AT_START,
+
+ /* The last character written to this ui_file is a newline. */
+ LINE_AT_START
+ };
+
+ /* Set this ui_file's line_state to STATE. */
+ virtual void set_line_state (line_state state)
+ { m_line_state = state; }
+
+ /* Return this ui_file's line_state. */
+ virtual line_state get_line_state ()
+ { return m_line_state; }
+
protected:
/* The currently applied style. */
ui_file_style m_applied_style;
+ /* The current line_state. */
+ line_state m_line_state;
+
+ /* Set line_state based on whether BUF ends in a newline. */
+ void set_line_state_from_buf (const char *buf);
+
+ /* Set line_state based on whether BUF ends in a newline. */
+ void set_line_state_from_buf (const char *buf, long length_buf);
+
private:
/* Helper function for putstr and putstrn. Print the character C on
@@ -428,6 +457,21 @@ class wrapped_file : public ui_file
void write_async_safe (const char *buf, long length_buf) override
{ return m_stream->write_async_safe (buf, length_buf); }
+ /* Return the line_state of the underlying stream. */
+ line_state get_line_state () override
+ {
+ if (m_stream == nullptr)
+ return ui_file::LINE_UNDEFINED;
+ return m_stream->get_line_state ();
+ }
+
+ /* Set the line_state of the underlying stream. */
+ void set_line_state (line_state state) override
+ {
+ if (m_stream != nullptr)
+ m_stream->set_line_state (state);
+ }
+
protected:
/* Note that this class does not assume ownership of the stream.
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
` (2 preceding siblings ...)
2023-02-27 19:42 ` [PATCH 4/7] gdb/ui-file: Add newline tracking Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-03-07 20:20 ` Tom Tromey
2023-02-27 19:42 ` [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests Aaron Merey
` (3 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
At the beginning of a session, gdb may attempt to download debuginfo
for all shared libraries associated with the process or core file
being debugged. This can be a waste of time and storage space when much
of the debuginfo ends up not being used during the session.
To reduce the gdb's startup latency and to download only the debuginfo
that is really needed, this patch adds on-demand, a.k.a lazy, downloading.
When 'set debuginfo enabled lazy' is on, gdb will attempt to download
a .gdb_index for each shared library instead of its full debuginfo.
Each debuginfo download will be deferred until gdb needs to expand symtabs
associated with the debuginfo's index.
Because these indices are significantly smaller than their corresponding
debuginfo, this generally reduces the total amount of data gdb downloads
by a large margin. Reductions of 80%-95% have been commonly observed when
debugging large GUI programs.
(gdb) set debuginfod enabled lazy
(gdb) start
Downloading section .gdb_index for /lib64/libcurl.so.4
[...]
1826 client->server_mhandle = curl_multi_init ();
(gdb) step
Downloading separate debug info for /lib64/libcurl.so.4
Downloading separate debug info for [libcurl dwz]
curl_multi_init () at ../../lib/multi.c:457
Downloading source file /usr/src/debug/curl-7.85.0-6.fc37.x86_64/build-full/lib/../../lib/multi.c
457 {
(gdb)
Some of the key functions below include dwarf2_has_separate_index which
downloads the separate .gdb_index. If successful, it initializes a
separate debug objfile to own the index and act as a placeholder for
the full debuginfo that might be downloaded later.
read_full_dwarf_from_debuginfod downloads the full debuginfo and
reinitializes the separate debug objfile. It is called by
dwarf2_gdb_index::expand_symtabs_matching and
dwarf2_base_index_functions::find_pc_sect_compunit_symtab when symtab
expansion is required.
---
gdb/dwarf2/frame.c | 13 +++
gdb/dwarf2/frame.h | 4 +
gdb/dwarf2/index-cache.c | 33 ++++++
gdb/dwarf2/index-cache.h | 13 +++
gdb/dwarf2/public.h | 7 ++
gdb/dwarf2/read-gdb-index.c | 89 ++++++++++++----
gdb/dwarf2/read.c | 197 +++++++++++++++++++++++++++++++++++-
gdb/dwarf2/read.h | 9 ++
gdb/dwarf2/section.c | 4 +-
gdb/elfread.c | 2 +-
gdb/objfiles.c | 18 ++++
gdb/objfiles.h | 16 +++
gdb/symfile.c | 76 +++++++++++++-
gdb/symfile.h | 7 ++
gdb/symtab.h | 5 +
15 files changed, 472 insertions(+), 21 deletions(-)
diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index a561aaf3100..f6d227d8b36 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -1609,6 +1609,19 @@ set_comp_unit (struct objfile *objfile, struct comp_unit *unit)
return dwarf2_frame_bfd_data.set (abfd, unit);
}
+/* See frame.h. */
+
+void
+clear_comp_unit (struct objfile *objfile)
+{
+ bfd *abfd = objfile->obfd.get ();
+
+ if (gdb_bfd_requires_relocations (abfd))
+ dwarf2_frame_objfile_data.clear (objfile);
+ else
+ dwarf2_frame_bfd_data.clear (abfd);
+}
+
/* Find the FDE for *PC. Return a pointer to the FDE, and store the
initial location associated with it into *PC. */
diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
index 5643e557513..1851a14b67e 100644
--- a/gdb/dwarf2/frame.h
+++ b/gdb/dwarf2/frame.h
@@ -238,6 +238,10 @@ void dwarf2_append_unwinders (struct gdbarch *gdbarch);
extern const struct frame_base *
dwarf2_frame_base_sniffer (frame_info_ptr this_frame);
+/* Delete OBJFILEs comp_unit. */
+
+extern void clear_comp_unit (struct objfile * objfile);
+
/* Compute the DWARF CFA for a frame. */
CORE_ADDR dwarf2_frame_cfa (frame_info_ptr this_frame);
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 79ab706ee9d..bbafcd321b2 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -216,6 +216,33 @@ index_cache::lookup_gdb_index (const bfd_build_id *build_id,
return {};
}
+/* See index-cache.h. */
+
+gdb::array_view<const gdb_byte>
+index_cache::lookup_gdb_index_debuginfod (const char *index_path,
+ std::unique_ptr<index_cache_resource> *resource)
+{
+ try
+ {
+ /* Try to map that file. */
+ index_cache_resource_mmap *mmap_resource
+ = new index_cache_resource_mmap (index_path);
+
+ /* Hand the resource to the caller. */
+ resource->reset (mmap_resource);
+
+ return gdb::array_view<const gdb_byte>
+ ((const gdb_byte *) mmap_resource->mapping.get (),
+ mmap_resource->mapping.size ());
+ }
+ catch (const gdb_exception_error &except)
+ {
+ warning (_("Unable to read %s: %s"), index_path, except.what ());
+ }
+
+ return {};
+}
+
#else /* !HAVE_SYS_MMAN_H */
/* See dwarf-index-cache.h. This is a no-op on unsupported systems. */
@@ -227,6 +254,12 @@ index_cache::lookup_gdb_index (const bfd_build_id *build_id,
return {};
}
+gdb::array_view<const gdb_byte>
+index_cache::lookup_gdb_index_debuginfod (const char *index_path,
+ std::unique_ptr<index_cache_resource> *resource)
+{
+ return {};
+}
#endif
/* See dwarf-index-cache.h. */
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 1efff17049f..e400afd5123 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -67,6 +67,19 @@ class index_cache
lookup_gdb_index (const bfd_build_id *build_id,
std::unique_ptr<index_cache_resource> *resource);
+ /* Look for an index file located at INDEX_PATH in the debuginfod cache.
+ Unlike lookup_gdb_index, this function does not exit early if the
+ index cache has not been enabled.
+
+ If found, return the contents as an array_view and store the underlying
+ resources (allocated memory, mapped file, etc) in RESOURCE. The returned
+ array_view is valid as long as RESOURCE is not destroyed.
+
+ If no matching index file is found, return an empty array view. */
+ gdb::array_view<const gdb_byte>
+ lookup_gdb_index_debuginfod (const char *index_path,
+ std::unique_ptr<index_cache_resource> *resource);
+
/* Return the number of cache hits. */
unsigned int n_hits () const
{ return m_n_hits; }
diff --git a/gdb/dwarf2/public.h b/gdb/dwarf2/public.h
index 0e74857eb1a..4a44cdbc223 100644
--- a/gdb/dwarf2/public.h
+++ b/gdb/dwarf2/public.h
@@ -40,4 +40,11 @@ extern void dwarf2_initialize_objfile (struct objfile *objfile);
extern void dwarf2_build_frame_info (struct objfile *);
+/* Query debuginfod for the .gdb_index associated with OBJFILE. If
+ successful, create an objfile to hold the .gdb_index information
+ and act as a placeholder until the full debuginfo needs to be
+ downloaded. */
+
+extern bool dwarf2_has_separate_index (struct objfile *);
+
#endif /* DWARF2_PUBLIC_H */
diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index 1006386cb2d..f066d55bd1a 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -143,6 +143,9 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
int global,
symbol_compare_ftype *ordered_compare) override;
+ /* Calls do_expand_symtabs_matching. If this index was initialized
+ from a file downloaded from debuginfod and there is a match, attempt
+ to download debuginfo. */
bool expand_symtabs_matching
(struct objfile *objfile,
gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
@@ -152,6 +155,16 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
block_search_flags search_flags,
domain_enum domain,
enum search_domain kind) override;
+
+ bool do_expand_symtabs_matching
+ (struct objfile *objfile,
+ gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
+ const lookup_name_info *lookup_name,
+ gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
+ gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
+ block_search_flags search_flags,
+ domain_enum domain,
+ enum search_domain kind);
};
/* This dumps minimal information about the index.
@@ -455,7 +468,7 @@ dw2_expand_marked_cus
}
bool
-dwarf2_gdb_index::expand_symtabs_matching
+dwarf2_gdb_index::do_expand_symtabs_matching
(struct objfile *objfile,
gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
const lookup_name_info *lookup_name,
@@ -504,6 +517,44 @@ dwarf2_gdb_index::expand_symtabs_matching
return result;
}
+bool
+dwarf2_gdb_index::expand_symtabs_matching
+ (struct objfile *objfile,
+ gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
+ const lookup_name_info *lookup_name,
+ gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
+ gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
+ block_search_flags search_flags,
+ domain_enum domain,
+ enum search_domain kind)
+{
+ if (objfile->flags & OBJF_READNEVER)
+ return false;
+
+ try
+ {
+ return do_expand_symtabs_matching (objfile, file_matcher, lookup_name,
+ symbol_matcher, expansion_notify,
+ search_flags, domain, kind);
+ }
+ catch (gdb_exception e)
+ {
+ if (!objfile->is_separate_index_stub ())
+ {
+ exception_print (gdb_stderr, e);
+ return false;
+ }
+
+ /* Objfile is a stub holding only index information. Try to reinitialize
+ objfile with the full debuginfo. */
+ if (!read_full_dwarf_from_debuginfod (objfile))
+ return false;
+ return do_expand_symtabs_matching (objfile, file_matcher, lookup_name,
+ symbol_matcher, expansion_notify,
+ search_flags, domain, kind);
+ }
+}
+
quick_symbol_functions_up
mapped_gdb_index::make_quick_functions () const
{
@@ -797,28 +848,32 @@ dwarf2_read_gdb_index
/* If there is a .dwz file, read it so we can get its CU list as
well. */
- dwz = dwarf2_get_dwz_file (per_bfd);
- if (dwz != NULL)
+ if (get_gdb_index_contents_dwz != nullptr)
{
mapped_gdb_index dwz_map;
const gdb_byte *dwz_types_ignore;
offset_type dwz_types_elements_ignore;
+ dwz = dwarf2_get_dwz_file (per_bfd);
- gdb::array_view<const gdb_byte> dwz_index_content
- = get_gdb_index_contents_dwz (objfile, dwz);
-
- if (dwz_index_content.empty ())
- return 0;
-
- if (!read_gdb_index_from_buffer (bfd_get_filename (dwz->dwz_bfd.get ()),
- 1, dwz_index_content, &dwz_map,
- &dwz_list, &dwz_list_elements,
- &dwz_types_ignore,
- &dwz_types_elements_ignore))
+ if (dwz != nullptr)
{
- warning (_("could not read '.gdb_index' section from %s; skipping"),
- bfd_get_filename (dwz->dwz_bfd.get ()));
- return 0;
+ gdb::array_view<const gdb_byte> dwz_index_content
+ = get_gdb_index_contents_dwz (objfile, dwz);
+
+ if (dwz_index_content.empty ())
+ return 0;
+
+ if (!read_gdb_index_from_buffer (bfd_get_filename
+ (dwz->dwz_bfd.get ()),
+ 1, dwz_index_content, &dwz_map,
+ &dwz_list, &dwz_list_elements,
+ &dwz_types_ignore,
+ &dwz_types_elements_ignore))
+ {
+ warning (_("could not read '.gdb_index' section from %s; skipping"),
+ bfd_get_filename (dwz->dwz_bfd.get ()));
+ return 0;
+ }
}
}
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 69b2310be1f..12b0dbd1ee8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -34,6 +34,7 @@
#include "dwarf2/attribute.h"
#include "dwarf2/comp-unit-head.h"
#include "dwarf2/cu.h"
+#include "dwarf2/frame.h"
#include "dwarf2/index-cache.h"
#include "dwarf2/index-common.h"
#include "dwarf2/leb.h"
@@ -95,6 +96,8 @@
#include "split-name.h"
#include "gdbsupport/parallel-for.h"
#include "gdbsupport/thread-pool.h"
+#include "inferior.h"
+#include "debuginfod-support.h"
/* When == 1, print basic high level tracing messages.
When > 1, be more verbose.
@@ -3156,7 +3159,7 @@ dwarf2_base_index_functions::find_per_cu (dwarf2_per_bfd *per_bfd,
}
struct compunit_symtab *
-dwarf2_base_index_functions::find_pc_sect_compunit_symtab
+dwarf2_base_index_functions::do_find_pc_sect_compunit_symtab
(struct objfile *objfile,
struct bound_minimal_symbol msymbol,
CORE_ADDR pc,
@@ -3187,6 +3190,39 @@ dwarf2_base_index_functions::find_pc_sect_compunit_symtab
return result;
}
+struct compunit_symtab *
+dwarf2_base_index_functions::find_pc_sect_compunit_symtab
+ (struct objfile *objfile,
+ struct bound_minimal_symbol msymbol,
+ CORE_ADDR pc,
+ struct obj_section *section,
+ int warn_if_readin)
+{
+ if (objfile->flags & OBJF_READNEVER)
+ return nullptr;
+
+ try
+ {
+ return do_find_pc_sect_compunit_symtab (objfile, msymbol, pc,
+ section, warn_if_readin);
+ }
+ catch (gdb_exception e)
+ {
+ if (!objfile->is_separate_index_stub ())
+ {
+ exception_print (gdb_stderr, e);
+ return nullptr;
+ }
+
+ /* Objfile is a stub holding only index information. Try to
+ reinitialize objfile with the full debuginfo. */
+ if (!read_full_dwarf_from_debuginfod (objfile))
+ return nullptr;
+ return do_find_pc_sect_compunit_symtab (objfile, msymbol, pc,
+ section, warn_if_readin);
+ }
+}
+
void
dwarf2_base_index_functions::map_symbol_filenames
(struct objfile *objfile,
@@ -3412,6 +3448,165 @@ dwarf2_initialize_objfile (struct objfile *objfile)
objfile->qf.push_front (make_cooked_index_funcs ());
}
+/* Query debuginfod for the .gdb_index matching OBJFILE's build-id. Return the
+ contents if successful. */
+
+static gdb::array_view<const gdb_byte>
+get_gdb_index_contents_from_debuginfod (objfile *objfile, dwarf2_per_bfd *per_bfd)
+{
+ const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
+ if (build_id == nullptr)
+ return {};
+
+ gdb::unique_xmalloc_ptr<char> index_path;
+ scoped_fd fd = debuginfod_section_query (build_id->data, build_id->size,
+ bfd_get_filename
+ (objfile->obfd.get ()),
+ ".gdb_index",
+ &index_path);
+ if (fd.get () < 0)
+ return {};
+
+ return global_index_cache.lookup_gdb_index_debuginfod
+ (index_path.get (), &per_bfd->index_cache_res);
+}
+
+/* See read.h. */
+
+bool
+read_full_dwarf_from_debuginfod (struct objfile *objfile)
+{
+ gdb_assert (objfile->is_separate_index_stub ());
+
+ const struct bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
+ if (build_id == nullptr)
+ return false;
+
+ const char *filename = bfd_get_filename (objfile->obfd.get ());
+ gdb_bfd_ref_ptr debug_bfd;
+ gdb::unique_xmalloc_ptr<char> symfile_path;
+
+ scoped_fd fd (debuginfod_debuginfo_query (build_id->data,
+ build_id->size,
+ filename,
+ &symfile_path));
+
+ if (fd.get () < 0)
+ return false;
+
+ /* File successfully retrieved from server. */
+ debug_bfd = symfile_bfd_open (symfile_path.get ());
+ if (debug_bfd == nullptr
+ || !build_id_verify (debug_bfd.get (), build_id->size, build_id->data))
+ {
+ warning (_("File \"%s\" from debuginfod cannot be opened as bfd"),
+ filename);
+ return false;
+ }
+
+ /* Fill in objfile's missing information using the debuginfo. This
+ may also download dwz, if available. */
+ if (!objfile->reinit (debug_bfd.release ()))
+ return false;
+
+ struct objfile *parent = objfile->separate_debug_objfile_backlink;
+ gdb_assert (parent != nullptr);
+
+ /* Clear the parent objfile's frame comp_unit information so it can be
+ recalculated with DWARF. */
+ clear_comp_unit (parent);
+
+ return true;
+}
+
+/* See public.h. */
+
+bool
+dwarf2_has_separate_index (struct objfile *objfile)
+{
+ if (objfile->flags & OBJF_MAINLINE
+ || objfile->separate_debug_objfile_backlink != nullptr)
+ return 0;
+ if (objfile->is_separate_index_stub ())
+ return 1;
+ if (!IS_DIR_SEPARATOR (*objfile_filename (objfile)))
+ return 0;
+
+ gdb::unique_xmalloc_ptr<char> index_path;
+ const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
+ scoped_fd fd = debuginfod_section_query (build_id->data,
+ build_id->size,
+ bfd_get_filename
+ (objfile->obfd.get ()),
+ ".gdb_index",
+ &index_path);
+ if (fd.get () >= 0)
+ {
+ /* We found a separate .gdb_index file so a separate debuginfo file
+ should exist, but don't want to read it until we really have to.
+ Create an objfile to own the index information and act as a
+ placeholder for the debuginfo that we have the option of aquiring
+ later. */
+ gdb_bfd_ref_ptr abfd (gdb_bfd_open (objfile_filename (objfile), gnutarget));
+ if (abfd == nullptr)
+ return false;
+
+ dwarf2_per_bfd_objfile_data_key.clear (objfile);
+ dwarf2_objfile_data_key.clear (objfile);
+
+ /* Initialize the new objfile using a bfd opened from the parent's
+ filename. This objfile will be reinitialized if/when we download
+ the full debuginfo. */
+ symbol_file_add_from_index
+ (abfd, current_inferior ()->symfile_flags | SYMFILE_NO_READ, objfile);
+
+ dwarf2_per_bfd *per_bfd;
+ dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
+
+ /* Perform a limited initialization based to dwarf2_has_info and
+ dwarf2_initialize_objfile. */
+ if (per_objfile == nullptr)
+ {
+ per_bfd = dwarf2_per_bfd_objfile_data_key.get (objfile);
+ if (per_bfd == nullptr)
+ {
+ per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), nullptr, false);
+ dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
+ }
+ per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
+ }
+
+ struct objfile *stub = objfile->separate_debug_objfile;
+ per_objfile = get_dwarf2_per_objfile (stub);
+ if (per_objfile == nullptr)
+ {
+ per_bfd = dwarf2_per_bfd_objfile_data_key.get (stub);
+ if (per_bfd == nullptr)
+ {
+ per_bfd = new dwarf2_per_bfd (stub->obfd.get (), nullptr, false);
+ dwarf2_per_bfd_objfile_data_key.set (stub, per_bfd);
+ }
+ per_objfile = dwarf2_objfile_data_key.emplace (stub, stub, per_bfd);
+ }
+
+ /* Do not include a dwz index callback, otherwise we could download a dwz
+ file. We don't want to do this until the full debuginfo is needed. */
+ if (dwarf2_read_gdb_index (per_objfile,
+ get_gdb_index_contents_from_debuginfod,
+ nullptr))
+ {
+ dwarf_read_debug_printf ("found .gdb_index from debuginfod");
+ stub->qf.push_front (per_bfd->index_table->make_quick_functions ());
+ return 1;
+ }
+
+ /* Unable to read the index. */
+ stub->unlink ();
+ }
+
+ return 0;
+}
+
\f
/* Build a partial symbol table. */
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 0e6853ea2f4..4e6c35cba5e 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -866,6 +866,10 @@ struct dwarf2_base_index_functions : public quick_symbol_functions
CORE_ADDR pc, struct obj_section *section, int warn_if_readin)
override final;
+ struct compunit_symtab *do_find_pc_sect_compunit_symtab
+ (struct objfile *objfile, struct bound_minimal_symbol msymbol,
+ CORE_ADDR pc, struct obj_section *section, int warn_if_readin);
+
struct compunit_symtab *find_compunit_symtab_by_address
(struct objfile *objfile, CORE_ADDR address) override
{
@@ -942,4 +946,9 @@ extern bool read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
dwarf2_section_info *section,
addrmap *mutable_map);
+/* If OBJFILE is a stub holding only information from a .gdb_index, then attempt
+ to download the full debuginfo and reinitialize OBJFILE with it. */
+
+extern bool read_full_dwarf_from_debuginfod (struct objfile *objfile);
+
#endif /* DWARF2READ_H */
diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
index c9ef41893ee..ff8ef527c29 100644
--- a/gdb/dwarf2/section.c
+++ b/gdb/dwarf2/section.c
@@ -54,7 +54,9 @@ dwarf2_section_info::get_bfd_owner () const
section = get_containing_section ();
gdb_assert (!section->is_virtual);
}
- gdb_assert (section->s.section != nullptr);
+ if (section->s.section == nullptr)
+ throw_error (NOT_FOUND_ERROR,
+ _("Can't find owner of DWARF section."));
return section->s.section->owner;
}
diff --git a/gdb/elfread.c b/gdb/elfread.c
index ca684aab57e..6c3e384eb8d 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1229,7 +1229,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
symbol_file_add_separate (debug_bfd, debugfile.c_str (),
symfile_flags, objfile);
}
- else
+ else if (!dwarf2_has_separate_index (objfile))
{
has_dwarf2 = false;
const struct bfd_build_id *build_id
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index ed29131d528..fa8ab4a02b6 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -52,6 +52,7 @@
#include "gdb_bfd.h"
#include "btrace.h"
#include "gdbsupport/pathstuff.h"
+#include "symfile.h"
#include <algorithm>
#include <vector>
@@ -298,6 +299,23 @@ build_objfile_section_table (struct objfile *objfile)
objfile, 1);
}
+bool
+objfile::reinit (struct bfd *abfd)
+{
+ try
+ {
+ deferred_read_symbols (this, abfd);
+ return true;
+ }
+ catch (const gdb_exception &except)
+ {
+ exception_print (gdb_stderr, except);
+ this->flags |= OBJF_READNEVER;
+ }
+
+ return false;
+}
+
/* Given a pointer to an initialized bfd (ABFD) and some flag bits,
initialize the new objfile as best we can and link it into the list
of all known objfiles.
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 342aa09ac6a..55c9ad1f987 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -375,6 +375,8 @@ class separate_debug_iterator
struct objfile *m_parent;
};
+#define SEPARATE_INDEX_FILENAME "<separate index stub>"
+
/* A range adapter wrapping separate_debug_iterator. */
typedef iterator_range<separate_debug_iterator> separate_debug_range;
@@ -497,6 +499,12 @@ struct objfile
/* See quick_symbol_functions. */
struct symtab *find_last_source_symtab ();
+ /* Reinitialize this objfile using ABFD. Return true if successful.
+ Objfile should have been originally initialized using a separate
+ index from ABFD. Updates this objfile with ABFD's symbols and
+ section information. */
+ bool reinit (struct bfd *abfd);
+
/* See quick_symbol_functions. */
void forget_cached_source_info ();
@@ -609,6 +617,14 @@ struct objfile
this->section_offsets[idx] = offset;
}
+ /* Return true if this objfile holds .gdb_index data and represents
+ a debuginfo file whose download has been deferred. */
+
+ bool is_separate_index_stub ()
+ {
+ return strcmp (original_name, SEPARATE_INDEX_FILENAME) == 0;
+ }
+
private:
/* Ensure that partial symbols have been read and return the "quick" (aka
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 373f5592107..1e05dd6af6e 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1035,7 +1035,8 @@ static struct objfile *
symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
symfile_add_flags add_flags,
section_addr_info *addrs,
- objfile_flags flags, struct objfile *parent)
+ objfile_flags flags, struct objfile *parent,
+ bool skip_syms = false)
{
struct objfile *objfile;
const int from_tty = add_flags & SYMFILE_VERBOSE;
@@ -1073,6 +1074,15 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
flags |= OBJF_MAINLINE;
objfile = objfile::make (abfd, name, flags, parent);
+ if (skip_syms)
+ {
+ /* objfile was initialized only using a separate index so don't
+ try to read symbols yet. */
+ objfile->section_offsets = parent->section_offsets;
+ gdb::observers::new_objfile.notify (objfile);
+ return objfile;
+ }
+
/* We either created a new mapped symbol table, mapped an existing
symbol table file which has not had initial symbol reading
performed, or need to read an unmapped symbol table. */
@@ -1134,6 +1144,27 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
return (objfile);
}
+/* See symfile.h. */
+
+void
+symbol_file_add_from_index (const gdb_bfd_ref_ptr &bfd,
+ symfile_add_flags symfile_flags,
+ struct objfile *parent)
+{
+ section_addr_info sap = build_section_addr_info_from_objfile (parent);
+
+ symbol_file_add_with_addrs
+ (bfd, SEPARATE_INDEX_FILENAME, symfile_flags, &sap,
+ (parent->flags & (OBJF_REORDERED | OBJF_SHARED | OBJF_READNOW
+ | OBJF_USERLOADED | OBJF_MAINLINE | OBJF_PSYMTABS_READ))
+ | OBJF_NOT_FILENAME, parent, true);
+
+ objfile *result = parent->separate_debug_objfile;
+ init_objfile_sect_indices (result);
+
+ return;
+}
+
/* Add BFD as a separate debug file for OBJFILE. For NAME description
see the objfile constructor. */
@@ -2440,6 +2471,49 @@ remove_symbol_file_command (const char *args, int from_tty)
clear_symtab_users (0);
}
+/* See symtab.h. */
+
+void
+deferred_read_symbols (struct objfile *objfile, struct bfd *abfd)
+{
+ const char *obfd_filename;
+
+ obfd_filename = bfd_get_filename (abfd);
+ gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget));
+
+ if (temp == nullptr)
+ error (_("Can't open %s to read symbols."), obfd_filename);
+
+ if (!bfd_check_format (temp.get (), bfd_object))
+ error (_("Can't read symbols from %s: %s."), obfd_filename,
+ bfd_errmsg (bfd_get_error ()));
+
+ objfile->obfd = std::move (temp);
+
+ /* Nuke all the state that we will re-read. */
+ objfile->registry_fields.clear_registry ();
+ objfile->sections = NULL;
+ objfile->sect_index_bss = -1;
+ objfile->sect_index_data = -1;
+ objfile->sect_index_rodata = -1;
+ objfile->sect_index_text = -1;
+ objfile->compunit_symtabs = NULL;
+ objfile->template_symbols = NULL;
+
+ objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd.get ()));
+ build_objfile_section_table (objfile);
+ (*objfile->sf->sym_init) (objfile);
+ init_objfile_sect_indices (objfile);
+
+ objfile->section_offsets.resize (gdb_bfd_count_sections
+ (objfile->obfd.get ()));
+ read_symbols (objfile, 0);
+
+ objfile->mtime = bfd_get_mtime (objfile->obfd.get ());
+ objfile->original_name
+ = obstack_strdup (&objfile->objfile_obstack, obfd_filename);
+}
+
/* Re-read symbols if a symbol-file has changed. */
void
diff --git a/gdb/symfile.h b/gdb/symfile.h
index b433e2be31a..353cf0cab42 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -241,6 +241,13 @@ extern struct objfile *symbol_file_add_from_bfd (const gdb_bfd_ref_ptr &,
extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
symfile_add_flags, struct objfile *);
+/* Initialize an objfile acting as a placeholder for separate debuginfo
+ that has not yet been opened or read. Infer some of its properties
+ from PARENT. */
+
+extern void symbol_file_add_from_index (const gdb_bfd_ref_ptr &,
+ symfile_add_flags, struct objfile *);
+
/* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
Returns pathname, or an empty string.
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 17d2746fd48..304482f81dd 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2206,6 +2206,11 @@ extern bool find_pc_line_pc_range (CORE_ADDR, CORE_ADDR *, CORE_ADDR *);
extern void reread_symbols (int from_tty);
+/* Reread a separate debug OBJFILE that was originally initialized
+ using an index. ABFD will be used as OBJFILE's new bfd. */
+
+extern void deferred_read_symbols (struct objfile *objfile, struct bfd *abfd);
+
/* Look up a type named NAME in STRUCT_DOMAIN in the current language.
The type returned must not be opaque -- i.e., must have at least one field
defined. */
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
` (3 preceding siblings ...)
2023-02-27 19:42 ` [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-05-02 15:48 ` Andrew Burgess
2023-05-24 10:12 ` Andrew Burgess
2023-02-27 19:42 ` [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading Aaron Merey
` (2 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
Add some tests for 'set debuginfod enabled lazy', .gdb_index downloading
and deferred debuginfo downloading.
---
.../gdb.debuginfod/fetch_src_and_symbols.exp | 58 +++++++++++++++++++
gdb/testsuite/gdb.debuginfod/libsection.c | 24 ++++++++
gdb/testsuite/gdb.debuginfod/section.c | 28 +++++++++
3 files changed, 110 insertions(+)
create mode 100644 gdb/testsuite/gdb.debuginfod/libsection.c
create mode 100644 gdb/testsuite/gdb.debuginfod/section.c
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 8158c5c3cc6..fc7e4b685dc 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -42,6 +42,23 @@ if { [gdb_compile "$sourcetmp" "${binfile}2" executable {debug build-id}] != ""
return -1
}
+set sectfile "section"
+set sectsrc $srcdir/$subdir/section.c
+set sectexec [standard_output_file $sectfile]
+
+set libfile "libsection"
+set libsrc $srcdir/$subdir/$libfile.c
+set lib_sl [standard_output_file $libfile.sl]
+
+set lib_opts [list debug build-id ]
+set exec_opts [list debug build-id shlib=$lib_sl]
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+ || [gdb_compile $sectsrc $sectexec executable $exec_opts] != "" } {
+ untested "failed to compile"
+ return -1
+}
+
# Write some assembly that just has a .gnu_debugaltlink section.
# Copied from testsuite/gdb.dwarf2/dwzbuildid.exp.
proc write_just_debugaltlink {filename dwzname buildid} {
@@ -94,6 +111,7 @@ proc write_dwarf_file {filename buildid {value 99}} {
}
set corefile [standard_output_file "corefile"]
+set lazy_support -1
# Setup the global variable DEBUGDIR as a directory containing the
# debug information for the test executable.
@@ -103,6 +121,8 @@ set corefile [standard_output_file "corefile"]
# running.
proc_with_prefix no_url { } {
global binfile outputdir debugdir
+ global sectexec lib_sl libfile lazy_support
+ global gdb_prompt
setenv DEBUGINFOD_URLS ""
@@ -119,11 +139,18 @@ proc_with_prefix no_url { } {
return -1
}
+ if { [gdb_gnu_strip_debug $lib_sl ""] != 0} {
+ fail "strip shlib debuginfo"
+ return -1
+ }
+
set debugdir [standard_output_file "debug"]
set debuginfo [standard_output_file "fetch_src_and_symbols.debug"]
+ set debuginfo_shlib [standard_output_file $libfile.sl.debug]
file mkdir $debugdir
file rename -force $debuginfo $debugdir
+ file rename -force $debuginfo_shlib $debugdir
# Test that GDB cannot find symbols without debuginfod.
clean_restart $binfile
@@ -171,6 +198,25 @@ proc_with_prefix no_url { } {
clean_restart
gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
+ clean_restart
+
+ # Check for lazy downloading support.
+ gdb_test_multiple "set debuginfod enabled lazy" "check for lazy" {
+ -re ".*lazy downloading is not compiled into GDB.*\n.*${gdb_prompt} $" {
+ set lazy_support 0
+ }
+ -re ".*${gdb_prompt} $" {
+ set lazy_support 1
+ }
+ }
+
+ if {$lazy_support == 1} {
+ gdb_test "file $sectexec" "" "file [file tail $sectexec] file no url"
+ gdb_test "start" "" "file [file tail $sectexec] start no url"
+ gdb_test "info sharedlibrary" ".*Yes \\(\\*\\).*libsection.*" "lib no debug"
+ } else {
+ untested "lazy support no_url"
+ }
}
# Test that GDB prints the debuginfod URLs when loading files. URLS
@@ -208,6 +254,7 @@ proc disable_delete_breakpoints {} {
# expected debug information.
proc_with_prefix local_url { } {
global binfile outputdir debugdir db
+ global sectexec lib_sl libfile lazy_support
set url [start_debuginfod $db $debugdir]
if { $url == "" } {
@@ -256,6 +303,17 @@ proc_with_prefix local_url { } {
"file [file tail ${binfile}_alt.o]" \
$enable_debuginfod_question "y"
+ if {$lazy_support == 1} {
+ # GDB should now download .gdb_index.
+ clean_restart
+ gdb_test "set debuginfod enabled lazy" "" "set lazy urls"
+ gdb_test "file $sectexec" "" "file [file tail $sectexec] file urls"
+ set res ".*Download.*\.gdb_index.*libsection.*\r\nDownload.*debug info.*libsection.*"
+ gdb_test "start $sectexec" $res "file [file tail $sectexec] start urls"
+ } else {
+ untested "lazy support urls"
+ }
+
# Configure debuginfod with commands.
unsetenv DEBUGINFOD_URLS
clean_restart
diff --git a/gdb/testsuite/gdb.debuginfod/libsection.c b/gdb/testsuite/gdb.debuginfod/libsection.c
new file mode 100644
index 00000000000..510f9205282
--- /dev/null
+++ b/gdb/testsuite/gdb.debuginfod/libsection.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2020-2023 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+
+void
+libsection_test ()
+{
+ printf ("In libsection\n");
+}
diff --git a/gdb/testsuite/gdb.debuginfod/section.c b/gdb/testsuite/gdb.debuginfod/section.c
new file mode 100644
index 00000000000..3598896c1bc
--- /dev/null
+++ b/gdb/testsuite/gdb.debuginfod/section.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2020-2023 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <fcntl.h>
+
+extern void libsection_test ();
+
+int
+main()
+{
+ (void) open ("", 0);
+ libsection_test ();
+ return 0;
+}
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
` (4 preceding siblings ...)
2023-02-27 19:42 ` [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests Aaron Merey
@ 2023-02-27 19:42 ` Aaron Merey
2023-03-07 20:36 ` Tom Tromey
2023-02-28 11:11 ` [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Alexandra Petlanova Hajkova
2023-05-24 9:01 ` Andrew Burgess
7 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2023-02-27 19:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Aaron Merey
'set debuginfod enabled lazy' allows gdb to download .gdb_index files in
order to defer full debuginfo downloads. However .gdb_index does not
contain any information regarding source filenames. When a gdb command
includes a filename argument (ex. 'break main.c:50'), this results in
the mass downloading of all deferred debuginfo so gdb can search the
debuginfo for matching source filenames.
To improve this, have gdb instead download each debuginfo's .debug_line
(and .debug_line_str if using DWARF5) when executing these commands.
Download full debuginfo only when its .debug_line contains a matching
filename.
Since the combined size of .debug_line and .debug_line_str is only about
1% the size of the corresponding debuginfo, significant time is saved
by checking these sections before choosing to download a deferred debuginfo.
This patch adds functions read_formatted_entries_separate and
dwarf_decode_line_header_separate. They are similar to
read_formatted_entries and dwarf_decode_line_header except that they are
able to work with .debug_line sections originating from separately
downloaded files.
dwarf2_gdb_index::expand_symtabs_matching initiates the downloading and
checking of an index's associated .debug_line when there is a filename
argument that must be matched. The .debug_line information is managed
by the new structs line_resource_mmap and mapped_debug_line.
---
| 332 ++++++++++++++++++++++++++++++++++++
| 10 ++
gdb/dwarf2/read-gdb-index.c | 26 +++
gdb/dwarf2/read.c | 164 ++++++++++++++++++
gdb/dwarf2/read.h | 31 ++++
5 files changed, 563 insertions(+)
--git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 9d74c8fe75b..7c51fb5bb12 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -113,6 +113,156 @@ read_checked_initial_length_and_offset (bfd *abfd, const gdb_byte *buf,
return length;
}
+
+/* Like read_formatted_entries but the .debug_line and .debug_line_str
+ are stored in LINE_BUFP and LINE_STR_DATA. This is used for cases
+ where these sections are read from separate files without necessarily
+ having access to the entire debuginfo file they originate from. */
+
+static void
+read_formatted_entries_separate
+ (bfd *parent_bfd, const gdb_byte **line_bufp,
+ const gdb::array_view<const gdb_byte> line_str_data,
+ struct line_header *lh,
+ unsigned int offset_size,
+ void (*callback) (struct line_header *lh,
+ const char *name,
+ dir_index d_index,
+ unsigned int mod_time,
+ unsigned int length))
+{
+ gdb_byte format_count, formati;
+ ULONGEST data_count, datai;
+ const gdb_byte *buf = *line_bufp;
+ const gdb_byte *str_buf = line_str_data.data ();
+ const gdb_byte *format_header_data;
+ unsigned int bytes_read;
+
+ format_count = read_1_byte (parent_bfd, buf);
+ buf += 1;
+ format_header_data = buf;
+ for (formati = 0; formati < format_count; formati++)
+ {
+ read_unsigned_leb128 (parent_bfd, buf, &bytes_read);
+ buf += bytes_read;
+ read_unsigned_leb128 (parent_bfd, buf, &bytes_read);
+ buf += bytes_read;
+ }
+
+ data_count = read_unsigned_leb128 (parent_bfd, buf, &bytes_read);
+ buf += bytes_read;
+ for (datai = 0; datai < data_count; datai++)
+ {
+ const gdb_byte *format = format_header_data;
+ struct file_entry fe;
+
+ for (formati = 0; formati < format_count; formati++)
+ {
+ ULONGEST content_type = read_unsigned_leb128 (parent_bfd, format, &bytes_read);
+ format += bytes_read;
+
+ ULONGEST form = read_unsigned_leb128 (parent_bfd, format, &bytes_read);
+ format += bytes_read;
+
+ gdb::optional<const char *> string;
+ gdb::optional<unsigned int> uint;
+
+ switch (form)
+ {
+ case DW_FORM_string:
+ string.emplace (read_direct_string (parent_bfd, buf, &bytes_read));
+ buf += bytes_read;
+ break;
+
+ case DW_FORM_line_strp:
+ {
+ if (line_str_data.empty ())
+ error (_("Dwarf Error: DW_FORM_line_strp used without " \
+ "required section"));
+ if (line_str_data.size () <= offset_size)
+ error (_("Dwarf Error: DW_FORM_line_strp pointing outside " \
+ "of section .debug_line"));
+
+ ULONGEST str_offset = read_offset (parent_bfd, buf, offset_size);
+
+ const char *str;
+ if (str_buf[str_offset] == '\0')
+ str = nullptr;
+ else
+ str = (const char *) (str_buf + str_offset);
+ string.emplace (str);
+ buf += offset_size;
+ break;
+ }
+
+ case DW_FORM_data1:
+ uint.emplace (read_1_byte (parent_bfd, buf));
+ buf += 1;
+ break;
+
+ case DW_FORM_data2:
+ uint.emplace (read_2_bytes (parent_bfd, buf));
+ buf += 2;
+ break;
+
+ case DW_FORM_data4:
+ uint.emplace (read_4_bytes (parent_bfd, buf));
+ buf += 4;
+ break;
+
+ case DW_FORM_data8:
+ uint.emplace (read_8_bytes (parent_bfd, buf));
+ buf += 8;
+ break;
+
+ case DW_FORM_data16:
+ /* This is used for MD5, but file_entry does not record MD5s. */
+ buf += 16;
+ break;
+
+ case DW_FORM_udata:
+ uint.emplace (read_unsigned_leb128 (parent_bfd, buf, &bytes_read));
+ buf += bytes_read;
+ break;
+
+ case DW_FORM_block:
+ /* It is valid only for DW_LNCT_timestamp which is ignored by
+ current GDB. */
+ break;
+ }
+
+ switch (content_type)
+ {
+ case DW_LNCT_path:
+ if (string.has_value ())
+ fe.name = *string;
+ break;
+ case DW_LNCT_directory_index:
+ if (uint.has_value ())
+ fe.d_index = (dir_index) *uint;
+ break;
+ case DW_LNCT_timestamp:
+ if (uint.has_value ())
+ fe.mod_time = *uint;
+ break;
+ case DW_LNCT_size:
+ if (uint.has_value ())
+ fe.length = *uint;
+ break;
+ case DW_LNCT_MD5:
+ break;
+ default:
+ complaint (_("Unknown format content type %s"),
+ pulongest (content_type));
+ }
+ }
+
+ callback (lh, fe.name, fe.d_index, fe.mod_time, fe.length);
+ }
+
+ *line_bufp = buf;
+}
+
/* Read directory or file name entry format, starting with byte of
format count entries, ULEB128 pairs of entry formats, ULEB128 of
entries count and the entries themselves in the described entry
@@ -247,6 +397,188 @@ read_formatted_entries (dwarf2_per_objfile *per_objfile, bfd *abfd,
*bufp = buf;
}
+/* See line-header.h. */
+
+line_header_up
+dwarf_decode_line_header_separate (bfd *parent_bfd,
+ gdb::array_view<const gdb_byte>
+ line_data,
+ gdb::array_view<const gdb_byte>
+ line_str_data,
+ const gdb_byte **debug_line_ptr)
+{
+ const gdb_byte *line_ptr, *buf;
+ unsigned int bytes_read, offset_size;
+ int i;
+ const char *cur_dir, *cur_file;
+
+ buf = *debug_line_ptr;
+
+ /* Make sure that at least there's room for the total_length field.
+ That could be 12 bytes long, but we're just going to fudge that. */
+ if (buf + 4 >= line_data.data () + line_data.size ())
+ {
+ dwarf2_statement_list_fits_in_line_number_section_complaint (); // need to print
+ return 0;
+ }
+
+ /* We don't have access to the CU DIE yet so we don't know the comp_dir. */
+ line_header_up lh (new line_header (nullptr));
+
+ lh->sect_off = (sect_offset) (buf - line_data.data ());
+ lh->offset_in_dwz = false;
+
+ line_ptr = buf;
+
+ /* Read in the header. */
+
+ LONGEST unit_length = read_initial_length (parent_bfd, buf, &bytes_read);
+ offset_size = (bytes_read == 4) ? 4 : 8;
+
+ line_ptr += bytes_read;
+
+ if (line_ptr + unit_length > buf + line_data.size ())
+ {
+ dwarf2_statement_list_fits_in_line_number_section_complaint ();
+ return 0;
+ }
+
+ const gdb_byte *start_here = line_ptr;
+
+ lh->statement_program_end = start_here + unit_length;
+ lh->version = read_2_bytes (parent_bfd, line_ptr);
+ line_ptr += 2;
+ if (lh->version > 5)
+ {
+ /* This is a version we don't understand. The format could have
+ changed in ways we don't handle properly so just punt. */
+ complaint (_("unsupported version in .debug_line section"));
+ return nullptr;
+ }
+ if (lh->version >= 5)
+ {
+ gdb_byte segment_selector_size;
+
+ /* Skip address size. */
+ read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ segment_selector_size = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+ if (segment_selector_size != 0)
+ {
+ complaint (_("unsupported segment selector size %u "
+ "in .debug_line section"),
+ segment_selector_size);
+ return nullptr;
+ }
+ }
+
+ LONGEST header_length = read_offset (parent_bfd, line_ptr, offset_size);
+ line_ptr += offset_size;
+ lh->statement_program_start = line_ptr + header_length;
+
+ lh->minimum_instruction_length = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ if (lh->version >= 4)
+ {
+ lh->maximum_ops_per_instruction = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+ }
+ else
+ lh->maximum_ops_per_instruction = 1;
+
+ if (lh->maximum_ops_per_instruction == 0)
+ {
+ lh->maximum_ops_per_instruction = 1;
+ complaint (_("invalid maximum_ops_per_instruction "
+ "in `.debug_line' section"));
+ }
+
+ lh->default_is_stmt = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ lh->line_base = read_1_signed_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ lh->line_range = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ lh->opcode_base = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+
+ lh->standard_opcode_lengths.reset (new unsigned char[lh->opcode_base]);
+
+ lh->standard_opcode_lengths[0] = 1; /* This should never be used anyway. */
+ for (i = 1; i < lh->opcode_base; ++i)
+ {
+ lh->standard_opcode_lengths[i] = read_1_byte (parent_bfd, line_ptr);
+ line_ptr += 1;
+ }
+
+ if (lh->version >= 5)
+ {
+ /* Read directory table. */
+ read_formatted_entries_separate
+ (parent_bfd, &line_ptr, line_str_data,
+ lh.get (), offset_size,
+ [] (struct line_header *header, const char *name,
+ dir_index d_index, unsigned int mod_time,
+ unsigned int length)
+ {
+ header->add_include_dir (name);
+ });
+
+ /* Read file name table. */
+ read_formatted_entries_separate
+ (parent_bfd, &line_ptr, line_str_data,
+ lh.get (), offset_size,
+ [] (struct line_header *header, const char *name,
+ dir_index d_index, unsigned int mod_time,
+ unsigned int length)
+ {
+ header->add_file_name (name, d_index, mod_time, length);
+ });
+ }
+ else
+ {
+ /* Read directory table. */
+ while ((cur_dir = read_direct_string (parent_bfd, line_ptr, &bytes_read)) != nullptr)
+ {
+ line_ptr += bytes_read;
+ lh->add_include_dir (cur_dir);
+ }
+ line_ptr += bytes_read;
+
+ /* Read file name table. */
+ while ((cur_file = read_direct_string (parent_bfd, line_ptr, &bytes_read)) != nullptr)
+ {
+ unsigned int mod_time, length;
+ dir_index d_index;
+
+ line_ptr += bytes_read;
+ d_index = (dir_index) read_unsigned_leb128 (parent_bfd, line_ptr, &bytes_read);
+ line_ptr += bytes_read;
+ mod_time = read_unsigned_leb128 (parent_bfd, line_ptr, &bytes_read);
+ line_ptr += bytes_read;
+ length = read_unsigned_leb128 (parent_bfd, line_ptr, &bytes_read);
+ line_ptr += bytes_read;
+
+ lh->add_file_name (cur_file, d_index, mod_time, length);
+ }
+ line_ptr += bytes_read;
+ }
+
+ if (line_ptr > (buf + line_data.size ()))
+ complaint (_("line number info header doesn't "
+ "fit in `.debug_line' section"));
+
+ *debug_line_ptr += unit_length + offset_size;
+ return lh;
+}
+
+
/* See line-header.h. */
line_header_up
--git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index 59a42e336f5..18af953343c 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -217,4 +217,14 @@ extern line_header_up dwarf_decode_line_header
struct dwarf2_section_info *section, const struct comp_unit_head *cu_header,
const char *comp_dir);
+/* Like dwarf_decode_line_header but the .debug_line and .debug_line_str
+ are stored in LINE_DATA and LINE_STR_DATA. This is used when these
+ sections are read from separate files without necessarily having
+ access to the entire debuginfo file they originate from. */
+
+extern line_header_up dwarf_decode_line_header_separate
+ (bfd *parent_bfd, gdb::array_view<const gdb_byte> line_data,
+ gdb::array_view<const gdb_byte> line_str_data,
+ const gdb_byte **debug_line_ptr);
+
#endif /* DWARF2_LINE_HEADER_H */
diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index f066d55bd1a..15f7d79e2d3 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -128,6 +128,9 @@ struct mapped_gdb_index final : public mapped_index_base
}
};
+struct mapped_debug_line;
+typedef std::unique_ptr<mapped_debug_line> mapped_debug_line_up;
+
struct dwarf2_gdb_index : public dwarf2_base_index_functions
{
/* This dumps minimal information about the index.
@@ -165,6 +168,15 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
block_search_flags search_flags,
domain_enum domain,
enum search_domain kind);
+
+ /* Filename information related to this .gdb_index. */
+ mapped_debug_line_up mdl;
+
+ /* Return true if any of the filenames in this .gdb_index's .debug_line
+ mapping match FILE_MATCHER. Initializes the mapping if necessary. */
+ bool filename_in_debug_line
+ (objfile *objfile,
+ gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher);
};
/* This dumps minimal information about the index.
@@ -517,6 +529,17 @@ dwarf2_gdb_index::do_expand_symtabs_matching
return result;
}
+bool
+dwarf2_gdb_index::filename_in_debug_line
+ (objfile *objfile,
+ gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher)
+{
+ if (mdl == nullptr)
+ mdl.reset (new mapped_debug_line (objfile));
+
+ return mdl->contains_matching_filename (file_matcher);
+}
+
bool
dwarf2_gdb_index::expand_symtabs_matching
(struct objfile *objfile,
@@ -547,6 +570,9 @@ dwarf2_gdb_index::expand_symtabs_matching
/* Objfile is a stub holding only index information. Try to reinitialize
objfile with the full debuginfo. */
+ if (file_matcher != nullptr
+ && !filename_in_debug_line (objfile, file_matcher))
+ return true;
if (!read_full_dwarf_from_debuginfod (objfile))
return false;
return do_expand_symtabs_matching (objfile, file_matcher, lookup_name,
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 12b0dbd1ee8..8bcdf9824cc 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -81,6 +81,7 @@
#include "gdbsupport/gdb_optional.h"
#include "gdbsupport/underlying.h"
#include "gdbsupport/hash_enum.h"
+#include "gdbsupport/scoped_mmap.h"
#include "filename-seen-cache.h"
#include "producer.h"
#include <fcntl.h>
@@ -2103,6 +2104,169 @@ dw2_get_file_names (dwarf2_per_cu_data *this_cu,
return this_cu->file_names;
}
+#if !HAVE_SYS_MMAN_H
+
+bool
+mapped_debug_line::contains_matching_filename
+ (gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher)
+{
+ return false;
+}
+
+gdb::array_view<const gdb_byte>
+mapped_debug_line::read_debug_line_separate
+ (char *filename, std::unique_ptr<index_cache_resource> *resource)
+{
+ return {};
+}
+
+bool
+mapped_debug_line::read_debug_line_from_debuginfod (objfile *objfile)
+{
+ return false;
+}
+
+#else /* !HAVE_SYS_MMAN_H */
+
+struct line_resource_mmap final : public index_cache_resource
+{
+ /* Try to mmap FILENAME. Throw an exception on failure, including if the
+ file doesn't exist. */
+ line_resource_mmap (const char *filename)
+ : mapping (mmap_file (filename))
+ {}
+
+ scoped_mmap mapping;
+};
+
+/* See read.h. */
+
+bool
+mapped_debug_line::contains_matching_filename
+ (gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher)
+{
+ for (line_header_up &lh : line_headers)
+ for (file_entry &fe : lh->file_names ())
+ {
+ const char *filename = fe.name;
+
+ if (file_matcher (fe.name, false))
+ return true;
+
+ bool basename_match = file_matcher (lbasename (fe.name), true);
+
+ if (!basenames_may_differ && !basename_match)
+ continue;
+
+ /* DW_AT_comp_dir is not explicitly mentioned in the .debug_line
+ until DWARF5. Since we don't have access to the CU at this
+ point we just check for a partial match on the filename.
+ If there is a match, the full debuginfo will be downloaded
+ ane the match will be re-evalute with DW_AT_comp_dir. */
+ if (lh->version < 5 && fe.d_index == 0)
+ return basename_match;
+
+ const char *dirname = fe.include_dir (&*lh);
+ std::string fullname;
+
+ if (dirname == nullptr || IS_ABSOLUTE_PATH (filename))
+ fullname = filename;
+ else
+ fullname = std::string (dirname) + SLASH_STRING + filename;
+
+ gdb::unique_xmalloc_ptr<char> rewritten
+ = rewrite_source_path (fullname.c_str ());
+ if (rewritten != nullptr)
+ fullname = rewritten.release ();
+
+ if (file_matcher (fullname.c_str (), false))
+ return true;
+ }
+
+ return false;
+}
+
+/* See read.h. */
+
+gdb::array_view<const gdb_byte>
+mapped_debug_line::read_debug_line_separate
+ (char *filename, std::unique_ptr<index_cache_resource> *resource)
+{
+ if (filename == nullptr)
+ return {};
+
+ try
+ {
+ line_resource_mmap *mmap_resource
+ = new line_resource_mmap (filename);
+
+ resource->reset (mmap_resource);
+
+ return gdb::array_view<const gdb_byte>
+ ((const gdb_byte *) mmap_resource->mapping.get (),
+ mmap_resource->mapping.size ());
+ }
+ catch (const gdb_exception &except)
+ {
+ exception_print (gdb_stderr, except);
+ }
+
+ return {};
+}
+
+/* See read.h. */
+
+bool
+mapped_debug_line::read_debug_line_from_debuginfod (objfile *objfile)
+{
+ const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
+ if (build_id == nullptr)
+ return false;
+
+ gdb::unique_xmalloc_ptr<char> line_path;
+ scoped_fd line_fd = debuginfod_section_query (build_id->data,
+ build_id->size,
+ bfd_get_filename
+ (objfile->obfd.get ()),
+ ".debug_line",
+ &line_path);
+
+ gdb::unique_xmalloc_ptr<char> line_str_path;
+ scoped_fd line_str_fd = debuginfod_section_query (build_id->data,
+ build_id->size,
+ bfd_get_filename
+ (objfile->obfd.get ()),
+ ".debug_line_str",
+ &line_str_path);
+
+ if (line_fd.get () < 0)
+ return false;
+
+ line_data = read_debug_line_separate (line_path.get (), &line_resource);
+ line_str_data = read_debug_line_separate (line_str_path.get (),
+ &line_str_resource);
+
+ const gdb_byte *line_ptr = line_data.data ();
+
+ while (line_ptr < line_data.data () + line_data.size ())
+ {
+ line_header_up lh
+ = dwarf_decode_line_header_separate (objfile->obfd.get (),
+ line_data, line_str_data,
+ &line_ptr);
+ line_headers.emplace_back (lh.release ());
+ }
+
+ return true;
+}
+#endif /* !HAVE_SYS_MMAN_H */
+
+mapped_debug_line::mapped_debug_line (objfile *objfile)
+{
+ if (!read_debug_line_from_debuginfod (objfile))
+ line_headers.clear ();
+}
+
/* A helper for the "quick" functions which computes and caches the
real path for a given file name from the line table. */
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 4e6c35cba5e..63d804f7b44 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -34,6 +34,7 @@
#include "gdbsupport/hash_enum.h"
#include "gdbsupport/function-view.h"
#include "gdbsupport/packed.h"
+#include "dwarf2/line-header.h"
/* Hold 'maintenance (set|show) dwarf' commands. */
extern struct cmd_list_element *set_dwarf_cmdlist;
@@ -951,4 +952,34 @@ extern bool read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
extern bool read_full_dwarf_from_debuginfod (struct objfile *objfile);
+struct mapped_debug_line
+{
+ mapped_debug_line (objfile *objfile);
+
+ /* Return true if any of the mapped .debug_line's filenames match
+ FILE_MATCHER. */
+
+ bool contains_matching_filename
+ (gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher);
+
+private:
+ std::vector<line_header_up> line_headers;
+
+ gdb::array_view<const gdb_byte> line_data;
+ gdb::array_view<const gdb_byte> line_str_data;
+
+ std::unique_ptr<index_cache_resource> line_resource;
+ std::unique_ptr<index_cache_resource> line_str_resource;
+
+ /* Download the .debug_line and .debug_line_str associated with OBJFILE
+ and populate line_headers. */
+
+ bool read_debug_line_from_debuginfod (objfile *objfile);
+
+ /* Initialize line_data and line_str_data with the .debug_line and
+ .debug_line_str downloaded read_debug_line_from_debuginfod. */
+
+ gdb::array_view<const gdb_byte> read_debug_line_separate
+ (char *filename, std::unique_ptr<index_cache_resource> *resource);
+};
#endif /* DWARF2READ_H */
--
2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled'
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
@ 2023-02-27 19:54 ` Eli Zaretskii
2023-05-24 9:31 ` Andrew Burgess
1 sibling, 0 replies; 25+ messages in thread
From: Eli Zaretskii @ 2023-02-27 19:54 UTC (permalink / raw)
To: Aaron Merey; +Cc: gdb-patches
> Cc: Aaron Merey <amerey@redhat.com>
> Date: Mon, 27 Feb 2023 14:42:07 -0500
> From: Aaron Merey via Gdb-patches <gdb-patches@sourceware.org>
>
> 'set debuginfod enabled lazy' turns on debuginfod downloading like
> 'set debuginfod enabled on' but also enables ELF/DWARFs section
> downloading via debuginfod_section_query.
>
> If support for debuginfod section queries was not found at configure
> time, 'set debuginfod enabled lazy' will print an error message
> indicating the missing support and default to 'set debuginfod enabled on'.
>
> Also update the help text and gdb.texinfo section for 'set debuginfod enabled'
> with information on the lazy setting.
> ---
> gdb/debuginfod-support.c | 20 +++++++++++++++++---
> gdb/doc/gdb.texinfo | 9 +++++++--
> 2 files changed, 24 insertions(+), 5 deletions(-)
Thanks.
> @@ -550,8 +560,12 @@ _initialize_debuginfod ()
> _("Set whether to use debuginfod."),
> _("Show whether to use debuginfod."),
> _("\
> -When on, enable the use of debuginfod to download missing debug info and\n\
> -source files."),
> +When set to \"on\", enable the use of debuginfod to download missing\n\
> +debug info and source files. \"off\" disables the use of debuginfod.\n\
> +When set to \"ask\", a prompt may ask whether to enable or disable\n\
> +debuginfod. When set to \"lazy\", debug info downloading will be\n\
> +deferred until it is required. GDB may also download components of\n\
^^
Two spaces there.
> +@item set debuginfod enabled lazy
> +@value{GDBN} will attempt to defer downloading entire debug info files until
> +necessary. @value{GDBN} may instead download individual components of the
^^
And there.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
` (5 preceding siblings ...)
2023-02-27 19:42 ` [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading Aaron Merey
@ 2023-02-28 11:11 ` Alexandra Petlanova Hajkova
2023-05-24 9:01 ` Andrew Burgess
7 siblings, 0 replies; 25+ messages in thread
From: Alexandra Petlanova Hajkova @ 2023-02-28 11:11 UTC (permalink / raw)
To: Aaron Merey; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 625 bytes --]
On Mon, Feb 27, 2023 at 8:42 PM Aaron Merey via Gdb-patches <
gdb-patches@sourceware.org> wrote:
> Add new function debuginfod_section_query. This function queries
> debuginfod servers for an individual ELF/DWARF section associated with
> a given build-id.
>
> Also check for libdebuginfod version >= 0.188 at configure time.
> debuginfod_section_query simply returns -ENOSYS if this condition
> is not met.
> ---
>
> I'm excited these series are coming. It's a great improvement to the
debuginfod. I currently do have set debuginfod enabled off because of the
time it sometimes takes to download all the missing packages.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/7] gdb/debuginfod: disable pagination during downloads
2023-02-27 19:42 ` [PATCH 3/7] gdb/debuginfod: disable pagination during downloads Aaron Merey
@ 2023-03-03 21:33 ` Tom Tromey
2023-03-06 23:07 ` Aaron Merey
2023-05-24 9:38 ` Andrew Burgess
1 sibling, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2023-03-03 21:33 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> Disable pagination during downloads in order to avoid inconvenient
Aaron> continue prompts "--Type <RET> for more, q to quit...".
I still don't get the rationale for this patch.
If the user has paging enabled, then barring some critical reason to
disable it, it seems like paging should happen.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/7] gdb/debuginfod: disable pagination during downloads
2023-03-03 21:33 ` Tom Tromey
@ 2023-03-06 23:07 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2023-03-06 23:07 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches
Hi Tom,
On Fri, Mar 3, 2023 at 4:33 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> Disable pagination during downloads in order to avoid inconvenient
> Aaron> continue prompts "--Type <RET> for more, q to quit...".
>
> I still don't get the rationale for this patch.
>
> If the user has paging enabled, then barring some critical reason to
> disable it, it seems like paging should happen.
During debuginfod progress updates, paging prompts sometimes happen
and sometimes don't happen due to conditions internal to gdb that aren't
obvious to the user. This may be perplexing to the user. IMO it's better
if the prompt behavior is more uniform.
I've also heard from at least one person that they left gdb unsupervised
during a large series of downloads and were annoyed when they returned
to see that the prompt stopped downloading part way through waiting
for input.
However you are right that users already have the option to disable
pagination. They can separately disable debuginfod progress messages
too. If you'd prefer that we keep the existing pagination behaviour, at
least users have multiple options if they don't like something.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/7] gdb/ui-file: Add newline tracking
2023-02-27 19:42 ` [PATCH 4/7] gdb/ui-file: Add newline tracking Aaron Merey
@ 2023-03-07 19:33 ` Tom Tromey
2023-03-07 20:30 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2023-03-07 19:33 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> Without any newline tracking in this case, we can end up with
Aaron> poorly formatted output:
Aaron> (gdb) backtrace
Aaron> [...]
Aaron> #8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron> function_cache=0x561221b224d0, state=<optimized out>...
Aaron> With newline tracking, the progress message writer can detect
Aaron> whether the message should start with a newline. This ensures
Aaron> that all progress messages print on their own line:
Aaron> (gdb) backtrace
Aaron> [...]
Aaron> #8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (
Aaron> Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron> function_cache=0x561221b224d0, state=<optimized out>...
Still kind of badly formatted though?
A long time ago, IIRC, the backtrace code had a second loop to do some
unwinding precisely to cause debuginfo to be loaded. I wonder if it
makes sense to resurrect this idea. It's unclear if this would work
with frame filters though.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading
2023-02-27 19:42 ` [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading Aaron Merey
@ 2023-03-07 20:20 ` Tom Tromey
2023-03-09 0:22 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2023-03-07 20:20 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> At the beginning of a session, gdb may attempt to download debuginfo
Aaron> for all shared libraries associated with the process or core file
Aaron> being debugged. This can be a waste of time and storage space when much
Aaron> of the debuginfo ends up not being used during the session.
Thank you for the patch.
Aaron> +/* See frame.h. */
Aaron> +
Aaron> +void
Aaron> +clear_comp_unit (struct objfile *objfile)
"comp unit" has a very different meaning in the DWARF code, it's
confusing here.
Maybe a name like "dwarf2_clear_frame_data" would be better.
Though I wonder how this gets populated if there's no frame info.
Aaron> + bool do_expand_symtabs_matching
I tend to think this isn't needed, see below.
Aaron> +bool
Aaron> +dwarf2_gdb_index::expand_symtabs_matching
Aaron> + (struct objfile *objfile,
Aaron> + gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
Aaron> + const lookup_name_info *lookup_name,
Aaron> + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
Aaron> + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
Aaron> + block_search_flags search_flags,
Aaron> + domain_enum domain,
Aaron> + enum search_domain kind)
Aaron> +{
Aaron> + if (objfile->flags & OBJF_READNEVER)
Can this even happen here? If 'readnever' is set, debuginfod simply
should never be queried in the first plae.
Aaron> + try
Aaron> + {
Aaron> + return do_expand_symtabs_matching (objfile, file_matcher, lookup_name,
Aaron> + symbol_matcher, expansion_notify,
Aaron> + search_flags, domain, kind);
Aaron> + }
Aaron> + catch (gdb_exception e)
Aaron> + {
Aaron> + if (!objfile->is_separate_index_stub ())
Aaron> + {
Aaron> + exception_print (gdb_stderr, e);
Aaron> + return false;
Aaron> + }
I guess the idea here is to try not to download anything -- maybe the
search will fail and we can skip the downloading entirely.
However, it seems better in this case to integrate it into the inner
loop of expand_symtabs_matching.
Also other methods of dwarf2_gdb_index can result in CU expansion, and
so require all the DWARF. So maybe some other refactoring is needed,
like some kind of virtual function that is called before any possible
call to dw2_instantiate_symtab along these code paths. I see some of
this in read.c but it seems like some are missing.
E.g., it seems like calls to dw2_expand_symtabs_matching_one are
unaffected, but should be.
Aaron> +/* See public.h. */
Aaron> +
Aaron> +bool
Aaron> +dwarf2_has_separate_index (struct objfile *objfile)
Aaron> +{
Aaron> + if (objfile->flags & OBJF_MAINLINE
What's the reason for testing this flag?
Anyway, gdb style is: (objfile->flags & OBJF_MAINLINE) != 0
Aaron> + return 0;
bool
Aaron> + /* We found a separate .gdb_index file so a separate debuginfo file
Aaron> + should exist, but don't want to read it until we really have to.
Aaron> + Create an objfile to own the index information and act as a
Aaron> + placeholder for the debuginfo that we have the option of aquiring
Aaron> + later. */
Aaron> + gdb_bfd_ref_ptr abfd (gdb_bfd_open (objfile_filename (objfile), gnutarget));
Aaron> + if (abfd == nullptr)
Aaron> + return false;
Aaron> +
Aaron> + dwarf2_per_bfd_objfile_data_key.clear (objfile);
Aaron> + dwarf2_objfile_data_key.clear (objfile);
This seems suspicious.
Aaron> + dwarf2_per_bfd *per_bfd;
Aaron> + dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
Aaron> +
Aaron> + /* Perform a limited initialization based to dwarf2_has_info and
Aaron> + dwarf2_initialize_objfile. */
Aaron> + if (per_objfile == nullptr)
Aaron> + {
Aaron> + per_bfd = dwarf2_per_bfd_objfile_data_key.get (objfile);
Aaron> + if (per_bfd == nullptr)
Aaron> + {
Aaron> + per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), nullptr, false);
Aaron> + dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
Aaron> + }
Aaron> + per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
Aaron> + }
Changing the objfile seems wrong.
I don't understand why this is needed.
Aaron> + struct objfile *stub = objfile->separate_debug_objfile;
Aaron> + per_objfile = get_dwarf2_per_objfile (stub);
Aaron> + if (per_objfile == nullptr)
Aaron> + {
Aaron> + per_bfd = dwarf2_per_bfd_objfile_data_key.get (stub);
Aaron> + if (per_bfd == nullptr)
Aaron> + {
Aaron> + per_bfd = new dwarf2_per_bfd (stub->obfd.get (), nullptr, false);
Aaron> + dwarf2_per_bfd_objfile_data_key.set (stub, per_bfd);
Aaron> + }
Aaron> + per_objfile = dwarf2_objfile_data_key.emplace (stub, stub, per_bfd);
Aaron> + }
This also seems suspect. Shouldn't it have been done when the new stub
objfile was created, like by dwarf2_has_info or something like that?
Aaron> diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
Aaron> index c9ef41893ee..ff8ef527c29 100644
Aaron> --- a/gdb/dwarf2/section.c
Aaron> +++ b/gdb/dwarf2/section.c
Aaron> @@ -54,7 +54,9 @@ dwarf2_section_info::get_bfd_owner () const
Aaron> section = get_containing_section ();
Aaron> gdb_assert (!section->is_virtual);
Aaron> }
Aaron> - gdb_assert (section->s.section != nullptr);
Aaron> + if (section->s.section == nullptr)
Aaron> + throw_error (NOT_FOUND_ERROR,
Aaron> + _("Can't find owner of DWARF section."));
I think this can just use error, but when is this needed?
Aaron> + /* Return true if this objfile holds .gdb_index data and represents
Aaron> + a debuginfo file whose download has been deferred. */
Aaron> +
Aaron> + bool is_separate_index_stub ()
Aaron> + {
Aaron> + return strcmp (original_name, SEPARATE_INDEX_FILENAME) == 0;
Aaron> + }
If this is needed then I think it's better to have a new OBJF_ flag
rather than a magic file name.
Aaron> +bool
Aaron> +objfile::reinit (struct bfd *abfd)
Aaron> +{
Aaron> + try
Aaron> + {
Aaron> + deferred_read_symbols (this, abfd);
Aaron> + return true;
Aaron> + }
Aaron> + catch (const gdb_exception &except)
Aaron> + {
Aaron> + exception_print (gdb_stderr, except);
Aaron> + this->flags |= OBJF_READNEVER;
Ok, maybe this answers my readnever question.
Aaron> +void
Aaron> +symbol_file_add_from_index (const gdb_bfd_ref_ptr &bfd,
Aaron> + symfile_add_flags symfile_flags,
Aaron> + struct objfile *parent)
Aaron> +{
Aaron> + section_addr_info sap = build_section_addr_info_from_objfile (parent);
Aaron> +
Aaron> + symbol_file_add_with_addrs
Aaron> + (bfd, SEPARATE_INDEX_FILENAME, symfile_flags, &sap,
Aaron> + (parent->flags & (OBJF_REORDERED | OBJF_SHARED | OBJF_READNOW
Aaron> + | OBJF_USERLOADED | OBJF_MAINLINE | OBJF_PSYMTABS_READ))
Aaron> + | OBJF_NOT_FILENAME, parent, true);
Aaron> +
Aaron> + objfile *result = parent->separate_debug_objfile;
Aaron> + init_objfile_sect_indices (result);
Aaron> +
Aaron> + return;
No need for this.
Aaron> +/* See symtab.h. */
Aaron> +
Aaron> +void
Aaron> +deferred_read_symbols (struct objfile *objfile, struct bfd *abfd)
Aaron> +{
Aaron> + const char *obfd_filename;
Aaron> +
Aaron> + obfd_filename = bfd_get_filename (abfd);
Aaron> + gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget));
Aaron> +
Aaron> + if (temp == nullptr)
Aaron> + error (_("Can't open %s to read symbols."), obfd_filename);
Aaron> +
Aaron> + if (!bfd_check_format (temp.get (), bfd_object))
Aaron> + error (_("Can't read symbols from %s: %s."), obfd_filename,
Aaron> + bfd_errmsg (bfd_get_error ()));
Aaron> +
Aaron> + objfile->obfd = std::move (temp);
Aaron> +
Aaron> + /* Nuke all the state that we will re-read. */
Aaron> + objfile->registry_fields.clear_registry ();
Aaron> + objfile->sections = NULL;
Aaron> + objfile->sect_index_bss = -1;
Aaron> + objfile->sect_index_data = -1;
Aaron> + objfile->sect_index_rodata = -1;
Aaron> + objfile->sect_index_text = -1;
Aaron> + objfile->compunit_symtabs = NULL;
Aaron> + objfile->template_symbols = NULL;
Aaron> +
Aaron> + objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd.get ()));
Aaron> + build_objfile_section_table (objfile);
Aaron> + (*objfile->sf->sym_init) (objfile);
Aaron> + init_objfile_sect_indices (objfile);
Aaron> +
Aaron> + objfile->section_offsets.resize (gdb_bfd_count_sections
Aaron> + (objfile->obfd.get ()));
Aaron> + read_symbols (objfile, 0);
Aaron> +
Aaron> + objfile->mtime = bfd_get_mtime (objfile->obfd.get ());
Aaron> + objfile->original_name
Aaron> + = obstack_strdup (&objfile->objfile_obstack, obfd_filename);
So, gdb already does this kind of thing in reread_symbols, and it's been
a source of bugs. I wonder if it's possible to just create a new,
non-stub objfile here and splice it in; removing the stub one.
Alternatively, I also don't really understand why this is needed here.
It seems like the reading of the symbols can be done purely in the DWARF
reader.
The idea of the "quick" API is to isolate the core gdb from whatever the
readers do under the hood. It seems to be that the stub objfile can be
created with the index, and then when the full data is needed, it can
just be read in without involving or notifying any other part of gdb.
This is what's currently done with section data after all, just in this
case it's being downloaded from some server.
One other thing to consider is the sharing of indices. The DWARF reader
tries pretty hard to share data across inferiors. However I think this
new code will download the index separately for each inferior. The full
DWARF will be shared again, due to the BFD cache.
The way this works is that the DWARF reader puts all the shareable data
into an object that is attached to the BFD's registry. Then when a new
objfile is initialized, it checks the BFD's registry to see if there's
already DWARF info attached. If so, it's reused.
The index code could maybe attach its own object to the parent BFD to
make this sharing work.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/7] gdb/ui-file: Add newline tracking
2023-03-07 19:33 ` Tom Tromey
@ 2023-03-07 20:30 ` Aaron Merey
2023-03-07 20:47 ` Tom Tromey
0 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2023-03-07 20:30 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches
On Tue, Mar 7, 2023 at 2:34 PM Tom Tromey <tom@tromey.com> wrote:
> Aaron> (gdb) backtrace
> Aaron> [...]
> Aaron> #8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (
> Aaron> Downloading separate debug info for /lib64/libpython3.11.so.1.0
> Aaron> function_cache=0x561221b224d0, state=<optimized out>...
>
> Still kind of badly formatted though?
>
> A long time ago, IIRC, the backtrace code had a second loop to do some
> unwinding precisely to cause debuginfo to be loaded. I wonder if it
> makes sense to resurrect this idea. It's unclear if this would work
> with frame filters though.
Can we accumulate the backtrace output in a buffer and print the whole
thing in one shot after all frames have been computed/filtered? Then all
debuginfod messages would print together, followed by the uninterrupted
backtrace.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading
2023-02-27 19:42 ` [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading Aaron Merey
@ 2023-03-07 20:36 ` Tom Tromey
2023-03-09 0:26 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2023-03-07 20:36 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> This patch adds functions read_formatted_entries_separate and
Aaron> dwarf_decode_line_header_separate. They are similar to
Aaron> read_formatted_entries and dwarf_decode_line_header except that they are
Aaron> able to work with .debug_line sections originating from separately
Aaron> downloaded files.
I think there has to be some other refactoring to avoid duplicating code
in this patch. Copying ~300 lines like that seems bad, especially
considering they'll need parallel updates whenever we find bugs, when
DWARF changes, etc.
Aaron> + gdb::unique_xmalloc_ptr<char> line_path;
Aaron> + scoped_fd line_fd = debuginfod_section_query (build_id->data,
Aaron> + build_id->size,
Aaron> + bfd_get_filename
Aaron> + (objfile->obfd.get ()),
Aaron> + ".debug_line",
Aaron> + &line_path);
For gdb's purposes, it's a shame debuginfod works explicitly on sections
and not as more of a locally-caching filesystem-like API. With the
latter we would perhaps have very little or nothing to do to make this
work, provided we were careful to keep the gdb-index code lazy about
reading sections. Also, inside gdb, all the sharing across inferiors
and such would automatically work. The cost would be whatever BFD
requests when identifying a file, not sure how much data that is.
Anyway, back to current reality -- the DWARF reader already does try to
lazily map section data. So I think one big question is, why can't this
be the mechanism for all the sections with debuginfod? That is, stick
the debuginfod calls into dwarf2_section_info::read. I don't know what
the debuginfod client does under the hood, but if it doesn't cache this
data somewhere, perhaps gdb could.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/7] gdb/ui-file: Add newline tracking
2023-03-07 20:30 ` Aaron Merey
@ 2023-03-07 20:47 ` Tom Tromey
0 siblings, 0 replies; 25+ messages in thread
From: Tom Tromey @ 2023-03-07 20:47 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: Tom Tromey, Aaron Merey
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> On Tue, Mar 7, 2023 at 2:34 PM Tom Tromey <tom@tromey.com> wrote:
Aaron> (gdb) backtrace
Aaron> [...]
Aaron> #8 0x00007fbe8af7d7cf in pygi_invoke_c_callable (
Aaron> Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron> function_cache=0x561221b224d0, state=<optimized out>...
>> Still kind of badly formatted though?
>> A long time ago, IIRC, the backtrace code had a second loop to do some
>> unwinding precisely to cause debuginfo to be loaded. I wonder if it
>> makes sense to resurrect this idea. It's unclear if this would work
>> with frame filters though.
Aaron> Can we accumulate the backtrace output in a buffer and print the whole
Aaron> thing in one shot after all frames have been computed/filtered? Then all
Aaron> debuginfod messages would print together, followed by the uninterrupted
Aaron> backtrace.
I like this idea.
To make it still seem "incremental", it seems like we'd only need to
print a single frame to a ui_file. Then after that frame is done, the
buffer can be printed and gdb can move on to the next one.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading
2023-03-07 20:20 ` Tom Tromey
@ 2023-03-09 0:22 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2023-03-09 0:22 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches
On Tue, Mar 7, 2023 at 3:20 PM Tom Tromey <tom@tromey.com> wrote:
> Aaron> +/* See frame.h. */
> Aaron> +
> Aaron> +void
> Aaron> +clear_comp_unit (struct objfile *objfile)
>
> "comp unit" has a very different meaning in the DWARF code, it's
> confusing here.
>
> Maybe a name like "dwarf2_clear_frame_data" would be better.
> Though I wonder how this gets populated if there's no frame info.
Will change the name. I don't remember the details but the comp_unit
could end up with an empty fde_table when DWARF wasn't present. The
empty table would hang around and indicate a lack of DWARF frame data
unless cleared this way, even after DWARF was acquired.
> Aaron> + try
> Aaron> + {
> Aaron> + return do_expand_symtabs_matching (objfile, file_matcher, lookup_name,
> Aaron> + symbol_matcher, expansion_notify,
> Aaron> + search_flags, domain, kind);
> Aaron> + }
> Aaron> + catch (gdb_exception e)
> Aaron> + {
> Aaron> + if (!objfile->is_separate_index_stub ())
> Aaron> + {
> Aaron> + exception_print (gdb_stderr, e);
> Aaron> + return false;
> Aaron> + }
>
> I guess the idea here is to try not to download anything -- maybe the
> search will fail and we can skip the downloading entirely.
>
> However, it seems better in this case to integrate it into the inner
> loop of expand_symtabs_matching.
The reason for downloading the full DWARF at a level above the inner
loop of expand_symtabs_matching is because the loop acts on the
per_objfile and per_bfd which are deleted and reinitialized when
full DWARF is downloaded.
> Also other methods of dwarf2_gdb_index can result in CU expansion, and
> so require all the DWARF. So maybe some other refactoring is needed,
> like some kind of virtual function that is called before any possible
> call to dw2_instantiate_symtab along these code paths. I see some of
> this in read.c but it seems like some are missing.
>
> E.g., it seems like calls to dw2_expand_symtabs_matching_one are
> unaffected, but should be.
Will fix this.
> Aaron> +/* See public.h. */
> Aaron> +
> Aaron> +bool
> Aaron> +dwarf2_has_separate_index (struct objfile *objfile)
> Aaron> +{
> Aaron> + if (objfile->flags & OBJF_MAINLINE
>
> What's the reason for testing this flag?
>
> Anyway, gdb style is: (objfile->flags & OBJF_MAINLINE) != 0
I wanted to avoid unnecessarily downloading a separate .gdb_index
since we'll always want the debuginfo of the main executable.
> Aaron> + return 0;
>
> bool
>
> Aaron> + /* We found a separate .gdb_index file so a separate debuginfo file
> Aaron> + should exist, but don't want to read it until we really have to.
> Aaron> + Create an objfile to own the index information and act as a
> Aaron> + placeholder for the debuginfo that we have the option of aquiring
> Aaron> + later. */
> Aaron> + gdb_bfd_ref_ptr abfd (gdb_bfd_open (objfile_filename (objfile), gnutarget));
> Aaron> + if (abfd == nullptr)
> Aaron> + return false;
> Aaron> +
> Aaron> + dwarf2_per_bfd_objfile_data_key.clear (objfile);
> Aaron> + dwarf2_objfile_data_key.clear (objfile);
>
> This seems suspicious.
>
> Aaron> + dwarf2_per_bfd *per_bfd;
> Aaron> + dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
> Aaron> +
> Aaron> + /* Perform a limited initialization based to dwarf2_has_info and
> Aaron> + dwarf2_initialize_objfile. */
> Aaron> + if (per_objfile == nullptr)
> Aaron> + {
> Aaron> + per_bfd = dwarf2_per_bfd_objfile_data_key.get (objfile);
> Aaron> + if (per_bfd == nullptr)
> Aaron> + {
> Aaron> + per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), nullptr, false);
> Aaron> + dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
> Aaron> + }
> Aaron> + per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
> Aaron> + }
>
> Changing the objfile seems wrong.
> I don't understand why this is needed.
You're right, this is unnecessary.
> Aaron> + struct objfile *stub = objfile->separate_debug_objfile;
> Aaron> + per_objfile = get_dwarf2_per_objfile (stub);
> Aaron> + if (per_objfile == nullptr)
> Aaron> + {
> Aaron> + per_bfd = dwarf2_per_bfd_objfile_data_key.get (stub);
> Aaron> + if (per_bfd == nullptr)
> Aaron> + {
> Aaron> + per_bfd = new dwarf2_per_bfd (stub->obfd.get (), nullptr, false);
> Aaron> + dwarf2_per_bfd_objfile_data_key.set (stub, per_bfd);
> Aaron> + }
> Aaron> + per_objfile = dwarf2_objfile_data_key.emplace (stub, stub, per_bfd);
> Aaron> + }
>
> This also seems suspect. Shouldn't it have been done when the new stub
> objfile was created, like by dwarf2_has_info or something like that?
We avoid a call to dwarf2_has_info by returning early from
symbol_file_add_with_addrs.
I tried to contain the stub initialization to this function to avoid unintended
interactions with the normal objfile process while also minimizing changes
to existing code.
We don't want to interact with the stub's bfd (a bfd of the shared library)
any more than we need to. It's only there because gdb requires some of
the section indices and offsets.
> Aaron> diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
> Aaron> index c9ef41893ee..ff8ef527c29 100644
> Aaron> --- a/gdb/dwarf2/section.c
> Aaron> +++ b/gdb/dwarf2/section.c
> Aaron> @@ -54,7 +54,9 @@ dwarf2_section_info::get_bfd_owner () const
> Aaron> section = get_containing_section ();
> Aaron> gdb_assert (!section->is_virtual);
> Aaron> }
> Aaron> - gdb_assert (section->s.section != nullptr);
> Aaron> + if (section->s.section == nullptr)
> Aaron> + throw_error (NOT_FOUND_ERROR,
> Aaron> + _("Can't find owner of DWARF section."));
>
> I think this can just use error, but when is this needed?
section->s.section == nullptr when attempting to expand symtabs for a
stub objfile. This error is caught in expand_symtabs_matching and
find_pc_sect_compunit_symtab which triggers the full debuginfo download.
> Aaron> + /* Return true if this objfile holds .gdb_index data and represents
> Aaron> + a debuginfo file whose download has been deferred. */
> Aaron> +
> Aaron> + bool is_separate_index_stub ()
> Aaron> + {
> Aaron> + return strcmp (original_name, SEPARATE_INDEX_FILENAME) == 0;
> Aaron> + }
>
> If this is needed then I think it's better to have a new OBJF_ flag
> rather than a magic file name.
Agreed.
> Aaron> +bool
> Aaron> +objfile::reinit (struct bfd *abfd)
> Aaron> +{
> Aaron> + try
> Aaron> + {
> Aaron> + deferred_read_symbols (this, abfd);
> Aaron> + return true;
> Aaron> + }
> Aaron> + catch (const gdb_exception &except)
> Aaron> + {
> Aaron> + exception_print (gdb_stderr, except);
> Aaron> + this->flags |= OBJF_READNEVER;
>
> Ok, maybe this answers my readnever question.
>
> Aaron> +void
> Aaron> +symbol_file_add_from_index (const gdb_bfd_ref_ptr &bfd,
> Aaron> + symfile_add_flags symfile_flags,
> Aaron> + struct objfile *parent)
> Aaron> +{
> Aaron> + section_addr_info sap = build_section_addr_info_from_objfile (parent);
> Aaron> +
> Aaron> + symbol_file_add_with_addrs
> Aaron> + (bfd, SEPARATE_INDEX_FILENAME, symfile_flags, &sap,
> Aaron> + (parent->flags & (OBJF_REORDERED | OBJF_SHARED | OBJF_READNOW
> Aaron> + | OBJF_USERLOADED | OBJF_MAINLINE | OBJF_PSYMTABS_READ))
> Aaron> + | OBJF_NOT_FILENAME, parent, true);
> Aaron> +
> Aaron> + objfile *result = parent->separate_debug_objfile;
> Aaron> + init_objfile_sect_indices (result);
> Aaron> +
> Aaron> + return;
>
> No need for this.
Ok.
> Aaron> +/* See symtab.h. */
> Aaron> +
> Aaron> +void
> Aaron> +deferred_read_symbols (struct objfile *objfile, struct bfd *abfd)
> Aaron> +{
> Aaron> + const char *obfd_filename;
> Aaron> +
> Aaron> + obfd_filename = bfd_get_filename (abfd);
> Aaron> + gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget));
> Aaron> +
> Aaron> + if (temp == nullptr)
> Aaron> + error (_("Can't open %s to read symbols."), obfd_filename);
> Aaron> +
> Aaron> + if (!bfd_check_format (temp.get (), bfd_object))
> Aaron> + error (_("Can't read symbols from %s: %s."), obfd_filename,
> Aaron> + bfd_errmsg (bfd_get_error ()));
> Aaron> +
> Aaron> + objfile->obfd = std::move (temp);
> Aaron> +
> Aaron> + /* Nuke all the state that we will re-read. */
> Aaron> + objfile->registry_fields.clear_registry ();
> Aaron> + objfile->sections = NULL;
> Aaron> + objfile->sect_index_bss = -1;
> Aaron> + objfile->sect_index_data = -1;
> Aaron> + objfile->sect_index_rodata = -1;
> Aaron> + objfile->sect_index_text = -1;
> Aaron> + objfile->compunit_symtabs = NULL;
> Aaron> + objfile->template_symbols = NULL;
> Aaron> +
> Aaron> + objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd.get ()));
> Aaron> + build_objfile_section_table (objfile);
> Aaron> + (*objfile->sf->sym_init) (objfile);
> Aaron> + init_objfile_sect_indices (objfile);
> Aaron> +
> Aaron> + objfile->section_offsets.resize (gdb_bfd_count_sections
> Aaron> + (objfile->obfd.get ()));
> Aaron> + read_symbols (objfile, 0);
> Aaron> +
> Aaron> + objfile->mtime = bfd_get_mtime (objfile->obfd.get ());
> Aaron> + objfile->original_name
> Aaron> + = obstack_strdup (&objfile->objfile_obstack, obfd_filename);
>
> So, gdb already does this kind of thing in reread_symbols, and it's been
> a source of bugs. I wonder if it's possible to just create a new,
> non-stub objfile here and splice it in; removing the stub one.
Ok we could do something like std::swap so that we create the new objfile
the normal way while still using the pointer to the stub (which functions
down the callstack will still be using when deferred_read_symbols is
called).
> Alternatively, I also don't really understand why this is needed here.
> It seems like the reading of the symbols can be done purely in the DWARF
> reader.
>
> The idea of the "quick" API is to isolate the core gdb from whatever the
> readers do under the hood. It seems to be that the stub objfile can be
> created with the index, and then when the full data is needed, it can
> just be read in without involving or notifying any other part of gdb.
> This is what's currently done with section data after all, just in this
> case it's being downloaded from some server.
I can try to incorporate this, but I think there still needs to be a step where
we remove everything in the stub related to its original bfd and update it
with a bfd of the debuginfo. Otherwise it will be in an inconsistent state
with the wrong bfd, per_bfd, etc.
> One other thing to consider is the sharing of indices. The DWARF reader
> tries pretty hard to share data across inferiors. However I think this
> new code will download the index separately for each inferior. The full
> DWARF will be shared again, due to the BFD cache.
>
> The way this works is that the DWARF reader puts all the shareable data
> into an object that is attached to the BFD's registry. Then when a new
> objfile is initialized, it checks the BFD's registry to see if there's
> already DWARF info attached. If so, it's reused.
>
> The index code could maybe attach its own object to the parent BFD to
> make this sharing work.
Debuginfod caches section downloads but the contents will have to be
reread, I'll see if the read contents can be shared.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading
2023-03-07 20:36 ` Tom Tromey
@ 2023-03-09 0:26 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2023-03-09 0:26 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches
On Tue, Mar 7, 2023 at 3:36 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> This patch adds functions read_formatted_entries_separate and
> Aaron> dwarf_decode_line_header_separate. They are similar to
> Aaron> read_formatted_entries and dwarf_decode_line_header except that they are
> Aaron> able to work with .debug_line sections originating from separately
> Aaron> downloaded files.
>
> I think there has to be some other refactoring to avoid duplicating code
> in this patch. Copying ~300 lines like that seems bad, especially
> considering they'll need parallel updates whenever we find bugs, when
> DWARF changes, etc.
Will do.
>
> Aaron> + gdb::unique_xmalloc_ptr<char> line_path;
> Aaron> + scoped_fd line_fd = debuginfod_section_query (build_id->data,
> Aaron> + build_id->size,
> Aaron> + bfd_get_filename
> Aaron> + (objfile->obfd.get ()),
> Aaron> + ".debug_line",
> Aaron> + &line_path);
>
> For gdb's purposes, it's a shame debuginfod works explicitly on sections
> and not as more of a locally-caching filesystem-like API. With the
> latter we would perhaps have very little or nothing to do to make this
> work, provided we were careful to keep the gdb-index code lazy about
> reading sections. Also, inside gdb, all the sharing across inferiors
> and such would automatically work. The cost would be whatever BFD
> requests when identifying a file, not sure how much data that is.
That's an interesting idea. Though I wonder if the extra latency from
possibly many more small, frequent transfers outweighs the benefits
over downloading just .gdb_index and maybe .debug_line and the
full debuginfo.
>
> Anyway, back to current reality -- the DWARF reader already does try to
> lazily map section data. So I think one big question is, why can't this
> be the mechanism for all the sections with debuginfod? That is, stick
> the debuginfod calls into dwarf2_section_info::read. I don't know what
> the debuginfod client does under the hood, but if it doesn't cache this
> data somewhere, perhaps gdb could.
Debuginfod does cache sections. But before the full debuginfo is downloaded
gdb only knows about sections present in the solib binary. This won't
include any .debug_* sections (otherwise gdb wouldn't be using debuginfod
here). I think to make this work debuginfod would have to provide the
section header table too?
Since .debug_info is so connected to the other .debug_* sections do we
have much to gain by downloading sections separately once we know we'll
need the .debug_info? At that point we might as well just download the
whole file.
Thanks for reviewing these patches.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests
2023-02-27 19:42 ` [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests Aaron Merey
@ 2023-05-02 15:48 ` Andrew Burgess
2023-05-02 16:24 ` Aaron Merey
2023-05-24 10:12 ` Andrew Burgess
1 sibling, 1 reply; 25+ messages in thread
From: Andrew Burgess @ 2023-05-02 15:48 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches, gdb-patches; +Cc: Aaron Merey
Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Add some tests for 'set debuginfod enabled lazy', .gdb_index downloading
> and deferred debuginfo downloading.
> ---
> .../gdb.debuginfod/fetch_src_and_symbols.exp | 58 +++++++++++++++++++
> gdb/testsuite/gdb.debuginfod/libsection.c | 24 ++++++++
> gdb/testsuite/gdb.debuginfod/section.c | 28 +++++++++
> 3 files changed, 110 insertions(+)
> create mode 100644 gdb/testsuite/gdb.debuginfod/libsection.c
> create mode 100644 gdb/testsuite/gdb.debuginfod/section.c
I compile GDB with both '-D_GLIBCXX_DEBUG=1' and
'-D_GLIBCXX_DEBUG_PEDANTIC=1' defined.
After applying this series I see the following GDB crash when running
the fetch_src_and_symbols.exp test:
(gdb) file
No executable file now.
/usr/include/c++/9/debug/safe_iterator.h:597:
In function:
__gnu_debug::_Safe_iterator<_Iterator, _Sequence,
std::bidirectional_iterator_tag>& __gnu_debug::_Safe_iterator<_Iterator,
_Sequence, std::bidirectional_iterator_tag>::operator--() [with
_Iterator = std::__cxx1998::_List_iterator<std::unique_ptr<objfile> >;
_Sequence = std::__debug::list<std::unique_ptr<objfile> >]
Error: attempt to decrement a dereferenceable (start-of-sequence) iterator.
Objects involved in the operation:
iterator "this" @ 0x0x7ffebcfec598 {
type = std::__cxx1998::_List_iterator<std::unique_ptr<objfile, std::default_delete<objfile> > > (mutable iterato>
state = dereferenceable (start-of-sequence);
references sequence with type 'std::__debug::list<std::unique_ptr<objfile, std::default_delete<objfile> >, std::>
}
Fatal signal: Aborted
----- Backtrace -----
0x603500 gdb_internal_backtrace_1
../../src.dev-4/gdb/bt-utils.c:122
0x6035a3 _Z22gdb_internal_backtracev
../../src.dev-4/gdb/bt-utils.c:168
0x8a7732 handle_fatal_signal
../../src.dev-4/gdb/event-top.c:880
0x7fd0b9c686af ???
0x7fd0b9c68625 ???
0x7fd0b9c518d8 ???
0x7fd0b9ff5d18 ???
0x9eae05 _ZN11__gnu_debug14_Safe_iteratorINSt9__cxx199814_List_iteratorISt10unique_ptrI7objfileSt14default_deleteIS4_E>
/usr/include/c++/9/debug/safe_iterator.h:597
0x9e9843 _ZN27unwrapping_reverse_iteratorIN11__gnu_debug14_Safe_iteratorINSt9__cxx199814_List_iteratorISt10unique_ptrI>
../../src.dev-4/gdb/progspace.h:116
0x9e840a _ZN22reverse_iterator_rangeI27unwrapping_reverse_iteratorIN11__gnu_debug14_Safe_iteratorINSt9__cxx199814_List>
../../src.dev-4/gdb/../gdbsupport/iterator-range.h:85
0x9e7af3 _ZN13program_space13objfiles_safeEv
../../src.dev-4/gdb/progspace.h:280
0xb5386c _Z20objfile_purge_solibsv
../../src.dev-4/gdb/objfiles.c:804
0xd93da0 _Z19no_shared_librariesPKci
../../src.dev-4/gdb/solib.c:1285
0xdeb0a4 _Z17symbol_file_cleari
../../src.dev-4/gdb/symfile.c:1234
0xdec299 _Z19symbol_file_commandPKci
../../src.dev-4/gdb/symfile.c:1628
0x8aab49 file_command
../../src.dev-4/gdb/exec.c:554
0x6800d0 do_simple_func
../../src.dev-4/gdb/cli/cli-decode.c:95
0x685145 _Z8cmd_funcP16cmd_list_elementPKci
../../src.dev-4/gdb/cli/cli-decode.c:2735
0xe90e33 _Z15execute_commandPKci
../../src.dev-4/gdb/top.c:572
0x8a6eea _Z15command_handlerPKc
../../src.dev-4/gdb/event-top.c:543
0x8a73f3 _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
../../src.dev-4/gdb/event-top.c:779
Thanks,
Andrew
>
> diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> index 8158c5c3cc6..fc7e4b685dc 100644
> --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> @@ -42,6 +42,23 @@ if { [gdb_compile "$sourcetmp" "${binfile}2" executable {debug build-id}] != ""
> return -1
> }
>
> +set sectfile "section"
> +set sectsrc $srcdir/$subdir/section.c
> +set sectexec [standard_output_file $sectfile]
> +
> +set libfile "libsection"
> +set libsrc $srcdir/$subdir/$libfile.c
> +set lib_sl [standard_output_file $libfile.sl]
> +
> +set lib_opts [list debug build-id ]
> +set exec_opts [list debug build-id shlib=$lib_sl]
> +
> +if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
> + || [gdb_compile $sectsrc $sectexec executable $exec_opts] != "" } {
> + untested "failed to compile"
> + return -1
> +}
> +
> # Write some assembly that just has a .gnu_debugaltlink section.
> # Copied from testsuite/gdb.dwarf2/dwzbuildid.exp.
> proc write_just_debugaltlink {filename dwzname buildid} {
> @@ -94,6 +111,7 @@ proc write_dwarf_file {filename buildid {value 99}} {
> }
>
> set corefile [standard_output_file "corefile"]
> +set lazy_support -1
>
> # Setup the global variable DEBUGDIR as a directory containing the
> # debug information for the test executable.
> @@ -103,6 +121,8 @@ set corefile [standard_output_file "corefile"]
> # running.
> proc_with_prefix no_url { } {
> global binfile outputdir debugdir
> + global sectexec lib_sl libfile lazy_support
> + global gdb_prompt
>
> setenv DEBUGINFOD_URLS ""
>
> @@ -119,11 +139,18 @@ proc_with_prefix no_url { } {
> return -1
> }
>
> + if { [gdb_gnu_strip_debug $lib_sl ""] != 0} {
> + fail "strip shlib debuginfo"
> + return -1
> + }
> +
> set debugdir [standard_output_file "debug"]
> set debuginfo [standard_output_file "fetch_src_and_symbols.debug"]
> + set debuginfo_shlib [standard_output_file $libfile.sl.debug]
>
> file mkdir $debugdir
> file rename -force $debuginfo $debugdir
> + file rename -force $debuginfo_shlib $debugdir
>
> # Test that GDB cannot find symbols without debuginfod.
> clean_restart $binfile
> @@ -171,6 +198,25 @@ proc_with_prefix no_url { } {
>
> clean_restart
> gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
> + clean_restart
> +
> + # Check for lazy downloading support.
> + gdb_test_multiple "set debuginfod enabled lazy" "check for lazy" {
> + -re ".*lazy downloading is not compiled into GDB.*\n.*${gdb_prompt} $" {
> + set lazy_support 0
> + }
> + -re ".*${gdb_prompt} $" {
> + set lazy_support 1
> + }
> + }
> +
> + if {$lazy_support == 1} {
> + gdb_test "file $sectexec" "" "file [file tail $sectexec] file no url"
> + gdb_test "start" "" "file [file tail $sectexec] start no url"
> + gdb_test "info sharedlibrary" ".*Yes \\(\\*\\).*libsection.*" "lib no debug"
> + } else {
> + untested "lazy support no_url"
> + }
> }
>
> # Test that GDB prints the debuginfod URLs when loading files. URLS
> @@ -208,6 +254,7 @@ proc disable_delete_breakpoints {} {
> # expected debug information.
> proc_with_prefix local_url { } {
> global binfile outputdir debugdir db
> + global sectexec lib_sl libfile lazy_support
>
> set url [start_debuginfod $db $debugdir]
> if { $url == "" } {
> @@ -256,6 +303,17 @@ proc_with_prefix local_url { } {
> "file [file tail ${binfile}_alt.o]" \
> $enable_debuginfod_question "y"
>
> + if {$lazy_support == 1} {
> + # GDB should now download .gdb_index.
> + clean_restart
> + gdb_test "set debuginfod enabled lazy" "" "set lazy urls"
> + gdb_test "file $sectexec" "" "file [file tail $sectexec] file urls"
> + set res ".*Download.*\.gdb_index.*libsection.*\r\nDownload.*debug info.*libsection.*"
> + gdb_test "start $sectexec" $res "file [file tail $sectexec] start urls"
> + } else {
> + untested "lazy support urls"
> + }
> +
> # Configure debuginfod with commands.
> unsetenv DEBUGINFOD_URLS
> clean_restart
> diff --git a/gdb/testsuite/gdb.debuginfod/libsection.c b/gdb/testsuite/gdb.debuginfod/libsection.c
> new file mode 100644
> index 00000000000..510f9205282
> --- /dev/null
> +++ b/gdb/testsuite/gdb.debuginfod/libsection.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2020-2023 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +
> +void
> +libsection_test ()
> +{
> + printf ("In libsection\n");
> +}
> diff --git a/gdb/testsuite/gdb.debuginfod/section.c b/gdb/testsuite/gdb.debuginfod/section.c
> new file mode 100644
> index 00000000000..3598896c1bc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.debuginfod/section.c
> @@ -0,0 +1,28 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2020-2023 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <fcntl.h>
> +
> +extern void libsection_test ();
> +
> +int
> +main()
> +{
> + (void) open ("", 0);
> + libsection_test ();
> + return 0;
> +}
> --
> 2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests
2023-05-02 15:48 ` Andrew Burgess
@ 2023-05-02 16:24 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2023-05-02 16:24 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Aaron Merey via Gdb-patches
Hi Andrew,
On Tue, May 2, 2023 at 11:48 AM Andrew Burgess <aburgess@redhat.com> wrote:
> I compile GDB with both '-D_GLIBCXX_DEBUG=1' and
> '-D_GLIBCXX_DEBUG_PEDANTIC=1' defined.
>
> After applying this series I see the following GDB crash when running
> the fetch_src_and_symbols.exp test:
>
> (gdb) file
> No executable file now.
> /usr/include/c++/9/debug/safe_iterator.h:597:
> In function:
> __gnu_debug::_Safe_iterator<_Iterator, _Sequence,
> std::bidirectional_iterator_tag>& __gnu_debug::_Safe_iterator<_Iterator,
> _Sequence, std::bidirectional_iterator_tag>::operator--() [with
> _Iterator = std::__cxx1998::_List_iterator<std::unique_ptr<objfile> >;
> _Sequence = std::__debug::list<std::unique_ptr<objfile> >]
>
> Error: attempt to decrement a dereferenceable (start-of-sequence) iterator.
Thanks for spotting this. It looks like the culprit is from the following change
in patch 5/7 [1]:
diff --git a/gdbsupport/iterator-range.h b/gdbsupport/iterator-range.h
index e934f5ebf01..853ebb9074a 100644
--- a/gdbsupport/iterator-range.h
+++ b/gdbsupport/iterator-range.h
@@ -61,4 +61,49 @@ struct iterator_range
+template <typename IteratorType>
+struct reverse_iterator_range
+{
+ using iterator = IteratorType;
+
+ /* Create a reverse iterator range using explicit BEGIN and END
iterators. */
+ template <typename... Args>
+ reverse_iterator_range (IteratorType begin, IteratorType end)
+ : m_begin (std::move (end)), m_end (std::move (begin))
+ {
+ if (m_begin != m_end)
+ {
+ /* M_BEGIN is the base iterator's one-past-the-end. Increment it
+ so it points to the ranges's last element. */
+ ++m_begin;
+
+ /* M_END points to the range's first element. Increment it so it's
+ one-past-the-end. */
+ ++m_end;
+ }
+ }
[...]
'++m_end' has the effect of decrementing a std::list begin() iterator, though it
should never be dereferenced. I'll fix this and post a follow up to patch 5/7.
Aaron
[1] https://sourceware.org/pipermail/gdb-patches/2023-April/199076.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
` (6 preceding siblings ...)
2023-02-28 11:11 ` [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Alexandra Petlanova Hajkova
@ 2023-05-24 9:01 ` Andrew Burgess
7 siblings, 0 replies; 25+ messages in thread
From: Andrew Burgess @ 2023-05-24 9:01 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches, gdb-patches; +Cc: Aaron Merey
Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Add new function debuginfod_section_query. This function queries
> debuginfod servers for an individual ELF/DWARF section associated with
> a given build-id.
>
> Also check for libdebuginfod version >= 0.188 at configure time.
> debuginfod_section_query simply returns -ENOSYS if this condition
> is not met.
> ---
> config/debuginfod.m4 | 27 +++++++++++
> gdb/config.in | 3 ++
> gdb/configure | 102 ++++++++++++++++++++++++++++++++++++++-
> gdb/configure.ac | 2 +-
> gdb/debuginfod-support.c | 55 +++++++++++++++++++++
> gdb/debuginfod-support.h | 24 +++++++++
> 6 files changed, 211 insertions(+), 2 deletions(-)
>
> diff --git a/config/debuginfod.m4 b/config/debuginfod.m4
> index 2c1bfbdb544..203a799719d 100644
> --- a/config/debuginfod.m4
> +++ b/config/debuginfod.m4
> @@ -26,3 +26,30 @@ else
> AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
> fi
> ])
> +
> +AC_DEFUN([AC_DEBUGINFOD_SECTION],
> +[
> +# Handle optional debuginfod support as well as optional section downloading support
Missing a '.' from the end of this sentence.
I'd also like to see this comment explain what this define actually
does. Like:
# Handle optional debuginfod support as well as optional section
# downloading support.
#
# Define HAVE_LIBDEBUGINFOD if ....
#
# Define HAVE_DEBUGINFOD_FIND_SECTION if ...
> +AC_ARG_WITH([debuginfod],
> + AC_HELP_STRING([--with-debuginfod], [Enable debuginfo lookups with debuginfod (auto/yes/no)]),
> + [], [with_debuginfod=auto])
> +AC_MSG_CHECKING([whether to use debuginfod])
> +AC_MSG_RESULT([$with_debuginfod])
> +
> +if test "x$with_debuginfod" != xno; then
> + PKG_CHECK_MODULES([DEBUGINFOD], [libdebuginfod >= 0.188],
> + [AC_DEFINE([HAVE_DEBUGINFOD_FIND_SECTION], [1],
I think this should be HAVE_LIBDEBUGINFOD_FIND_SECTION in order to be
consistent with the plain HAVE_LIBDEBUGINFOD.
> + [Define to 1 if debuginfod section downloading is supported.])],
> + [AC_MSG_WARN([libdebuginfod is missing or some features may be unavailable.])])
> +
> + PKG_CHECK_MODULES([DEBUGINFOD], [libdebuginfod >= 0.179],
> + [AC_DEFINE([HAVE_LIBDEBUGINFOD], [1], [Define to 1 if debuginfod is enabled.])],
> + [if test "x$with_debuginfod" = xyes; then
> + AC_MSG_ERROR(["--with-debuginfod was given, but libdebuginfod is missing or unusable."])
> + else
> + AC_MSG_WARN([libdebuginfod is missing or unusable; some features may be unavailable.])
> + fi])
> +else
> + AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
> +fi
> +])
> diff --git a/gdb/config.in b/gdb/config.in
> index a7da88b92d7..157acd46c7c 100644
> --- a/gdb/config.in
> +++ b/gdb/config.in
> @@ -99,6 +99,9 @@
> /* define if the compiler supports basic C++11 syntax */
> #undef HAVE_CXX11
>
> +/* Define to 1 if debuginfod section downloading is supported. */
> +#undef HAVE_DEBUGINFOD_FIND_SECTION
> +
> /* Define to 1 if you have the declaration of `ADDR_NO_RANDOMIZE', and to 0 if
> you don't. */
> #undef HAVE_DECL_ADDR_NO_RANDOMIZE
> diff --git a/gdb/configure b/gdb/configure
> index 017ec05e4b7..338460c07bd 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -18349,7 +18349,7 @@ esac
>
> # Handle optional debuginfod support
>
> -# Handle optional debuginfod support
> +# Handle optional debuginfod support as well as optional section downloading support
>
> # Check whether --with-debuginfod was given.
> if test "${with_debuginfod+set}" = set; then :
> @@ -18365,6 +18365,106 @@ $as_echo "$with_debuginfod" >&6; }
>
> if test "x$with_debuginfod" != xno; then
>
> +pkg_failed=no
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libdebuginfod >= 0.188" >&5
> +$as_echo_n "checking for libdebuginfod >= 0.188... " >&6; }
> +
> +if test -n "$DEBUGINFOD_CFLAGS"; then
> + pkg_cv_DEBUGINFOD_CFLAGS="$DEBUGINFOD_CFLAGS"
> + elif test -n "$PKG_CONFIG"; then
> + if test -n "$PKG_CONFIG" && \
> + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libdebuginfod >= 0.188\""; } >&5
> + ($PKG_CONFIG --exists --print-errors "libdebuginfod >= 0.188") 2>&5
> + ac_status=$?
> + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> + test $ac_status = 0; }; then
> + pkg_cv_DEBUGINFOD_CFLAGS=`$PKG_CONFIG --cflags "libdebuginfod >= 0.188" 2>/dev/null`
> + test "x$?" != "x0" && pkg_failed=yes
> +else
> + pkg_failed=yes
> +fi
> + else
> + pkg_failed=untried
> +fi
> +if test -n "$DEBUGINFOD_LIBS"; then
> + pkg_cv_DEBUGINFOD_LIBS="$DEBUGINFOD_LIBS"
> + elif test -n "$PKG_CONFIG"; then
> + if test -n "$PKG_CONFIG" && \
> + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libdebuginfod >= 0.188\""; } >&5
> + ($PKG_CONFIG --exists --print-errors "libdebuginfod >= 0.188") 2>&5
> + ac_status=$?
> + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> + test $ac_status = 0; }; then
> + pkg_cv_DEBUGINFOD_LIBS=`$PKG_CONFIG --libs "libdebuginfod >= 0.188" 2>/dev/null`
> + test "x$?" != "x0" && pkg_failed=yes
> +else
> + pkg_failed=yes
> +fi
> + else
> + pkg_failed=untried
> +fi
> +
> +if test $pkg_failed = no; then
> + pkg_save_LDFLAGS="$LDFLAGS"
> + LDFLAGS="$LDFLAGS $pkg_cv_DEBUGINFOD_LIBS"
> + cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h. */
> +
> +int
> +main ()
> +{
> +
> + ;
> + return 0;
> +}
> +_ACEOF
> +if ac_fn_c_try_link "$LINENO"; then :
> +
> +else
> + pkg_failed=yes
> +fi
> +rm -f core conftest.err conftest.$ac_objext \
> + conftest$ac_exeext conftest.$ac_ext
> + LDFLAGS=$pkg_save_LDFLAGS
> +fi
> +
> +
> +
> +if test $pkg_failed = yes; then
> + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> +
> +if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then
> + _pkg_short_errors_supported=yes
> +else
> + _pkg_short_errors_supported=no
> +fi
> + if test $_pkg_short_errors_supported = yes; then
> + DEBUGINFOD_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "libdebuginfod >= 0.188" 2>&1`
> + else
> + DEBUGINFOD_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "libdebuginfod >= 0.188" 2>&1`
> + fi
> + # Put the nasty error message in config.log where it belongs
> + echo "$DEBUGINFOD_PKG_ERRORS" >&5
> +
> + { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: libdebuginfod is missing or some features may be unavailable." >&5
> +$as_echo "$as_me: WARNING: libdebuginfod is missing or some features may be unavailable." >&2;}
> +elif test $pkg_failed = untried; then
> + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> + { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: libdebuginfod is missing or some features may be unavailable." >&5
> +$as_echo "$as_me: WARNING: libdebuginfod is missing or some features may be unavailable." >&2;}
> +else
> + DEBUGINFOD_CFLAGS=$pkg_cv_DEBUGINFOD_CFLAGS
> + DEBUGINFOD_LIBS=$pkg_cv_DEBUGINFOD_LIBS
> + { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
> +$as_echo "yes" >&6; }
> +
> +$as_echo "#define HAVE_DEBUGINFOD_FIND_SECTION 1" >>confdefs.h
> +
> +fi
> +
> +
> pkg_failed=no
> { $as_echo "$as_me:${as_lineno-$LINENO}: checking for libdebuginfod >= 0.179" >&5
> $as_echo_n "checking for libdebuginfod >= 0.179... " >&6; }
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index fb43cd10d6c..499802bb4c9 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -342,7 +342,7 @@ case $host_os in
> esac
>
> # Handle optional debuginfod support
> -AC_DEBUGINFOD
> +AC_DEBUGINFOD_SECTION
>
> # Libunwind support for ia64.
> AC_ARG_WITH(libunwind-ia64,
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 04d254a1601..f909742362f 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -80,6 +80,15 @@ debuginfod_exec_query (const unsigned char *build_id,
> return scoped_fd (-ENOSYS);
> }
>
> +scoped_fd
> +debuginfod_section_query (const unsigned char *build_id,
> + int build_id_len,
> + const char *filename,
> + const char *section_name,
> + gdb::unique_xmalloc_ptr<char> *destname)
> +{
> + return scoped_fd (-ENOSYS);
> +}
> #define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
>
> #else
> @@ -388,6 +397,52 @@ debuginfod_exec_query (const unsigned char *build_id,
>
> return fd;
> }
> +
> +/* See debuginfod-support.h */
> +
> +scoped_fd
> +debuginfod_section_query (const unsigned char *build_id,
> + int build_id_len,
> + const char *filename,
> + const char *section_name,
> + gdb::unique_xmalloc_ptr<char> *destname)
> +{
> +#if !defined (HAVE_DEBUGINFOD_FIND_SECTION)
> + return scoped_fd (-ENOSYS);
> +#else
> +
> + if (!debuginfod_is_enabled ())
> + return scoped_fd (-ENOSYS);
> +
> + debuginfod_client *c = get_debuginfod_client ();
> +
> + if (c == nullptr)
> + return scoped_fd (-ENOMEM);
> +
> + char *dname = nullptr;
> + std::string desc = std::string ("section ") + section_name + " for";
> + user_data data (desc.c_str (), filename);
> +
> + debuginfod_set_user_data (c, &data);
> + gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
> + if (target_supports_terminal_ours ())
> + {
> + term_state.emplace ();
> + target_terminal::ours ();
> + }
> +
> + scoped_fd fd (debuginfod_find_section (c, build_id, build_id_len,
> + section_name, &dname));
> + debuginfod_set_user_data (c, nullptr);
> + print_outcome (data, fd.get ());
> +
> + if (fd.get () >= 0 && destname != nullptr)
> + destname->reset (dname);
Given all the uses of debuginfod_section_query pass a non-nullptr
destname, and the comment in debuginfod-support.h doesn't indicate that
destname is optional, I would suggest removing the 'destname != nullptr'
check here, and replace it with an extra line:
gdb_assert (destname != nullptr);
Thanks,
Andrew
> +
> + return fd;
> +#endif /* HAVE_DEBUGINFOD_FIND_SECTION */
> +}
> +
> #endif
>
> /* Set callback for "set debuginfod enabled". */
> diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> index 633600a79da..9701e3b4685 100644
> --- a/gdb/debuginfod-support.h
> +++ b/gdb/debuginfod-support.h
> @@ -81,4 +81,28 @@ extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
> const char *filename,
> gdb::unique_xmalloc_ptr<char>
> *destname);
> +
> +/* Query debuginfod servers for the binary contents of a ELF/DWARF section
> + from a file matching BUILD_ID. BUILD_ID can be given as a binary blob
> + or a null-terminated string. If given as a binary blob, BUILD_ID_LEN
> + should be the number of bytes. If given as a null-terminated string,
> + BUILD_ID_LEN should be 0.
> +
> + FILENAME should be the name or path associated with the file matching
> + BUILD_ID. It is used for printing messages to the user.
> +
> + SECTION_NAME should be the name of an ELF/DWARF section.
> +
> + If the file is successfully retrieved, return a file descriptor and store
> + the file's local path in DESTNAME. If unsuccessful, print an error message
> + and return a negative errno. If GDB is not built with debuginfod or
> + libdebuginfod does not support section queries, this function returns
> + -ENOSYS. */
> +
> +extern scoped_fd debuginfod_section_query (const unsigned char *build_id,
> + int build_id_len,
> + const char *filename,
> + const char *section_name,
> + gdb::unique_xmalloc_ptr<char>
> + *destname);
> #endif /* DEBUGINFOD_SUPPORT_H */
> --
> 2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled'
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
2023-02-27 19:54 ` Eli Zaretskii
@ 2023-05-24 9:31 ` Andrew Burgess
1 sibling, 0 replies; 25+ messages in thread
From: Andrew Burgess @ 2023-05-24 9:31 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches, gdb-patches; +Cc: Aaron Merey
Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 'set debuginfod enabled lazy' turns on debuginfod downloading like
> 'set debuginfod enabled on' but also enables ELF/DWARFs section
> downloading via debuginfod_section_query.
If I understand correctly, 'lazy' can still download the full files
(when needed), but might first download some of the sections (e.g. the
index), which might allow us to skip downloading the full files?
I'm not sure that adding the 'lazy' setting as another option is the
correct way to go. I'll explain more below...
>
> If support for debuginfod section queries was not found at configure
> time, 'set debuginfod enabled lazy' will print an error message
> indicating the missing support and default to 'set debuginfod enabled on'.
>
> Also update the help text and gdb.texinfo section for 'set debuginfod enabled'
> with information on the lazy setting.
> ---
> gdb/debuginfod-support.c | 20 +++++++++++++++++---
> gdb/doc/gdb.texinfo | 9 +++++++--
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index f909742362f..12025fcf0c0 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -34,12 +34,14 @@ static cmd_list_element *show_debuginfod_prefix_list;
> static const char debuginfod_on[] = "on";
> static const char debuginfod_off[] = "off";
> static const char debuginfod_ask[] = "ask";
> +static const char debuginfod_lazy[] = "lazy";
>
> static const char *debuginfod_enabled_enum[] =
> {
> debuginfod_on,
> debuginfod_off,
> debuginfod_ask,
> + debuginfod_lazy,
> nullptr
> };
>
> @@ -411,7 +413,7 @@ debuginfod_section_query (const unsigned char *build_id,
> return scoped_fd (-ENOSYS);
> #else
>
> - if (!debuginfod_is_enabled ())
> + if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
> return scoped_fd (-ENOSYS);
Because you check for debuginfod_lazy first, and due to the
short-circuit evaluation, then if debuginfod_enabled is set to 'on',
'off', or 'ask' we will always return -ENOSYS.
As the default is 'ask' this feature is off by default, which I think is
a shame.
Even if we could call debuginfod_is_enabled here, and it asked the user,
it would just set the value to 'on', which apparently still isn't good
enough.
I would suggest that this option (debuginfod_enabled) should remain as a
yes/no/ask choice. If the user selects 'yes', and section downloading
is supported by the current version of debuginfod then we should just do
that. I don't think this level of control is something that users will
normally care about -- they'll either be OK with downloading stuff (and
select 'on'), or they are against downloading stuff (and select 'off').
I doubt there are many users who say ... I'm OK downloading full debug
files, but not the individual sections.
You could possibly add a new setting to individually control downloading
the sections, though I'm so convinced that this isn't something a user
will care about that I would suggest this should be a maintenance
setting:
maintenance set debuginfod download-sections on
maintenance set debuginfod download-sections off
maintenance show debuginfod download-sections
We can always move maintenance commands into the user space later if
needed,
>
> debuginfod_client *c = get_debuginfod_client ();
> @@ -451,6 +453,14 @@ static void
> set_debuginfod_enabled (const char *value)
> {
> #if defined(HAVE_LIBDEBUGINFOD)
> +#if !defined(HAVE_DEBUGINFOD_FIND_SECTION)
> + if (value == debuginfod_lazy)
> + {
> + debuginfod_enabled = debuginfod_on;
> + error (_("Support for lazy downloading is not compiled into GDB. " \
> +"Defaulting to \"on\"."));
> + }
> +#endif
> debuginfod_enabled = value;
> #else
> /* Disabling debuginfod when gdb is not built with it is a no-op. */
> @@ -550,8 +560,12 @@ _initialize_debuginfod ()
> _("Set whether to use debuginfod."),
> _("Show whether to use debuginfod."),
> _("\
> -When on, enable the use of debuginfod to download missing debug info and\n\
> -source files."),
> +When set to \"on\", enable the use of debuginfod to download missing\n\
> +debug info and source files. \"off\" disables the use of debuginfod.\n\
> +When set to \"ask\", a prompt may ask whether to enable or disable\n\
> +debuginfod. When set to \"lazy\", debug info downloading will be\n\
> +deferred until it is required. GDB may also download components of\n\
> +debug info instead of entire files." ),
> set_debuginfod_enabled,
> get_debuginfod_enabled,
> show_debuginfod_enabled,
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index c1ca45521ea..ac0636e3115 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -48757,10 +48757,15 @@ debug info or source files. By default, @code{debuginfod enabled} is set to
> attempting to perform the next query. By default, @code{debuginfod enabled}
> is set to @code{ask} for interactive sessions.
>
> +@item set debuginfod enabled lazy
> +@value{GDBN} will attempt to defer downloading entire debug info files until
> +necessary. @value{GDBN} may instead download individual components of the
> +debug info files such as @code{.gdb_index}.
Given how the code is currently written I would expect some changes to
'on' to make it clear that some functionality is missing in this mode.
But if you take my advice then the docs would end up different anyway..
Thanks,
Andrew
> +
> @kindex show debuginfod enabled
> @item show debuginfod enabled
> -Display whether @code{debuginfod enabled} is set to @code{on}, @code{off} or
> -@code{ask}.
> +Display whether @code{debuginfod enabled} is set to @code{on}, @code{off},
> +@code{ask} or @code{lazy}.
>
> @kindex set debuginfod urls
> @cindex configure debuginfod URLs
> --
> 2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/7] gdb/debuginfod: disable pagination during downloads
2023-02-27 19:42 ` [PATCH 3/7] gdb/debuginfod: disable pagination during downloads Aaron Merey
2023-03-03 21:33 ` Tom Tromey
@ 2023-05-24 9:38 ` Andrew Burgess
2023-05-24 18:57 ` Aaron Merey
1 sibling, 1 reply; 25+ messages in thread
From: Andrew Burgess @ 2023-05-24 9:38 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches, gdb-patches; +Cc: Aaron Merey
Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Disable pagination during downloads in order to avoid inconvenient
> continue prompts "--Type <RET> for more, q to quit...".
>
> For more discussion on this issue see the following thread
> https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html
Is this patch critical to the new functionality in this series? If it's
not then you might be better spinning this patch into it's own thread.
I also echo Tom's query about why this change is needed. I haven't read
the thread you reference above -- I want to review the rest of this
series first -- but if there's good justification for this change in
that thread then it would be nice to see that in the commit message, the
commit message is carried with the code, but the mail archive might
disappear in the future.
> ---
> gdb/debuginfod-support.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 12025fcf0c0..f4969e94b0a 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -292,6 +292,9 @@ debuginfod_source_query (const unsigned char *build_id,
> const char *srcpath,
> gdb::unique_xmalloc_ptr<char> *destname)
> {
> + scoped_restore save_count_lines_printed
> + = make_scoped_restore (&pagination_enabled, false);
Given you have several uses of the same pattern, it might be nice to add
a new 'scoped_restore_pagination' class, we already have lots of
scoped_restore_XXX specialisation classes.
Thanks,
Andrew
> +
> if (!debuginfod_is_enabled ())
> return scoped_fd (-ENOSYS);
>
> @@ -333,6 +336,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> const char *filename,
> gdb::unique_xmalloc_ptr<char> *destname)
> {
> + scoped_restore save_count_lines_printed
> + = make_scoped_restore (&pagination_enabled, false);
> +
> if (!debuginfod_is_enabled ())
> return scoped_fd (-ENOSYS);
>
> @@ -371,6 +377,9 @@ debuginfod_exec_query (const unsigned char *build_id,
> const char *filename,
> gdb::unique_xmalloc_ptr<char> *destname)
> {
> + scoped_restore save_count_lines_printed
> + = make_scoped_restore (&pagination_enabled, false);
> +
> if (!debuginfod_is_enabled ())
> return scoped_fd (-ENOSYS);
>
> @@ -412,6 +421,8 @@ debuginfod_section_query (const unsigned char *build_id,
> #if !defined (HAVE_DEBUGINFOD_FIND_SECTION)
> return scoped_fd (-ENOSYS);
> #else
> + scoped_restore save_count_lines_printed
> + = make_scoped_restore (&pagination_enabled, false);
>
> if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
> return scoped_fd (-ENOSYS);
> --
> 2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests
2023-02-27 19:42 ` [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests Aaron Merey
2023-05-02 15:48 ` Andrew Burgess
@ 2023-05-24 10:12 ` Andrew Burgess
1 sibling, 0 replies; 25+ messages in thread
From: Andrew Burgess @ 2023-05-24 10:12 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches, gdb-patches; +Cc: Aaron Merey
Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Add some tests for 'set debuginfod enabled lazy', .gdb_index downloading
> and deferred debuginfo downloading.
> ---
> .../gdb.debuginfod/fetch_src_and_symbols.exp | 58 +++++++++++++++++++
> gdb/testsuite/gdb.debuginfod/libsection.c | 24 ++++++++
> gdb/testsuite/gdb.debuginfod/section.c | 28 +++++++++
> 3 files changed, 110 insertions(+)
> create mode 100644 gdb/testsuite/gdb.debuginfod/libsection.c
> create mode 100644 gdb/testsuite/gdb.debuginfod/section.c
>
> diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> index 8158c5c3cc6..fc7e4b685dc 100644
> --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> @@ -42,6 +42,23 @@ if { [gdb_compile "$sourcetmp" "${binfile}2" executable {debug build-id}] != ""
> return -1
> }
>
> +set sectfile "section"
> +set sectsrc $srcdir/$subdir/section.c
> +set sectexec [standard_output_file $sectfile]
> +
> +set libfile "libsection"
> +set libsrc $srcdir/$subdir/$libfile.c
> +set lib_sl [standard_output_file $libfile.sl]
> +
> +set lib_opts [list debug build-id ]
> +set exec_opts [list debug build-id shlib=$lib_sl]
> +
> +if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
> + || [gdb_compile $sectsrc $sectexec executable $exec_opts] != "" } {
> + untested "failed to compile"
> + return -1
> +}
I'm really not a fan of either having a separate "add tests" commit,
this makes it so much harder in the future to track down which test
corresponds to a particular change. Nor am I a fan of having this one
test script fetch_src_and_symbols.exp, into which all debuginfod tests
just keep getting added.
I already made an attempt to factor out some of the support
functionality into lib/debuginfod-support.exp, and it would be great to
see more functionality added in there so that it becomes easier to add
new debuginfod tests.
I mean, it's fine, if the new test makes use of an existing binary and
you're just running some extra commands as part of an existing test
script, but for me the red flag is that you have a whole new source file
which you compile, that indicates (to me) that this should be a new test
script.
Also, this approach of adding tests in a single commit makes it really
easy to miss testing some of the changes -- for example, I see no use of
backtrace anywhere in fetch_src_and_symbols.exp, so that patch isn't
being tested at all.
> +
> # Write some assembly that just has a .gnu_debugaltlink section.
> # Copied from testsuite/gdb.dwarf2/dwzbuildid.exp.
> proc write_just_debugaltlink {filename dwzname buildid} {
> @@ -94,6 +111,7 @@ proc write_dwarf_file {filename buildid {value 99}} {
> }
>
> set corefile [standard_output_file "corefile"]
> +set lazy_support -1
>
> # Setup the global variable DEBUGDIR as a directory containing the
> # debug information for the test executable.
> @@ -103,6 +121,8 @@ set corefile [standard_output_file "corefile"]
> # running.
> proc_with_prefix no_url { } {
> global binfile outputdir debugdir
> + global sectexec lib_sl libfile lazy_support
> + global gdb_prompt
>
> setenv DEBUGINFOD_URLS ""
>
> @@ -119,11 +139,18 @@ proc_with_prefix no_url { } {
> return -1
> }
>
> + if { [gdb_gnu_strip_debug $lib_sl ""] != 0} {
> + fail "strip shlib debuginfo"
> + return -1
> + }
> +
> set debugdir [standard_output_file "debug"]
> set debuginfo [standard_output_file "fetch_src_and_symbols.debug"]
> + set debuginfo_shlib [standard_output_file $libfile.sl.debug]
>
> file mkdir $debugdir
> file rename -force $debuginfo $debugdir
> + file rename -force $debuginfo_shlib $debugdir
>
> # Test that GDB cannot find symbols without debuginfod.
> clean_restart $binfile
> @@ -171,6 +198,25 @@ proc_with_prefix no_url { } {
>
> clean_restart
> gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
> + clean_restart
> +
> + # Check for lazy downloading support.
> + gdb_test_multiple "set debuginfod enabled lazy" "check for lazy" {
> + -re ".*lazy downloading is not compiled into GDB.*\n.*${gdb_prompt} $" {
> + set lazy_support 0
> + }
> + -re ".*${gdb_prompt} $" {
> + set lazy_support 1
> + }
> + }
As one example, this would be a great candidate for moving into
lib/debuginfod-support.exp.
Thanks,
Andrew
> +
> + if {$lazy_support == 1} {
> + gdb_test "file $sectexec" "" "file [file tail $sectexec] file no url"
> + gdb_test "start" "" "file [file tail $sectexec] start no url"
> + gdb_test "info sharedlibrary" ".*Yes \\(\\*\\).*libsection.*" "lib no debug"
> + } else {
> + untested "lazy support no_url"
> + }
> }
>
> # Test that GDB prints the debuginfod URLs when loading files. URLS
> @@ -208,6 +254,7 @@ proc disable_delete_breakpoints {} {
> # expected debug information.
> proc_with_prefix local_url { } {
> global binfile outputdir debugdir db
> + global sectexec lib_sl libfile lazy_support
>
> set url [start_debuginfod $db $debugdir]
> if { $url == "" } {
> @@ -256,6 +303,17 @@ proc_with_prefix local_url { } {
> "file [file tail ${binfile}_alt.o]" \
> $enable_debuginfod_question "y"
>
> + if {$lazy_support == 1} {
> + # GDB should now download .gdb_index.
> + clean_restart
> + gdb_test "set debuginfod enabled lazy" "" "set lazy urls"
> + gdb_test "file $sectexec" "" "file [file tail $sectexec] file urls"
> + set res ".*Download.*\.gdb_index.*libsection.*\r\nDownload.*debug info.*libsection.*"
> + gdb_test "start $sectexec" $res "file [file tail $sectexec] start urls"
> + } else {
> + untested "lazy support urls"
> + }
> +
> # Configure debuginfod with commands.
> unsetenv DEBUGINFOD_URLS
> clean_restart
> diff --git a/gdb/testsuite/gdb.debuginfod/libsection.c b/gdb/testsuite/gdb.debuginfod/libsection.c
> new file mode 100644
> index 00000000000..510f9205282
> --- /dev/null
> +++ b/gdb/testsuite/gdb.debuginfod/libsection.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2020-2023 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +
> +void
> +libsection_test ()
> +{
> + printf ("In libsection\n");
> +}
> diff --git a/gdb/testsuite/gdb.debuginfod/section.c b/gdb/testsuite/gdb.debuginfod/section.c
> new file mode 100644
> index 00000000000..3598896c1bc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.debuginfod/section.c
> @@ -0,0 +1,28 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2020-2023 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <fcntl.h>
> +
> +extern void libsection_test ();
> +
> +int
> +main()
> +{
> + (void) open ("", 0);
> + libsection_test ();
> + return 0;
> +}
> --
> 2.39.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/7] gdb/debuginfod: disable pagination during downloads
2023-05-24 9:38 ` Andrew Burgess
@ 2023-05-24 18:57 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2023-05-24 18:57 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Aaron Merey via Gdb-patches
Hi Andrew,
On Wed, May 24, 2023 at 5:38 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> > Disable pagination during downloads in order to avoid inconvenient
> > continue prompts "--Type <RET> for more, q to quit...".
> >
> > For more discussion on this issue see the following thread
> > https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html
>
> Is this patch critical to the new functionality in this series? If it's
> not then you might be better spinning this patch into it's own thread.
I think we can just drop this patch. If the user doesn't want pagination
prompts they already have a way to disable it.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2023-05-24 18:57 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-27 19:42 [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Aaron Merey
2023-02-27 19:42 ` [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled' Aaron Merey
2023-02-27 19:54 ` Eli Zaretskii
2023-05-24 9:31 ` Andrew Burgess
2023-02-27 19:42 ` [PATCH 3/7] gdb/debuginfod: disable pagination during downloads Aaron Merey
2023-03-03 21:33 ` Tom Tromey
2023-03-06 23:07 ` Aaron Merey
2023-05-24 9:38 ` Andrew Burgess
2023-05-24 18:57 ` Aaron Merey
2023-02-27 19:42 ` [PATCH 4/7] gdb/ui-file: Add newline tracking Aaron Merey
2023-03-07 19:33 ` Tom Tromey
2023-03-07 20:30 ` Aaron Merey
2023-03-07 20:47 ` Tom Tromey
2023-02-27 19:42 ` [PATCH 5/7] gdb/debuginfod: Support on-demand debuginfo downloading Aaron Merey
2023-03-07 20:20 ` Tom Tromey
2023-03-09 0:22 ` Aaron Merey
2023-02-27 19:42 ` [PATCH 6/7] gdb/testsuite/gdb.debuginfod: Add lazy downloading tests Aaron Merey
2023-05-02 15:48 ` Andrew Burgess
2023-05-02 16:24 ` Aaron Merey
2023-05-24 10:12 ` Andrew Burgess
2023-02-27 19:42 ` [PATCH 7/7] gdb/debuginfod: Add .debug_line downloading Aaron Merey
2023-03-07 20:36 ` Tom Tromey
2023-03-09 0:26 ` Aaron Merey
2023-02-28 11:11 ` [PATCH 1/7] gdb/debuginfod: Add debuginfod_section_query Alexandra Petlanova Hajkova
2023-05-24 9:01 ` Andrew Burgess
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).