diff mbox series

[3/5] gdb: refactor objfile::find_and_add_separate_symbol_file

Message ID 108c51107d11eae41fd77d54d73613569875c4c7.1697626088.git.aburgess@redhat.com
State New
Headers
Series New Python hook for missing debug information |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Andrew Burgess Oct. 18, 2023, 10:53 a.m. UTC 
  This is purely a refactoring commit.

This commit splits objfile::find_and_add_separate_symbol_file into
some separate helper functions.  My hope is that the steps for looking
up separate debug information are now clearer.

In a later commit I'm going to extend
objfile::find_and_add_separate_symbol_file, with some additional
logic, so starting with a simpler function will make the following
changes easier.

There should be no user visible changes after this commit.
---
 gdb/symfile-debug.c | 127 ++++++++++++++++++++++++++++----------------
 1 file changed, 82 insertions(+), 45 deletions(-)

Comments

Tom Tromey Oct. 20, 2023, 5:50 p.m. UTC | #1 
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> This is purely a refactoring commit.
Andrew> This commit splits objfile::find_and_add_separate_symbol_file into
Andrew> some separate helper functions.  My hope is that the steps for looking
Andrew> up separate debug information are now clearer.

Andrew> In a later commit I'm going to extend
Andrew> objfile::find_and_add_separate_symbol_file, with some additional
Andrew> logic, so starting with a simpler function will make the following
Andrew> changes easier.

Andrew> There should be no user visible changes after this commit.

Andrew> +static std::pair<gdb_bfd_ref_ptr, std::string>
Andrew> +simple_find_and_open_separate_symbol_file
...
Andrew> +  result = simple_find_and_open_separate_symbol_file
Andrew> +    (this, find_separate_debug_file_by_buildid, &warnings);
... 
Andrew> +  if (result.first == nullptr)

If this happens to go in after the C++17 series, this code could be
cleaned up a little using structured bindings.

Tom
Andrew Burgess Oct. 24, 2023, 12:03 p.m. UTC | #2 
Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> This is purely a refactoring commit.
> Andrew> This commit splits objfile::find_and_add_separate_symbol_file into
> Andrew> some separate helper functions.  My hope is that the steps for looking
> Andrew> up separate debug information are now clearer.
>
> Andrew> In a later commit I'm going to extend
> Andrew> objfile::find_and_add_separate_symbol_file, with some additional
> Andrew> logic, so starting with a simpler function will make the following
> Andrew> changes easier.
>
> Andrew> There should be no user visible changes after this commit.
>
> Andrew> +static std::pair<gdb_bfd_ref_ptr, std::string>
> Andrew> +simple_find_and_open_separate_symbol_file
> ...
> Andrew> +  result = simple_find_and_open_separate_symbol_file
> Andrew> +    (this, find_separate_debug_file_by_buildid, &warnings);
> ... 
> Andrew> +  if (result.first == nullptr)
>
> If this happens to go in after the C++17 series, this code could be
> cleaned up a little using structured bindings.

I'll keep that in mind.

Thanks,
Andrew
Andrew Burgess Nov. 8, 2023, 3:22 p.m. UTC | #3 
Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> This is purely a refactoring commit.
> Andrew> This commit splits objfile::find_and_add_separate_symbol_file into
> Andrew> some separate helper functions.  My hope is that the steps for looking
> Andrew> up separate debug information are now clearer.
>
> Andrew> In a later commit I'm going to extend
> Andrew> objfile::find_and_add_separate_symbol_file, with some additional
> Andrew> logic, so starting with a simpler function will make the following
> Andrew> changes easier.
>
> Andrew> There should be no user visible changes after this commit.
>
> Andrew> +static std::pair<gdb_bfd_ref_ptr, std::string>
> Andrew> +simple_find_and_open_separate_symbol_file
> ...
> Andrew> +  result = simple_find_and_open_separate_symbol_file
> Andrew> +    (this, find_separate_debug_file_by_buildid, &warnings);
> ... 
> Andrew> +  if (result.first == nullptr)
>
> If this happens to go in after the C++17 series, this code could be
> cleaned up a little using structured bindings.

I took a look at making use of structured bindings here, and I don't
think it offers any improvements.

I can write something like this:

  auto [debug_bfd, filename] = some_function ();

But, I can't write this:

  [debug_bfd, filename] = some_function ();

Which means if, what I really want to write is:

  [debug_bfd, filename] = some_function ();
  if (debug_bfd == nullptr)
    [debug_bfd, filename] = some_other_function ();
  if (debug_bfd == nullptr)
    [debug_bfd, filename] = yet_other_function ();

then every structured binding needs to write into a new set of local
variable, which makes the logic much more complex.

However, while researching this, I did discover std::tie, which seems to
do a similar thing as structured bindings, but without the variable
reuse limitation, and removing the .first, .second does make the code
more readable.  I'll post a V2 shortly, which will include this change.

Thanks,
Andrew
diff mbox series

Patch

diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index 961ae2327f7..0a4499320c7 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -560,65 +560,102 @@  objfile::require_partial_symbols (bool verbose)
     }
 }
 
+/* Call LOOKUP_FUNC to find the filename of a file containing the separate
+   debug information matching OBJFILE.  If LOOKUP_FUNC does return a
+   filename then open this file and return a std::pair containing the
+   gdb_bfd_ref_ptr of the open file and the filename returned by
+   LOOKUP_FUNC, otherwise this function returns an empty pair; the first
+   item will be nullptr, and the second will be an empty string.
+
+   Any warnings generated by this function, or by calling LOOKUP_FUNC are
+   placed into WARNINGS, these warnings are only displayed to the user if
+   GDB is unable to find the separate debug information via any route.  */
+static std::pair<gdb_bfd_ref_ptr, std::string>
+simple_find_and_open_separate_symbol_file
+  (struct objfile *objfile,
+   std::string (*lookup_func) (struct objfile *, deferred_warnings *),
+   deferred_warnings *warnings)
+{
+  std::string filename = lookup_func (objfile, warnings);
+
+  if (!filename.empty ())
+    {
+      gdb_bfd_ref_ptr symfile_bfd
+	= symfile_bfd_open_no_error (filename.c_str ());
+      if (symfile_bfd != nullptr)
+	return { symfile_bfd, filename };
+    }
+
+  return {};
+}
+
+/* Lookup separate debug information for OBJFILE via debuginfod.  If
+   successful the debug information will be have been downloaded into the
+   debuginfod cache and this function will return a std::pair containing a
+   gdb_bfd_ref_ptr of the open debug information file and the filename for
+   the file within the debuginfod cache.  If no debug information could be
+   found then this function returns an empty pair; the first item will be
+   nullptr, and the second will be an empty string.  */
+
+static std::pair<gdb_bfd_ref_ptr, std::string>
+debuginfod_find_and_open_separate_symbol_file (struct objfile * objfile)
+{
+  const struct bfd_build_id *build_id
+    = build_id_bfd_get (objfile->obfd.get ());
+  const char *filename = bfd_get_filename (objfile->obfd.get ());
+
+  if (build_id != nullptr)
+    {
+      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)
+	{
+	  /* File successfully retrieved from server.  */
+	  gdb_bfd_ref_ptr debug_bfd
+	    (symfile_bfd_open_no_error (symfile_path.get ()));
+
+	  if (debug_bfd != nullptr
+	      && build_id_verify (debug_bfd.get (),
+				  build_id->size, build_id->data))
+	    return { debug_bfd, std::string (symfile_path.get ()) };
+	}
+    }
+
+  return {};
+}
+
 /* See objfiles.h.  */
 
 bool
 objfile::find_and_add_separate_symbol_file (symfile_add_flags symfile_flags)
 {
-  bool has_dwarf2 = true;
-
+  bool has_dwarf2 = false;
   deferred_warnings warnings;
 
-  std::string debugfile
-    = find_separate_debug_file_by_buildid (this, &warnings);
+  std::pair<gdb_bfd_ref_ptr, std::string> result;
 
-  if (debugfile.empty ())
-    debugfile = find_separate_debug_file_by_debuglink (this, &warnings);
-
-  if (!debugfile.empty ())
-    {
-      gdb_bfd_ref_ptr debug_bfd
-	(symfile_bfd_open_no_error (debugfile.c_str ()));
+  result = simple_find_and_open_separate_symbol_file
+    (this, find_separate_debug_file_by_buildid, &warnings);
 
-      if (debug_bfd != nullptr)
-	symbol_file_add_separate (debug_bfd, debugfile.c_str (),
-				  symfile_flags, this);
-    }
-  else
-    {
-      has_dwarf2 = false;
-      const struct bfd_build_id *build_id
-	= build_id_bfd_get (this->obfd.get ());
-      const char *filename = bfd_get_filename (this->obfd.get ());
+  if (result.first == nullptr)
+    result = simple_find_and_open_separate_symbol_file
+      (this, find_separate_debug_file_by_debuglink, &warnings);
 
-      if (build_id != nullptr)
-	{
-	  gdb::unique_xmalloc_ptr<char> symfile_path;
-	  scoped_fd fd (debuginfod_debuginfo_query (build_id->data,
-						    build_id->size,
-						    filename,
-						    &symfile_path));
+  if (result.first == nullptr)
+    result = debuginfod_find_and_open_separate_symbol_file (this);
 
-	  if (fd.get () >= 0)
-	    {
-	      /* File successfully retrieved from server.  */
-	      gdb_bfd_ref_ptr debug_bfd
-		(symfile_bfd_open_no_error (symfile_path.get ()));
-
-	      if (debug_bfd != nullptr
-		  && build_id_verify (debug_bfd.get (), build_id->size,
-				      build_id->data))
-		{
-		  symbol_file_add_separate (debug_bfd, symfile_path.get (),
-					    symfile_flags, this);
-		  has_dwarf2 = true;
-		}
-	    }
-	}
+  if (result.first != nullptr)
+    {
+      symbol_file_add_separate (result.first, result.second.c_str (),
+				symfile_flags, this);
+      has_dwarf2 = true;
     }
+
   /* If all the methods to collect the debuginfo failed, print the
      warnings, this is a no-op if there are no warnings.  */
-  if (debugfile.empty () && !has_dwarf2)
+  if (!has_dwarf2)
     warnings.emit ();
 
   return has_dwarf2;