[PATCH 0/3] Better type safety for bcache

[PATCH 0/3] Better type safety for bcache

[Tom Tromey]

This series changes bcache to only allow trivially-copyable types.
Some other minor API improvements are also made.

Regression tested on x86-64 Fedora 38

---
Tom Tromey (3):
      Some constification in psymtab
      Make bcache more type-safe
      Remove a couple unnecessary casts

 gdb/bcache.h   | 25 +++++++++++++++++++++-
 gdb/gdbtypes.c |  2 +-
 gdb/macrotab.c | 12 +++++------
 gdb/objfiles.h |  4 ++--
 gdb/psymtab.c  | 65 +++++++++++++++++++++++++++-------------------------------
 gdb/psymtab.h  |  4 ++--
 6 files changed, 65 insertions(+), 47 deletions(-)
---
base-commit: d5e9331b6ba459fa211229e56d63b11daaab3330
change-id: 20240320-bcache-type-408f5e90ca2a

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/3] Some constification in psymtab

[Tom Tromey]

This patch changes some spots in psymtab.[ch] to use 'const'.  This is
just preparation for a subsequent patch.  Note that psymbols are
conceptually const, and since they were changed to be
objfile-indepdendent, they are truly never modified -- which is what
makes this patch reasonably short.
---
 gdb/psymtab.c | 59 ++++++++++++++++++++++++++++-------------------------------
 gdb/psymtab.h |  4 ++--
 2 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index a831e5eec40..f1aabd5b10b 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -37,18 +37,15 @@
 #include <set>
 #include "gdbsupport/buildargv.h"
 
-static struct partial_symbol *lookup_partial_symbol (struct objfile *,
-						     struct partial_symtab *,
-						     const lookup_name_info &,
-						     int,
-						     domain_search_flags);
+static const struct partial_symbol *lookup_partial_symbol
+     (struct objfile *, struct partial_symtab *, const lookup_name_info &,
+      int, domain_search_flags);
 
 static const char *psymtab_to_fullname (struct partial_symtab *ps);
 
-static struct partial_symbol *find_pc_sect_psymbol (struct objfile *,
-						    struct partial_symtab *,
-						    CORE_ADDR,
-						    struct obj_section *);
+static const struct partial_symbol *find_pc_sect_psymbol
+     (struct objfile *, struct partial_symtab *, CORE_ADDR,
+      struct obj_section *);
 
 static struct compunit_symtab *psymtab_to_symtab (struct objfile *objfile,
 						  struct partial_symtab *pst);
@@ -117,7 +114,7 @@ find_pc_sect_psymtab_closer (struct objfile *objfile,
     {
       if (pc >= tpst->text_low (objfile) && pc < tpst->text_high (objfile))
 	{
-	  struct partial_symbol *p;
+	  const struct partial_symbol *p;
 	  CORE_ADDR this_addr;
 
 	  /* NOTE: This assumes that every psymbol has a
@@ -212,12 +209,12 @@ psymbol_functions::find_pc_sect_compunit_symtab
 /* Find which partial symbol within a psymtab matches PC and SECTION.
    Return NULL if none.  */
 
-static struct partial_symbol *
+static const struct partial_symbol *
 find_pc_sect_psymbol (struct objfile *objfile,
 		      struct partial_symtab *psymtab, CORE_ADDR pc,
 		      struct obj_section *section)
 {
-  struct partial_symbol *best = NULL;
+  const struct partial_symbol *best = NULL;
   CORE_ADDR best_pc;
   const CORE_ADDR textlow = psymtab->text_low (objfile);
 
@@ -229,7 +226,7 @@ find_pc_sect_psymbol (struct objfile *objfile,
   /* Search the global symbols as well as the static symbols, so that
      find_pc_partial_function doesn't use a minimal symbol and thus
      cache a bad endaddr.  */
-  for (partial_symbol *p : psymtab->global_psymbols)
+  for (const partial_symbol *p : psymtab->global_psymbols)
     {
       if (p->domain == VAR_DOMAIN
 	  && p->aclass == LOC_BLOCK
@@ -249,7 +246,7 @@ find_pc_sect_psymbol (struct objfile *objfile,
 	}
     }
 
-  for (partial_symbol *p : psymtab->static_psymbols)
+  for (const partial_symbol *p : psymtab->static_psymbols)
     {
       if (p->domain == VAR_DOMAIN
 	  && p->aclass == LOC_BLOCK
@@ -289,7 +286,7 @@ psymbol_functions::lookup_global_symbol_language (struct objfile *objfile,
 
   for (partial_symtab *ps : partial_symbols (objfile))
     {
-      struct partial_symbol *psym;
+      const struct partial_symbol *psym;
       if (ps->readin_p (objfile))
 	continue;
 
@@ -307,7 +304,7 @@ psymbol_functions::lookup_global_symbol_language (struct objfile *objfile,
 /* Returns true if PSYM matches LOOKUP_NAME.  */
 
 static bool
-psymbol_name_matches (partial_symbol *psym,
+psymbol_name_matches (const partial_symbol *psym,
 		      const lookup_name_info &lookup_name)
 {
   const language_defn *lang = language_def (psym->ginfo.language ());
@@ -320,14 +317,14 @@ psymbol_name_matches (partial_symbol *psym,
    LOOKUP_NAME.  Check the global symbols if GLOBAL, the static
    symbols if not.  */
 
-static struct partial_symbol *
+static const struct partial_symbol *
 lookup_partial_symbol (struct objfile *objfile,
 		       struct partial_symtab *pst,
 		       const lookup_name_info &lookup_name,
 		       int global, domain_search_flags domain)
 {
-  struct partial_symbol **start, **psym;
-  struct partial_symbol **top, **real_top, **bottom, **center;
+  const struct partial_symbol **start, **psym;
+  const struct partial_symbol **top, **real_top, **bottom, **center;
   int length = (global
 		? pst->global_psymbols.size ()
 		: pst->static_psymbols.size ());
@@ -497,11 +494,11 @@ psymbol_functions::forget_cached_source_info (struct objfile *objfile)
 
 static void
 print_partial_symbols (struct gdbarch *gdbarch, struct objfile *objfile,
-		       const std::vector<partial_symbol *> &symbols,
+		       const std::vector<const partial_symbol *> &symbols,
 		       const char *what, struct ui_file *outfile)
 {
   gdb_printf (outfile, "  %s partial symbols:\n", what);
-  for (partial_symbol *p : symbols)
+  for (const partial_symbol *p : symbols)
     {
       QUIT;
       gdb_printf (outfile, "    `%s'", p->ginfo.linkage_name ());
@@ -830,15 +827,15 @@ recursively_search_psymtabs
 	}
     }
 
-  partial_symbol **gbound = (ps->global_psymbols.data ()
-			     + ps->global_psymbols.size ());
-  partial_symbol **sbound = (ps->static_psymbols.data ()
-			     + ps->static_psymbols.size ());
-  partial_symbol **bound = gbound;
+  const partial_symbol **gbound = (ps->global_psymbols.data ()
+				   + ps->global_psymbols.size ());
+  const partial_symbol **sbound = (ps->static_psymbols.data ()
+				   + ps->static_psymbols.size ());
+  const partial_symbol **bound = gbound;
 
   /* Go through all of the symbols stored in a partial
      symtab in one loop.  */
-  partial_symbol **psym = ps->global_psymbols.data ();
+  const partial_symbol **psym = ps->global_psymbols.data ();
 
   if ((search_flags & SEARCH_GLOBAL_BLOCK) == 0)
     {
@@ -1006,7 +1003,7 @@ partial_symtab::end ()
   /* Sort the global list; don't sort the static list.  */
   std::sort (global_psymbols.begin (),
 	     global_psymbols.end (),
-	     [] (partial_symbol *s1, partial_symbol *s2)
+	     [] (const partial_symbol *s1, const partial_symbol *s2)
     {
       return strcmp_iw_ordered (s1->ginfo.search_name (),
 				s2->ginfo.search_name ()) < 0;
@@ -1075,7 +1072,7 @@ partial_symtab::add_psymbol (const partial_symbol &psymbol,
     return;
 
   /* Save pointer to partial symbol in psymtab, growing symtab if needed.  */
-  std::vector<partial_symbol *> &list
+  std::vector<const partial_symbol *> &list
     = (where == psymbol_placement::STATIC
        ? static_psymbols
        : global_psymbols);
@@ -1496,7 +1493,7 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 		continue;
 	      bv = cust->blockvector ();
 	      b = bv->static_block ();
-	      for (partial_symbol *psym : ps->static_psymbols)
+	      for (const partial_symbol *psym : ps->static_psymbols)
 		{
 		  /* Skip symbols for inlined functions without address.  These may
 		     or may not have a match in the full symtab.  */
@@ -1518,7 +1515,7 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 		    }
 		}
 	      b = bv->global_block ();
-	      for (partial_symbol *psym : ps->global_psymbols)
+	      for (const partial_symbol *psym : ps->global_psymbols)
 		{
 		  lookup_name_info lookup_name
 		    (psym->ginfo.search_name (), symbol_name_match_type::SEARCH_NAME);
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index 557a7cf4c08..f41f2418639 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -444,7 +444,7 @@ struct partial_symtab
      improve access.  Binary search will be the usual method of
      finding a symbol within it.  */
 
-  std::vector<partial_symbol *> global_psymbols;
+  std::vector<const partial_symbol *> global_psymbols;
 
   /* Static symbol list.  This list will *not* be sorted after readin;
      to find a symbol in it, exhaustive search must be used.  This is
@@ -453,7 +453,7 @@ struct partial_symtab
      to take a *lot* of time; check) or an error (and we don't care
      how long errors take).  */
 
-  std::vector<partial_symbol *> static_psymbols;
+  std::vector<const partial_symbol *> static_psymbols;
 
   /* True if the name of this partial symtab is not a source file name.  */
 

-- 
2.43.0

Re: [PATCH 1/3] Some constification in psymtab

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

On Wed, Mar 20, 2024 at 9:24 PM Tom Tromey <tromey@adacore.com> wrote:

> This patch changes some spots in psymtab.[ch] to use 'const'.  This is
> just preparation for a subsequent patch.  Note that psymbols are
> conceptually const, and since they were changed to be
> objfile-indepdendent, they are truly never modified -- which is what
> makes this patch reasonably short.
> ---
>  gdb/psymtab.c | 59
> ++++++++++++++++++++++++++++-------------------------------
>  gdb/psymtab.h |  4 ++--
>  2 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index a831e5eec40..f1aabd5b10b 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -37,18 +37,15 @@
>  #include <set>
>  #include "gdbsupport/buildargv.h"
>
> This patch looks reasonable and causes no regressions for Fedora Rawhide
> on aarch64.
>

[-- Attachment #2: Type: text/html, Size: 1249 bytes --]

[PATCH 2/3] Make bcache more type-safe

[Tom Tromey]

The bcache uses memcpy to make copies of the data passed to it.  In
C++, this is only safe for trivially-copyable types.

This patch changes bcache to require this property, and slightly
changes the API to make it easier to use when copying a single object.
It also makes the new 'insert' template methods return the correct
type.
---
 gdb/bcache.h   | 25 ++++++++++++++++++++++++-
 gdb/gdbtypes.c |  2 +-
 gdb/macrotab.c | 12 ++++++------
 gdb/psymtab.c  |  6 ++----
 4 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/gdb/bcache.h b/gdb/bcache.h
index 17a09880d99..a77cd005ea6 100644
--- a/gdb/bcache.h
+++ b/gdb/bcache.h
@@ -152,7 +152,26 @@ struct bcache
      were newly added to the cache, or to false if the bytes were
      found in the cache.  */
 
-  const void *insert (const void *addr, int length, bool *added = nullptr);
+  template<typename T, typename = gdb::Requires<std::is_trivially_copyable<T>>>
+  const T *insert (const T *addr, int length, bool *added = nullptr)
+  {
+    return (const T *) this->insert ((const void *) addr, length, added);
+  }
+
+  /* Find a copy of OBJECT in this bcache.  If BCACHE has never seen
+     those bytes before, add a copy of them to BCACHE.  In either
+     case, return a pointer to BCACHE's copy of that string.  Since
+     the cached value is meant to be read-only, return a const buffer.
+     If ADDED is not NULL, set *ADDED to true if the bytes were newly
+     added to the cache, or to false if the bytes were found in the
+     cache.  */
+
+  template<typename T, typename = gdb::Requires<std::is_trivially_copyable<T>>>
+  const T *insert (const T &object, bool *added = nullptr)
+  {
+    return (const T *) this->insert ((const void *) &object, sizeof (object),
+				     added);
+  }
 
   /* Print statistics on this bcache's memory usage and efficacity at
      eliminating duplication.  TYPE should be a string describing the
@@ -173,6 +192,10 @@ struct bcache
 
 private:
 
+  /* Implementation of the templated 'insert' methods.  */
+
+  const void *insert (const void *addr, int length, bool *added);
+
   /* All the bstrings are allocated here.  */
   struct obstack m_cache {};
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 960a7f49e45..4fcfbc587f6 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4373,7 +4373,7 @@ check_types_worklist (std::vector<type_equality_entry> *worklist,
 
       /* If the type pair has already been visited, we know it is
 	 ok.  */
-      cache->insert (&entry, sizeof (entry), &added);
+      cache->insert (entry, &added);
       if (!added)
 	continue;
 
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 5329d2a5a93..a9c425b63c9 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -110,8 +110,9 @@ macro_free (void *object, struct macro_table *t)
 /* If the macro table T has a bcache, then cache the LEN bytes at ADDR
    there, and return the cached copy.  Otherwise, just xmalloc a copy
    of the bytes, and return a pointer to that.  */
-static const void *
-macro_bcache (struct macro_table *t, const void *addr, int len)
+template<typename U>
+static const U *
+macro_bcache (struct macro_table *t, const U *addr, int len)
 {
   if (t->bcache)
     return t->bcache->insert (addr, len);
@@ -120,7 +121,7 @@ macro_bcache (struct macro_table *t, const void *addr, int len)
       void *copy = xmalloc (len);
 
       memcpy (copy, addr, len);
-      return copy;
+      return (const U *) copy;
     }
 }
 
@@ -131,7 +132,7 @@ macro_bcache (struct macro_table *t, const void *addr, int len)
 static const char *
 macro_bcache_str (struct macro_table *t, const char *s)
 {
-  return (const char *) macro_bcache (t, s, strlen (s) + 1);
+  return macro_bcache (t, s, strlen (s) + 1);
 }
 
 
@@ -572,8 +573,7 @@ new_macro_definition (struct macro_table *t,
 	cached_argv[i] = macro_bcache_str (t, argv[i]);
 
       /* Now bcache the array of argument pointers itself.  */
-      d->argv = ((const char * const *)
-		 macro_bcache (t, cached_argv, cached_argv_size));
+      d->argv = macro_bcache (t, cached_argv, cached_argv_size);
     }
 
   /* We don't bcache the entire definition structure because it's got
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index f1aabd5b10b..ca638515d61 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1062,10 +1062,8 @@ partial_symtab::add_psymbol (const partial_symbol &psymbol,
   bool added;
 
   /* Stash the partial symbol away in the cache.  */
-  partial_symbol *psym
-    = ((struct partial_symbol *)
-       partial_symtabs->psymbol_cache.insert
-       (&psymbol, sizeof (struct partial_symbol), &added));
+  const partial_symbol *psym = partial_symtabs->psymbol_cache.insert (psymbol,
+								      &added);
 
   /* Do not duplicate global partial symbols.  */
   if (where == psymbol_placement::GLOBAL && !added)

-- 
2.43.0

Re: [PATCH 2/3] Make bcache more type-safe

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Wed, Mar 20, 2024 at 9:24 PM Tom Tromey <tromey@adacore.com> wrote:

> The bcache uses memcpy to make copies of the data passed to it.  In
> C++, this is only safe for trivially-copyable types.
>
> This patch changes bcache to require this property, and slightly
> changes the API to make it easier to use when copying a single object.
> It also makes the new 'insert' template methods return the correct
> type.
> ---
>  gdb/bcache.h   | 25 ++++++++++++++++++++++++-
>  gdb/gdbtypes.c |  2 +-
>  gdb/macrotab.c | 12 ++++++------
>  gdb/psymtab.c  |  6 ++----
>  4 files changed, 33 insertions(+), 12 deletions(-)
>
>
> I can confirm this change does not cause any regressions on Fedora Rawhide
aarch64.

[-- Attachment #2: Type: text/html, Size: 1090 bytes --]

[PATCH 3/3] Remove a couple unnecessary casts

[Tom Tromey]

After the previous bcache change, a couple of casts in objfiles.h are
now redundant.
---
 gdb/objfiles.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 8b8b7182e87..ab655906508 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -234,14 +234,14 @@ struct objfile_per_bfd_storage
 
   const char *intern (const char *str)
   {
-    return (const char *) string_cache.insert (str, strlen (str) + 1);
+    return string_cache.insert (str, strlen (str) + 1);
   }
 
   /* Same as the above, but for an std::string.  */
 
   const char *intern (const std::string &str)
   {
-    return (const char *) string_cache.insert (str.c_str (), str.size () + 1);
+    return string_cache.insert (str.c_str (), str.size () + 1);
   }
 
   /* Get the BFD this object is associated to.  */

-- 
2.43.0

Re: [PATCH 3/3] Remove a couple unnecessary casts

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

On Wed, Mar 20, 2024 at 9:25 PM Tom Tromey <tromey@adacore.com> wrote:

> After the previous bcache change, a couple of casts in objfiles.h are
> now redundant.
> ---
>  gdb/objfiles.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> It's nice to get rid of redundant things. Causes no regressions as
> expected.
>

[-- Attachment #2: Type: text/html, Size: 670 bytes --]