[PATCH] gdb/solib: pass lm_info, original_name and name to solib constructor

[PATCH] gdb/solib: pass lm_info, original_name and name to solib constructor

[Simon Marchi]

These values are always known at construction time, so pass them to the
constructor.

Add constructors to some lm_info_* types when it's easy and it makes
things cleaner.

Change-Id: Ie2d751be276470c10c81792a93bf3ddafbcd4c33
---
 gdb/solib-aix.c    |  5 +----
 gdb/solib-darwin.c | 17 +++++++----------
 gdb/solib-dsbt.c   | 21 +++++++++++----------
 gdb/solib-frv.c    | 33 ++++++++++++++++-----------------
 gdb/solib-rocm.c   |  9 ++-------
 gdb/solib-svr4.c   | 16 ++++------------
 gdb/solib-target.c |  9 +--------
 gdb/solib.h        |  6 ++++++
 8 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 4af83de4d371..733c74dc9c3b 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -480,10 +480,7 @@ solib_aix_current_sos ()
 	}
 
       /* Add it to the list.  */
-      auto &new_solib = sos.emplace_back ();
-      new_solib.original_name = so_name;
-      new_solib.name = so_name;
-      new_solib.lm_info = std::make_unique<lm_info_aix> (info);
+      sos.emplace_back (std::make_unique<lm_info_aix> (info), so_name, so_name);
     }
 
   return sos;
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index 4003c77748ca..81047c38a9d4 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -137,8 +137,12 @@ darwin_load_image_infos (struct darwin_info *info)
 
 struct lm_info_darwin final : public lm_info
 {
+  explicit lm_info_darwin (CORE_ADDR lm_addr)
+    : lm_addr (lm_addr)
+  {}
+
   /* The target location of lm.  */
-  CORE_ADDR lm_addr = 0;
+  CORE_ADDR lm_addr;
 };
 
 /* Lookup the value for a specific symbol.  */
@@ -250,15 +254,8 @@ darwin_current_sos ()
 	break;
 
       /* Create and fill the new struct solib element.  */
-      auto &newobj = sos.emplace_back ();
-
-      auto li = std::make_unique<lm_info_darwin> ();
-
-      newobj.name = file_path.get ();
-      newobj.original_name = newobj.name;
-      li->lm_addr = load_addr;
-
-      newobj.lm_info = std::move (li);
+      sos.emplace_back (std::make_unique<lm_info_darwin> (load_addr),
+			file_path.get (), file_path.get ());
     }
 
   return sos;
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 2b3153692805..1f03e2b1edb1 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -124,13 +124,19 @@ struct dbst_ext_link_map
 
 struct lm_info_dsbt final : public lm_info
 {
+  explicit lm_info_dsbt (int_elf32_dsbt_loadmap *map)
+    : map (map)
+  {}
+
+  DISABLE_COPY_AND_ASSIGN (lm_info_dsbt);
+
   ~lm_info_dsbt ()
   {
     xfree (this->map);
   }
 
   /* The loadmap, digested into an easier to use form.  */
-  int_elf32_dsbt_loadmap *map = NULL;
+  int_elf32_dsbt_loadmap *map;
 };
 
 /* Per pspace dsbt specific data.  */
@@ -584,9 +590,6 @@ dsbt_current_sos (void)
 	      break;
 	    }
 
-	  auto &sop = sos.emplace_back ();
-	  auto li = std::make_unique<lm_info_dsbt> ();
-	  li->map = loadmap;
 	  /* Fetch the name.  */
 	  addr = extract_unsigned_integer (lm_buf.l_name,
 					   sizeof (lm_buf.l_name),
@@ -601,12 +604,11 @@ dsbt_current_sos (void)
 	      if (solib_dsbt_debug)
 		gdb_printf (gdb_stdlog, "current_sos: name = %s\n",
 			    name_buf.get ());
-
-	      sop.name = name_buf.get ();
-	      sop.original_name = sop.name;
 	    }
 
-	  sop.lm_info = std::move (li);
+	  sos.emplace_back (std::make_unique<lm_info_dsbt> (loadmap),
+			    name_buf != nullptr ? name_buf.get () : "",
+			    name_buf != nullptr ? name_buf.get () : "");
 	}
       else
 	{
@@ -789,8 +791,7 @@ dsbt_relocate_main_executable (void)
   ldm = info->exec_loadmap;
 
   delete info->main_executable_lm_info;
-  info->main_executable_lm_info = new lm_info_dsbt;
-  info->main_executable_lm_info->map = ldm;
+  info->main_executable_lm_info = new lm_info_dsbt (ldm);
 
   objfile *objf = current_program_space->symfile_object_file;
   section_offsets new_offsets (objf->section_offsets.size ());
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index ceef72208a1b..2af4dd9f10ab 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -197,6 +197,13 @@ struct ext_link_map
 
 struct lm_info_frv final : public lm_info
 {
+  lm_info_frv (int_elf32_fdpic_loadmap *map, CORE_ADDR got_value,
+	       CORE_ADDR lm_addr)
+    : map (map), got_value (got_value), lm_addr (lm_addr)
+  {}
+
+  DISABLE_COPY_AND_ASSIGN (lm_info_frv);
+
   ~lm_info_frv ()
   {
     xfree (this->map);
@@ -205,11 +212,11 @@ struct lm_info_frv final : public lm_info
   }
 
   /* The loadmap, digested into an easier to use form.  */
-  int_elf32_fdpic_loadmap *map = NULL;
+  int_elf32_fdpic_loadmap *map;
   /* The GOT address for this link map entry.  */
-  CORE_ADDR got_value = 0;
+  CORE_ADDR got_value;
   /* The link map address, needed for frv_fetch_objfile_link_map().  */
-  CORE_ADDR lm_addr = 0;
+  CORE_ADDR lm_addr;
 
   /* Cached dynamic symbol table and dynamic relocs initialized and
      used only by find_canonical_descriptor_in_load_object().
@@ -367,13 +374,6 @@ frv_current_sos ()
 	      break;
 	    }
 
-	  auto &sop = sos.emplace_back ();
-	  auto li = std::make_unique<lm_info_frv> ();
-	  li->map = loadmap;
-	  li->got_value = got_addr;
-	  li->lm_addr = lm_addr;
-	  sop.lm_info = std::move (li);
-
 	  /* Fetch the name.  */
 	  addr = extract_unsigned_integer (lm_buf.l_name,
 					   sizeof (lm_buf.l_name),
@@ -385,11 +385,11 @@ frv_current_sos ()
 
 	  if (name_buf == nullptr)
 	    warning (_("Can't read pathname for link map entry."));
-	  else
-	    {
-	      sop.name = name_buf.get ();
-	      sop.original_name = sop.name;
-	    }
+
+	  sos.emplace_back (std::make_unique<lm_info_frv> (loadmap, got_addr,
+							   lm_addr),
+			    name_buf != nullptr ? name_buf.get () : "",
+			    name_buf != nullptr ? name_buf.get () : "");
 	}
       else
 	{
@@ -725,8 +725,7 @@ frv_relocate_main_executable (void)
     error (_("Unable to load the executable's loadmap."));
 
   delete main_executable_lm_info;
-  main_executable_lm_info = new lm_info_frv;
-  main_executable_lm_info->map = ldm;
+  main_executable_lm_info = new lm_info_frv (ldm, 0, 0);
 
   objfile *objf = current_program_space->symfile_object_file;
   section_offsets new_offsets (objf->section_offsets.size ());
diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 92fb91563f00..ef01720ecf4b 100644
--- a/gdb/solib-rocm.c
+++ b/gdb/solib-rocm.c
@@ -209,13 +209,8 @@ solibs_from_rocm_sos (const std::vector<rocm_so> &sos)
   owning_intrusive_list<solib> dst;
 
   for (const rocm_so &so : sos)
-    {
-      auto &newobj = dst.emplace_back ();
-
-      newobj.lm_info = std::make_unique<lm_info_svr4> (*so.lm_info);
-      newobj.name = so.name;
-      newobj.original_name = so.unique_name;
-    }
+    dst.emplace_back (std::make_unique<lm_info_svr4> (*so.lm_info),
+		      so.unique_name, so.name);
 
   return dst;
 }
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 5a8b08edacda..44cad5ed2782 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1053,13 +1053,8 @@ solib_from_svr4_sos (const std::vector<svr4_so> &sos)
   owning_intrusive_list<solib> dst;
 
   for (const svr4_so &so : sos)
-    {
-      auto &newobj = dst.emplace_back ();
-
-      newobj.name = so.name;
-      newobj.original_name = so.name;
-      newobj.lm_info = std::make_unique<lm_info_svr4> (*so.lm_info);
-    }
+    dst.emplace_back (std::make_unique<lm_info_svr4> (*so.lm_info), so.name,
+		      so.name);
 
   return dst;
 }
@@ -1254,11 +1249,8 @@ svr4_default_sos (svr4_info *info)
   li->l_addr_p = 1;
 
   owning_intrusive_list<solib> sos;
-  auto &newobj = sos.emplace_back ();
-
-  newobj.lm_info = std::move (li);
-  newobj.name = info->debug_loader_name;
-  newobj.original_name = newobj.name;
+  sos.emplace_back (std::move (li), info->debug_loader_name,
+		    info->debug_loader_name);
 
   return sos;
 }
diff --git a/gdb/solib-target.c b/gdb/solib-target.c
index 68dc3cc92c0a..dce90a5f8c8e 100644
--- a/gdb/solib-target.c
+++ b/gdb/solib-target.c
@@ -244,14 +244,7 @@ solib_target_current_sos (void)
 
   /* Build a struct solib for each entry on the list.  */
   for (lm_info_target_up &info : library_list)
-    {
-      auto &new_solib = sos.emplace_back ();
-
-      /* We don't need a copy of the name in INFO anymore.  */
-      new_solib.name = std::move (info->name);
-      new_solib.original_name = new_solib.name;
-      new_solib.lm_info = std::move (info);
-    }
+    sos.emplace_back (std::move (info), info->name, info->name);
 
   return sos;
 }
diff --git a/gdb/solib.h b/gdb/solib.h
index f5922aa5f5d6..2bcfb63a82a4 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -56,6 +56,12 @@ using lm_info_up = std::unique_ptr<lm_info>;
 
 struct solib : intrusive_list_node<solib>
 {
+  solib (lm_info_up lm_info, std::string original_name, std::string name)
+    : lm_info (std::move (lm_info)),
+      original_name (std::move (original_name)),
+      name (std::move (name))
+  {}
+
   /* Free symbol-file related contents of SO and reset for possible reloading
      of SO.  If we have opened a BFD for SO, close it.  If we have placed SO's
      sections in some target's section table, the caller is responsible for

base-commit: 0b5023cc71d3af8b18e10e6599a3f9381bc15265
-- 
2.49.0

Re: [PATCH] gdb/solib: pass lm_info, original_name and name to solib constructor

[Guinevere Larsen]

On 6/3/25 12:32 PM, Simon Marchi wrote:
> diff --git a/gdb/solib-target.c b/gdb/solib-target.c
> index 68dc3cc92c0a..dce90a5f8c8e 100644
> --- a/gdb/solib-target.c
> +++ b/gdb/solib-target.c
> @@ -244,14 +244,7 @@ solib_target_current_sos (void)
>   
>     /* Build a struct solib for each entry on the list.  */
>     for (lm_info_target_up &info : library_list)
> -    {
> -      auto &new_solib = sos.emplace_back ();
> -
> -      /* We don't need a copy of the name in INFO anymore.  */
> -      new_solib.name = std::move (info->name);
> -      new_solib.original_name = new_solib.name;
> -      new_solib.lm_info = std::move (info);
> -    }
> +    sos.emplace_back (std::move (info), info->name, info->name);
This is a bad idea. I double checked with c++ folk and while no 
compilers warn about this, if the compiler evaluates things right to 
left (like gcc) this works, but if they evaluate left to right (like 
clang), then info->name will be de-referencing null and segfault. This 
might also depend on ABI, but the end result is the same: fragile at 
best. we'll need something that saves or moves info->name to pass it to 
the emplace_back constructor
>   
>     return sos;
>   }


-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH] gdb/solib: pass lm_info, original_name and name to solib constructor

[Simon Marchi]

On 2025-06-03 19:07, Guinevere Larsen wrote:
>> +    sos.emplace_back (std::move (info), info->name, info->name);
> This is a bad idea. I double checked with c++ folk and while no
> compilers warn about this, if the compiler evaluates things right to
> left (like gcc) this works, but if they evaluate left to right (like
> clang), then info->name will be de-referencing null and segfault. This
> might also depend on ABI, but the end result is the same: fragile at
> best. we'll need something that saves or moves info->name to pass it
> to the emplace_back constructor

Wow, good catch, thanks.  Actually, with C++17, I think the order of
evaluation has been specified from left to right.  So, I think it should
crash regardless of the compiler.  I don't think that the testsuite
exercises that code though on Linux.

A more long-term fix would be to move the name out of lm_info_target (it
doesn't belong there anyway).  But for now I'll change it like so in my
patch:


  for (lm_info_target_up &info : library_list)
    {
      /* Move NAME to a local variable to avoid reading INFO->NAME after having
	 moved info.  */
      std::string name = std::move (info->name);
      sos.emplace_back (std::move (info), name, name);
    }

It seems to be the only occurrence of this problem in the patch.

Simon