[PATCH] Add initializers to bound_minimal_symbol

[PATCH] Add initializers to bound_minimal_symbol

[Tom Tromey]

This adds initializers to bound_minimal_symbol, allowing for the
removal of some calls to memset.
---
 gdb/ada-lang.c | 2 --
 gdb/findvar.c  | 3 +--
 gdb/minsyms.h  | 4 ++--
 gdb/value.c    | 7 +------
 4 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0dd83cda8e5..d2f620cbb04 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4562,8 +4562,6 @@ ada_lookup_simple_minsym (const char *name)
 {
   struct bound_minimal_symbol result;
 
-  memset (&result, 0, sizeof (result));
-
   symbol_name_match_type match_type = name_match_type_from_name (name);
   lookup_name_info lookup_name (name, match_type);
 
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 1d22e645305..6660b006795 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -404,7 +404,7 @@ symbol_read_needs_frame (struct symbol *sym)
 struct minsym_lookup_data
 {
   /* The name of the minimal symbol we are searching for.  */
-  const char *name;
+  const char *name = nullptr;
 
   /* The field where the callback should store the minimal symbol
      if found.  It should be initialized to NULL before the search
@@ -751,7 +751,6 @@ language_defn::read_var_value (struct symbol *var,
 	struct minimal_symbol *msym;
 	struct obj_section *obj_section;
 
-	memset (&lookup_data, 0, sizeof (lookup_data));
 	lookup_data.name = var->linkage_name ();
 
 	gdbarch_iterate_over_objfiles_in_search_order
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 5970cb3bb99..ed31d32483a 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -31,12 +31,12 @@ struct bound_minimal_symbol
   /* The minimal symbol that was found, or NULL if no minimal symbol
      was found.  */
 
-  struct minimal_symbol *minsym;
+  struct minimal_symbol *minsym = nullptr;
 
   /* If MINSYM is not NULL, then this is the objfile in which the
      symbol is defined.  */
 
-  struct objfile *objfile;
+  struct objfile *objfile = nullptr;
 
   /* Return the obj_section from OBJFILE for MINSYM.  */
 
diff --git a/gdb/value.c b/gdb/value.c
index 7bd9891b3e9..76099966f13 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3171,13 +3171,8 @@ value_fn_field (struct value **arg1p, struct fn_field *f,
   struct bound_minimal_symbol msym;
 
   sym = lookup_symbol (physname, 0, VAR_DOMAIN, 0).symbol;
-  if (sym != NULL)
+  if (sym == nullptr)
     {
-      memset (&msym, 0, sizeof (msym));
-    }
-  else
-    {
-      gdb_assert (sym == NULL);
       msym = lookup_bound_minimal_symbol (physname);
       if (msym.minsym == NULL)
 	return NULL;
-- 
2.31.1

Re: [PATCH] Add initializers to bound_minimal_symbol

[Simon Marchi]

On 2022-02-08 15:42, Tom Tromey via Gdb-patches wrote:
> This adds initializers to bound_minimal_symbol, allowing for the
> removal of some calls to memset.

I was trying to see if it would be better to have a constructor (with
two arguments instead), so that we would never have an uninitialized
bound_minimal_symbol.  But there are cases where we really want to
initialize it as "empty" and then check for emptiness later, so your
change makes sense.

Simon

Re: [PATCH] Add initializers to bound_minimal_symbol

[Simon Marchi]

On 2022-02-11 08:34, Simon Marchi via Gdb-patches wrote:
> On 2022-02-08 15:42, Tom Tromey via Gdb-patches wrote:
>> This adds initializers to bound_minimal_symbol, allowing for the
>> removal of some calls to memset.
> 
> I was trying to see if it would be better to have a constructor (with
> two arguments instead), so that we would never have an uninitialized
> bound_minimal_symbol.  But there are cases where we really want to
> initialize it as "empty" and then check for emptiness later, so your
> change makes sense.
> 
> Simon


Hmm, I see this with g++-11:

/home/smarchi/src/binutils-gdb/gdb/linespec.c: In function ‘void add_minsym(minimal_symbol*, objfile*, symtab*, int, std::__debug::vector<bound_minimal_symbol>*)’:
/home/smarchi/src/binutils-gdb/gdb/linespec.c:4276:52: error: could not convert ‘{minsym, objfile}’ from ‘<brace-enclosed initializer list>’ to ‘bound_minimal_symbol’
 4276 |   struct bound_minimal_symbol mo = {minsym, objfile};
      |                                                    ^
      |                                                    |
      |                                                    <brace-enclosed initializer list>

Not sure what's the best way to fix this, possibly to add constructors
to bound_minimal_symbols.

Simon

Re: [PATCH] Add initializers to bound_minimal_symbol

[Tom Tromey]

Simon> Hmm, I see this with g++-11:

I'm also using gcc 11 and I don't see this error.

Simon> Not sure what's the best way to fix this, possibly to add constructors
Simon> to bound_minimal_symbols.

I wrote the appended and this also built fine.  But I don't know if it
would work for you.

Tom

diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index ed31d32483a..519702494ed 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -28,6 +28,14 @@ struct type;
 
 struct bound_minimal_symbol
 {
+  bound_minimal_symbol (struct minimal_symbol *msym, struct objfile *objf)
+    : minsym (msym),
+      objfile (objf)
+  {
+  }
+
+  bound_minimal_symbol () = default;
+
   /* The minimal symbol that was found, or NULL if no minimal symbol
      was found.  */
 

Re: [PATCH] Add initializers to bound_minimal_symbol

[Enze Li]

Hi Tom,

I also encountered a similar error message when I compiled with clang.Then I found this thread that you are discussing. 

I use the following commands to build,
$ ./configure --prefix=`pwd`/build CC=clang CXX=clang++ \
  CXXFLAGS=-std=gnu++11 --disable-unit-tests

It says,
=====================================================================
  CXX    macroscope.o
linespec.c:4276:31: error: no matching constructor for initialization
of 'struct bound_minimal_symbol'
  struct bound_minimal_symbol mo = {minsym, objfile};
                              ^    ~~~~~~~~~~~~~~~~~
./minsyms.h:29:8: note: candidate constructor (the implicit copy
constructor) not viable: requires 1 argument, but 2 were provided
struct bound_minimal_symbol
       ^
./minsyms.h:29:8: note: candidate constructor (the implicit move
constructor) not viable: requires 1 argument, but 2 were provided
./minsyms.h:29:8: note: candidate constructor (the implicit default
constructor) not viable: requires 0 arguments, but 2 were provided
=====================================================================

On Tue, 2022-02-15 at 08:41 -0700, Tom Tromey via Gdb-patches wrote:
> Simon> Hmm, I see this with g++-11:
> 
> I'm also using gcc 11 and I don't see this error.

It seems that Simon is referring to the use of the C++11 standard.

> 
> Simon> Not sure what's the best way to fix this, possibly to
> addconstructors
> Simon> to bound_minimal_symbols.
> 
> I wrote the appended and this also built fine.  But I don't know ifit
> would work for you.
> 
> Tom

With the appended patch, it built fine for me with clang.  And I also
tested it with gcc-11 as Simon did and again there were no problems.

$ ./configure --prefix=`pwd`/build CXXFLAGS=-std=gnu++11 \
  --disable-unit-tests

BTW, all tests were conducted on the latest openSUSE Tumbleweed.
It has:
- gcc (SUSE Linux) 11.2.1 20220103
- clang version 13.0.1

Thanks for fixing this.

Cheers!
Enze
 

Re: [PATCH] Add initializers to bound_minimal_symbol

[Simon Marchi]

On 2022-02-16 09:31, Enze Li wrote:
> Hi Tom,
> 
> I also encountered a similar error message when I compiled with clang.Then I found this thread that you are discussing. 
> 
> I use the following commands to build,
> $ ./configure --prefix=`pwd`/build CC=clang CXX=clang++ \
>   CXXFLAGS=-std=gnu++11 --disable-unit-tests
> 
> It says,
> =====================================================================
>   CXX    macroscope.o
> linespec.c:4276:31: error: no matching constructor for initialization
> of 'struct bound_minimal_symbol'
>   struct bound_minimal_symbol mo = {minsym, objfile};
>                               ^    ~~~~~~~~~~~~~~~~~
> ./minsyms.h:29:8: note: candidate constructor (the implicit copy
> constructor) not viable: requires 1 argument, but 2 were provided
> struct bound_minimal_symbol
>        ^
> ./minsyms.h:29:8: note: candidate constructor (the implicit move
> constructor) not viable: requires 1 argument, but 2 were provided
> ./minsyms.h:29:8: note: candidate constructor (the implicit default
> constructor) not viable: requires 0 arguments, but 2 were provided
> =====================================================================
> 
> On Tue, 2022-02-15 at 08:41 -0700, Tom Tromey via Gdb-patches wrote:
>> Simon> Hmm, I see this with g++-11:
>>
>> I'm also using gcc 11 and I don't see this error.
> 
> It seems that Simon is referring to the use of the C++11 standard.

I didn't mean to, but I think you are right.  This build of mine has
-std=c++11 in CXXFLAGS, which I added exactly to catch these issues
(since the recent compilers default to a more recent C++).

>> I wrote the appended and this also built fine.  But I don't know ifit
>> would work for you.
>>
>> Tom
> 
> With the appended patch, it built fine for me with clang.  And I also
> tested it with gcc-11 as Simon did and again there were no problems.
> 
> $ ./configure --prefix=`pwd`/build CXXFLAGS=-std=gnu++11 \
>   --disable-unit-tests
> 
> BTW, all tests were conducted on the latest openSUSE Tumbleweed.
> It has:
> - gcc (SUSE Linux) 11.2.1 20220103
> - clang version 13.0.1

LGTM too, thanks.

I would just suggest changing the call in add_minsym like so:

-  struct bound_minimal_symbol mo = {minsym, objfile};
-  msyms->push_back (mo);
+  msyms->emplace_back (minsym, objfile);

This just avoids constructing the object and then copying it.

Simon

Re: [PATCH] Add initializers to bound_minimal_symbol

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> I would just suggest changing the call in add_minsym like so:

Simon> -  struct bound_minimal_symbol mo = {minsym, objfile};
Simon> -  msyms->push_back (mo);
Simon> +  msyms->emplace_back (minsym, objfile);

Simon> This just avoids constructing the object and then copying it.

I did this and also made some other more cosmetic changes.
I sent the new patch to the list.

Tom