Use std::sort instead of qsort in minsyms.c
Commit Message
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(-)
Comments
* 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
>
>>>>> "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
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
> >
@@ -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. */