Use std::sort instead of qsort in minsyms.c

Message ID 20190929005005.67668-1-cbiesinger@google.com
State New
Headers show
Series
  • Use std::sort instead of qsort in minsyms.c
Related show

Commit Message

Christian Biesinger via gdb-patches Sept. 29, 2019, 12:50 a.m.
This has better typesafety and is also marginally faster (either
due to inlining or because it avoids indirection through a
function pointer).

Note that in this change:
-       return 1;               /* fn1 has no name, so it is "less".  */
+       return true;            /* fn1 has no name, so it is "less".  */
       else if (name1)          /* fn2 has no name, so it is "less".  */
-       return -1;
+       return false;
I am fairly sure the old code was wrong (ie. code didn't match the
comment and the comment seemed correct), so I fixed it.

gdb/ChangeLog:

2019-09-28  Christian Biesinger  <cbiesinger@google.com>

	* minsyms.c (compare_minimal_symbols): Rename to...
	(minimal_symbol_is_less_than): ...this, and adjust to STL
	conventions (return bool, take arguments as references)
	(minimal_symbol_reader::install): Call std::sort instead
	of qsort.
---
 gdb/minsyms.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

-- 
2.23.0.444.g18eeb5a265-goog

Comments

Andrew Burgess Sept. 30, 2019, 1:36 p.m. | #1
* Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> [2019-09-28 19:50:05 -0500]:

> This has better typesafety and is also marginally faster (either

> due to inlining or because it avoids indirection through a

> function pointer).

> 

> Note that in this change:

> -       return 1;               /* fn1 has no name, so it is "less".  */

> +       return true;            /* fn1 has no name, so it is "less".  */

>        else if (name1)          /* fn2 has no name, so it is "less".  */

> -       return -1;

> +       return false;

> I am fairly sure the old code was wrong (ie. code didn't match the

> comment and the comment seemed correct), so I fixed it.

> 

> gdb/ChangeLog:

> 

> 2019-09-28  Christian Biesinger  <cbiesinger@google.com>

> 

> 	* minsyms.c (compare_minimal_symbols): Rename to...

> 	(minimal_symbol_is_less_than): ...this, and adjust to STL

> 	conventions (return bool, take arguments as references)

> 	(minimal_symbol_reader::install): Call std::sort instead

> 	of qsort.

> ---

>  gdb/minsyms.c | 33 +++++++++++++--------------------

>  1 file changed, 13 insertions(+), 20 deletions(-)

> 

> diff --git a/gdb/minsyms.c b/gdb/minsyms.c

> index f06de4d88e..6384e7d72a 100644

> --- a/gdb/minsyms.c

> +++ b/gdb/minsyms.c

> @@ -1128,37 +1128,31 @@ minimal_symbol_reader::record_full (const char *name, int name_len,

>     on unsigned comparisons, so that we sort into unsigned numeric order.

>     Within groups with the same address, sort by name.  */


This comment ^^^ should be updated as it still talks about returning a
signed result.

>  

> -static int

> -compare_minimal_symbols (const void *fn1p, const void *fn2p)

> +static inline bool

> +minimal_symbol_is_less_than (const minimal_symbol &fn1, const minimal_symbol &fn2)


This line is too long now.

With those issues fixed this looks good to me.

Thanks,
Andrew

>  {

> -  const struct minimal_symbol *fn1;

> -  const struct minimal_symbol *fn2;

> -

> -  fn1 = (const struct minimal_symbol *) fn1p;

> -  fn2 = (const struct minimal_symbol *) fn2p;

> -

> -  if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) < MSYMBOL_VALUE_RAW_ADDRESS (fn2))

> +  if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) < MSYMBOL_VALUE_RAW_ADDRESS (&fn2))

>      {

> -      return (-1);		/* addr 1 is less than addr 2.  */

> +      return true;		/* addr 1 is less than addr 2.  */

>      }

> -  else if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) > MSYMBOL_VALUE_RAW_ADDRESS (fn2))

> +  else if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) > MSYMBOL_VALUE_RAW_ADDRESS (&fn2))

>      {

> -      return (1);		/* addr 1 is greater than addr 2.  */

> +      return false;		/* addr 1 is greater than addr 2.  */

>      }

>    else

>      /* addrs are equal: sort by name */

>      {

> -      const char *name1 = MSYMBOL_LINKAGE_NAME (fn1);

> -      const char *name2 = MSYMBOL_LINKAGE_NAME (fn2);

> +      const char *name1 = MSYMBOL_LINKAGE_NAME (&fn1);

> +      const char *name2 = MSYMBOL_LINKAGE_NAME (&fn2);

>  

>        if (name1 && name2)	/* both have names */

> -	return strcmp (name1, name2);

> +	return strcmp (name1, name2) < 0;

>        else if (name2)

> -	return 1;		/* fn1 has no name, so it is "less".  */

> +	return true;		/* fn1 has no name, so it is "less".  */

>        else if (name1)		/* fn2 has no name, so it is "less".  */

> -	return -1;

> +	return false;

>        else

> -	return (0);		/* Neither has a name, so they're equal.  */

> +	return false;		/* Neither has a name, so they're equal.  */

>      }

>  }

>  

> @@ -1315,8 +1309,7 @@ minimal_symbol_reader::install ()

>  

>        /* Sort the minimal symbols by address.  */

>  

> -      qsort (msymbols, mcount, sizeof (struct minimal_symbol),

> -	     compare_minimal_symbols);

> +      std::sort (msymbols, msymbols + mcount, minimal_symbol_is_less_than);

>  

>        /* Compact out any duplicates, and free up whatever space we are

>           no longer using.  */

> -- 

> 2.23.0.444.g18eeb5a265-goog

>
Tom Tromey Sept. 30, 2019, 2:17 p.m. | #2
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:


Christian> Note that in this change:
Christian> -       return 1;               /* fn1 has no name, so it is "less".  */
Christian> +       return true;            /* fn1 has no name, so it is "less".  */
Christian>        else if (name1)          /* fn2 has no name, so it is "less".  */
Christian> -       return -1;
Christian> +       return false;
Christian> I am fairly sure the old code was wrong (ie. code didn't match the
Christian> comment and the comment seemed correct), so I fixed it.

Yeah, it looks like the values were switched -- i.e., returning 1 if
name1 was NULL, where it should have returned -1 in that case.  I think
this didn't matter in practice, though, because the sorting of this case
is arbitrary, so either way is fine as long as it is consistent.

Christian> 2019-09-28  Christian Biesinger  <cbiesinger@google.com>

Christian> 	* minsyms.c (compare_minimal_symbols): Rename to...
Christian> 	(minimal_symbol_is_less_than): ...this, and adjust to STL
Christian> 	conventions (return bool, take arguments as references)
Christian> 	(minimal_symbol_reader::install): Call std::sort instead
Christian> 	of qsort.

This is ok.  Thanks.

Tom
Christian Biesinger via gdb-patches Sept. 30, 2019, 6:33 p.m. | #3
On Mon, Sep 30, 2019 at 8:36 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>

> * Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> [2019-09-28 19:50:05 -0500]:

>

> > This has better typesafety and is also marginally faster (either

> > due to inlining or because it avoids indirection through a

> > function pointer).

> >

> > Note that in this change:

> > -       return 1;               /* fn1 has no name, so it is "less".  */

> > +       return true;            /* fn1 has no name, so it is "less".  */

> >        else if (name1)          /* fn2 has no name, so it is "less".  */

> > -       return -1;

> > +       return false;

> > I am fairly sure the old code was wrong (ie. code didn't match the

> > comment and the comment seemed correct), so I fixed it.

> >

> > gdb/ChangeLog:

> >

> > 2019-09-28  Christian Biesinger  <cbiesinger@google.com>

> >

> >       * minsyms.c (compare_minimal_symbols): Rename to...

> >       (minimal_symbol_is_less_than): ...this, and adjust to STL

> >       conventions (return bool, take arguments as references)

> >       (minimal_symbol_reader::install): Call std::sort instead

> >       of qsort.

> > ---

> >  gdb/minsyms.c | 33 +++++++++++++--------------------

> >  1 file changed, 13 insertions(+), 20 deletions(-)

> >

> > diff --git a/gdb/minsyms.c b/gdb/minsyms.c

> > index f06de4d88e..6384e7d72a 100644

> > --- a/gdb/minsyms.c

> > +++ b/gdb/minsyms.c

> > @@ -1128,37 +1128,31 @@ minimal_symbol_reader::record_full (const char *name, int name_len,

> >     on unsigned comparisons, so that we sort into unsigned numeric order.

> >     Within groups with the same address, sort by name.  */

>

> This comment ^^^ should be updated as it still talks about returning a

> signed result.


Thanks, fixed.

> > -static int

> > -compare_minimal_symbols (const void *fn1p, const void *fn2p)

> > +static inline bool

> > +minimal_symbol_is_less_than (const minimal_symbol &fn1, const minimal_symbol &fn2)

>

> This line is too long now.

>

> With those issues fixed this looks good to me.


Fixed.

Pushed to master:
To ssh://sourceware.org/git/binutils-gdb.git
   4a56a52007..6fb08628e0  HEAD -> master

Christian

> > -  const struct minimal_symbol *fn1;

> > -  const struct minimal_symbol *fn2;

> > -

> > -  fn1 = (const struct minimal_symbol *) fn1p;

> > -  fn2 = (const struct minimal_symbol *) fn2p;

> > -

> > -  if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) < MSYMBOL_VALUE_RAW_ADDRESS (fn2))

> > +  if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) < MSYMBOL_VALUE_RAW_ADDRESS (&fn2))

> >      {

> > -      return (-1);           /* addr 1 is less than addr 2.  */

> > +      return true;           /* addr 1 is less than addr 2.  */

> >      }

> > -  else if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) > MSYMBOL_VALUE_RAW_ADDRESS (fn2))

> > +  else if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) > MSYMBOL_VALUE_RAW_ADDRESS (&fn2))

> >      {

> > -      return (1);            /* addr 1 is greater than addr 2.  */

> > +      return false;          /* addr 1 is greater than addr 2.  */

> >      }

> >    else

> >      /* addrs are equal: sort by name */

> >      {

> > -      const char *name1 = MSYMBOL_LINKAGE_NAME (fn1);

> > -      const char *name2 = MSYMBOL_LINKAGE_NAME (fn2);

> > +      const char *name1 = MSYMBOL_LINKAGE_NAME (&fn1);

> > +      const char *name2 = MSYMBOL_LINKAGE_NAME (&fn2);

> >

> >        if (name1 && name2)    /* both have names */

> > -     return strcmp (name1, name2);

> > +     return strcmp (name1, name2) < 0;

> >        else if (name2)

> > -     return 1;               /* fn1 has no name, so it is "less".  */

> > +     return true;            /* fn1 has no name, so it is "less".  */

> >        else if (name1)                /* fn2 has no name, so it is "less".  */

> > -     return -1;

> > +     return false;

> >        else

> > -     return (0);             /* Neither has a name, so they're equal.  */

> > +     return false;           /* Neither has a name, so they're equal.  */

> >      }

> >  }

> >

> > @@ -1315,8 +1309,7 @@ minimal_symbol_reader::install ()

> >

> >        /* Sort the minimal symbols by address.  */

> >

> > -      qsort (msymbols, mcount, sizeof (struct minimal_symbol),

> > -          compare_minimal_symbols);

> > +      std::sort (msymbols, msymbols + mcount, minimal_symbol_is_less_than);

> >

> >        /* Compact out any duplicates, and free up whatever space we are

> >           no longer using.  */

> > --

> > 2.23.0.444.g18eeb5a265-goog

> >

Patch

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index f06de4d88e..6384e7d72a 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1128,37 +1128,31 @@  minimal_symbol_reader::record_full (const char *name, int name_len,
    on unsigned comparisons, so that we sort into unsigned numeric order.
    Within groups with the same address, sort by name.  */
 
-static int
-compare_minimal_symbols (const void *fn1p, const void *fn2p)
+static inline bool
+minimal_symbol_is_less_than (const minimal_symbol &fn1, const minimal_symbol &fn2)
 {
-  const struct minimal_symbol *fn1;
-  const struct minimal_symbol *fn2;
-
-  fn1 = (const struct minimal_symbol *) fn1p;
-  fn2 = (const struct minimal_symbol *) fn2p;
-
-  if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) < MSYMBOL_VALUE_RAW_ADDRESS (fn2))
+  if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) < MSYMBOL_VALUE_RAW_ADDRESS (&fn2))
     {
-      return (-1);		/* addr 1 is less than addr 2.  */
+      return true;		/* addr 1 is less than addr 2.  */
     }
-  else if (MSYMBOL_VALUE_RAW_ADDRESS (fn1) > MSYMBOL_VALUE_RAW_ADDRESS (fn2))
+  else if (MSYMBOL_VALUE_RAW_ADDRESS (&fn1) > MSYMBOL_VALUE_RAW_ADDRESS (&fn2))
     {
-      return (1);		/* addr 1 is greater than addr 2.  */
+      return false;		/* addr 1 is greater than addr 2.  */
     }
   else
     /* addrs are equal: sort by name */
     {
-      const char *name1 = MSYMBOL_LINKAGE_NAME (fn1);
-      const char *name2 = MSYMBOL_LINKAGE_NAME (fn2);
+      const char *name1 = MSYMBOL_LINKAGE_NAME (&fn1);
+      const char *name2 = MSYMBOL_LINKAGE_NAME (&fn2);
 
       if (name1 && name2)	/* both have names */
-	return strcmp (name1, name2);
+	return strcmp (name1, name2) < 0;
       else if (name2)
-	return 1;		/* fn1 has no name, so it is "less".  */
+	return true;		/* fn1 has no name, so it is "less".  */
       else if (name1)		/* fn2 has no name, so it is "less".  */
-	return -1;
+	return false;
       else
-	return (0);		/* Neither has a name, so they're equal.  */
+	return false;		/* Neither has a name, so they're equal.  */
     }
 }
 
@@ -1315,8 +1309,7 @@  minimal_symbol_reader::install ()
 
       /* Sort the minimal symbols by address.  */
 
-      qsort (msymbols, mcount, sizeof (struct minimal_symbol),
-	     compare_minimal_symbols);
+      std::sort (msymbols, msymbols + mcount, minimal_symbol_is_less_than);
 
       /* Compact out any duplicates, and free up whatever space we are
          no longer using.  */