[review] Replace some more qsort calls with std::sort

Message ID gerrit.1571352182000.Ibcddce12a3d07448701e731b7150fa23611d86de@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 17, 2019, 10:43 p.m. UTC
  Christian Biesinger has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................

Replace some more qsort calls with std::sort

This has better typesafety, avoids a function pointer indirection,
and can benefit from inlining.

gdb/ChangeLog:

2019-10-03  Christian Biesinger  <cbiesinger@google.com>

	* bcache.c (bcache::print_statistics): Use std::sort instead of qsort.
	* breakpoint.c (bp_locations_compare): Rename to...
	(bp_location_is_less_than): ...this, and change to std::sort semantics.
	(update_global_location_list): Use std::sort instead of qsort.
	* buildsym.c (compare_line_numbers): Rename to...
	(lte_is_less_than): ...this, and change to std::sort semantics.
	(buildsym_compunit::end_symtab_with_blockvector): Use std::sort
	instead of qsort.
	* disasm.c (compare_lines): Rename to...
	(line_is_less_than): ...this, and change to std::sort semantics.
	(do_mixed_source_and_assembly_deprecated): Call std::sort instead
	of qsort.
	* dwarf2-frame.c (qsort_fde_cmp): Rename to...
	(fde_is_less_than): ...this, and change to std::sort semantics.
	(dwarf2_build_frame_info): Call std::sort instead of qsort.
	* mdebugread.c (compare_blocks):
	(block_is_less_than): ...this, and change to std::sort semantics.
	(sort_blocks): Call std::sort instead of qsort.
	* objfiles.c (qsort_cmp): Rename to...
	(sort_cmp): ...this, and change to std::sort semantics.
	(update_section_map): Call std::sort instead of qsort.
	* remote.c (compare_pnums): Remove.
	(map_regcache_remote_table): Call std::sort instead of qsort.
	* utils.c (compare_positive_ints): Remove.
	* utils.h (compare_positive_ints): Remove.
	* xcoffread.c (compare_lte): Remove.
	(arrange_linetable): Call std::sort instead of qsort.

Change-Id: Ibcddce12a3d07448701e731b7150fa23611d86de
---
M gdb/bcache.c
M gdb/breakpoint.c
M gdb/buildsym.c
M gdb/disasm.c
M gdb/dwarf2-frame.c
M gdb/mdebugread.c
M gdb/objfiles.c
M gdb/remote.c
M gdb/utils.c
M gdb/utils.h
M gdb/xcoffread.c
11 files changed, 76 insertions(+), 127 deletions(-)
  

Comments

Simon Marchi (Code Review) Oct. 18, 2019, 1:47 p.m. UTC | #1
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 1:

(8 comments)

Thank you for the patch.  This is a nice improvement.

I found a few nits, but nothing serious.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c 
File gdb/breakpoint.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@522 
PS1, Line 522: /* Array is sorted by bp_location_is_less_than - primarily by the ADDRESS.  */
In gerrit I can't tell if this line is overlong.
If it is, it should wrap.

It would be nice if we could add column guides to gerrit.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11440 
PS1, Line 11440:     return (a->address > b->address) < (a->address < b->address);
I think this can just be a->address < b->address ?


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11448 
PS1, Line 11448: 	    < (a->pspace->num < b->pspace->num));
Likewise this can be simplified.

BTW gerrit doesn't seem to allow multi-line selections... bummer.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11452 
PS1, Line 11452:     return (a->permanent < b->permanent) < (a->permanent > b->permanent);
Ditto.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11460 
PS1, Line 11460: 	    < (a->owner->number < b->owner->number));
Ditto.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11462 
PS1, Line 11462:   return (a > b) < (a < b);
Even here.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/disasm.c 
File gdb/disasm.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/disasm.c@184 
PS1, Line 184:         val = mle1.start_pc - mle2.start_pc;
This code is a bit weird.  It only works because "val" has signed type,
as start_pc is unsigned.  I think it would be a lot clearer to just
change this to use plain < instead of subtraction.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/mdebugread.c 
File gdb/mdebugread.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/mdebugread.c@4570 
PS1, Line 4570:   addr_diff = (BLOCK_START (b1)) - (BLOCK_START (b2));
This has the same oddity involving signs.  I think just using the
obvious comparisons would be better.
  
Simon Marchi (Code Review) Oct. 18, 2019, 9:41 p.m. UTC | #2
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 1:

(9 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c 
File gdb/breakpoint.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@522 
PS1, Line 522: /* Array is sorted by bp_location_is_less_than - primarily by the ADDRESS.  */
> In gerrit I can't tell if this line is overlong. […]
When you're logged in, you can set diff preferences using the "gear" icon on the right, where you can set the diff width to 80. That lets you see if a line is too long.

(this line is 78 characters long)


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11440 
PS1, Line 11440:     return (a->address > b->address) < (a->address < b->address);
> I think this can just be a->address < b->address ?
Yeah, I think you're right. Done. I was confused by this code.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11448 
PS1, Line 11448: 	    < (a->pspace->num < b->pspace->num));
> Likewise this can be simplified. […]
Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11450 
PS1, Line 11450: /* Sort permanent breakpoints first.  */
               :   if (a->permanent != b->permanent)
Test of a multiline comment (I think you have to highlight in the code, then press c, instead of directly selectng multiple lines)


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11452 
PS1, Line 11452:     return (a->permanent < b->permanent) < (a->permanent > b->permanent);
> Ditto.
Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11460 
PS1, Line 11460: 	    < (a->owner->number < b->owner->number));
> Ditto.
Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11462 
PS1, Line 11462:   return (a > b) < (a < b);
> Even here.
Done.

I wonder why this code wasn't just subtracting one variable from the other in all these cases.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/disasm.c 
File gdb/disasm.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/disasm.c@184 
PS1, Line 184:         val = mle1.start_pc - mle2.start_pc;
> This code is a bit weird.  It only works because "val" has signed type, […]
Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/mdebugread.c 
File gdb/mdebugread.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/mdebugread.c@4570 
PS1, Line 4570:   addr_diff = (BLOCK_START (b1)) - (BLOCK_START (b2));
> This has the same oddity involving signs.  I think just using the […]
Done
  
Simon Marchi (Code Review) Oct. 19, 2019, 4:32 a.m. UTC | #3
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c 
File gdb/breakpoint.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@522 
PS1, Line 522: /* Array is sorted by bp_location_is_less_than - primarily by the ADDRESS.  */
> When you're logged in, you can set diff preferences using the "gear" icon on the right, where you ca […]
Tom, I have made a change that should make the line length 80 columns for all users that haven't set it themselves.  You should therefore see a guide at column 80.  Please tell me if that worked (unless you have already changed it yourself, then we'll need to ask somebody else).
  
Simon Marchi (Code Review) Oct. 19, 2019, 8:42 p.m. UTC | #4
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

Thank you.  I think this is ok now.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c 
File gdb/breakpoint.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@522 
PS1, Line 522: /* Array is sorted by bp_location_is_less_than - primarily by the ADDRESS.  */
> Tom, I have made a change that should make the line length 80 columns for all users that haven't set […]
Thanks to both of you.  Simon, I had already modified my settings, just FYI.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131/1/gdb/breakpoint.c@11462 
PS1, Line 11462:   return (a > b) < (a < b);
> Done.
> 
> I wonder why this code wasn't just subtracting one variable from the other in all these cases.

Probably due to fear of underflow.
  
Simon Marchi (Code Review) Oct. 21, 2019, 4:44 a.m. UTC | #5
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 3:

It looks like this patch causes some regressions when building with -D_GLIBCXX_DEBUG=1.  One such failure is in gdb.dwarf2/ada-linkage-name.exp (make check TESTS="gdb.dwarf2/ada-linkage-name.exp").

The problem is that the comparison functions don't have the required properties for std::sort:

 49 /usr/include/c++/9.2.0/bits/stl_algo.h:4858:^M
 50 In function:^M
 51     void std::sort(_RAIter, _RAIter, _Compare) [with _RAIter = ^M
 52     obj_section**; _Compare = bool (*)(const obj_section*, const ^M
 53     obj_section*)]^M
 54 ^M
 55 Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).^M
 56 ^M
 57 Objects involved in the operation:^M
 58     instance "functor" @ 0x0x7ffda9ccc9a0 {^M
 59       type = bool (*)(obj_section const*, obj_section const*);^M
 60     }^M
 61     iterator::value_type "ordered type" {^M
 62       type = obj_section*;^M
 63     }^M

It is probably a pre-existing bad behavior with the comparison functions, but using std::sort points those out.

I would encourage you to use -D_GLIBCXX_DEBUG=1 all the time for your development builds (except for when you want to do benchmarks), it catches a bunch of things.
  
Simon Marchi (Code Review) Oct. 21, 2019, 4:22 p.m. UTC | #6
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/131
......................................................................


Patch Set 3:

> Patch Set 3:
> 
> It looks like this patch causes some regressions when building with -D_GLIBCXX_DEBUG=1.  One such failure is in gdb.dwarf2/ada-linkage-name.exp (make check TESTS="gdb.dwarf2/ada-linkage-name.exp").
> 
> The problem is that the comparison functions don't have the required properties for std::sort:
> 
>  49 /usr/include/c++/9.2.0/bits/stl_algo.h:4858:^M
>  50 In function:^M
>  51     void std::sort(_RAIter, _RAIter, _Compare) [with _RAIter = ^M
>  52     obj_section**; _Compare = bool (*)(const obj_section*, const ^M
>  53     obj_section*)]^M
>  54 ^M
>  55 Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).^M
>  56 ^M
>  57 Objects involved in the operation:^M
>  58     instance "functor" @ 0x0x7ffda9ccc9a0 {^M
>  59       type = bool (*)(obj_section const*, obj_section const*);^M
>  60     }^M
>  61     iterator::value_type "ordered type" {^M
>  62       type = obj_section*;^M
>  63     }^M
> 
> It is probably a pre-existing bad behavior with the comparison functions, but using std::sort points those out.
> 
> I would encourage you to use -D_GLIBCXX_DEBUG=1 all the time for your development builds (except for when you want to do benchmarks), it catches a bunch of things.

Thanks for finding this! I will make sure to use that define. Meanwhile aburgess uploaded a patch to fix this: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/177
  

Patch

diff --git a/gdb/bcache.c b/gdb/bcache.c
index 14a7847..3f0a63b 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -23,6 +23,8 @@ 
 #include "gdb_obstack.h"
 #include "bcache.h"
 
+#include <algorithm>
+
 /* The type used to hold a single bcache string.  The user data is
    stored in d.data.  Since it can be any type, it needs to have the
    same alignment as the most strict alignment of any type on the host
@@ -311,10 +313,8 @@ 
 
     /* To compute the median, we need the set of chain lengths
        sorted.  */
-    qsort (chain_length, m_num_buckets, sizeof (chain_length[0]),
-	   compare_positive_ints);
-    qsort (entry_size, m_unique_count, sizeof (entry_size[0]),
-	   compare_positive_ints);
+    std::sort (chain_length, chain_length + m_num_buckets);
+    std::sort (entry_size, entry_size + m_unique_count);
 
     if (m_num_buckets > 0)
       {
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2fd2438..157ba6e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -519,7 +519,7 @@ 
 
 static struct breakpoint *breakpoint_chain;
 
-/* Array is sorted by bp_locations_compare - primarily by the ADDRESS.  */
+/* Array is sorted by bp_location_is_less_than - primarily by the ADDRESS.  */
 
 static struct bp_location **bp_locations;
 
@@ -773,7 +773,7 @@ 
 
 /* A comparison function for bp_location AP and BP that is used by
    bsearch.  This comparison function only cares about addresses, unlike
-   the more general bp_locations_compare function.  */
+   the more general bp_location_is_less_than function.  */
 
 static int
 bp_locations_compare_addrs (const void *ap, const void *bp)
@@ -11428,19 +11428,16 @@ 
 }
 
 /* A comparison function for bp_location AP and BP being interfaced to
-   qsort.  Sort elements primarily by their ADDRESS (no matter what
+   std::sort.  Sort elements primarily by their ADDRESS (no matter what
    bl_address_is_meaningful says), secondarily by ordering first
    permanent elements and terciarily just ensuring the array is sorted
-   stable way despite qsort being an unstable algorithm.  */
+   stable way despite std::sort being an unstable algorithm.  */
 
 static int
-bp_locations_compare (const void *ap, const void *bp)
+bp_location_is_less_than (const bp_location *a, const bp_location *b)
 {
-  const struct bp_location *a = *(const struct bp_location **) ap;
-  const struct bp_location *b = *(const struct bp_location **) bp;
-
   if (a->address != b->address)
-    return (a->address > b->address) - (a->address < b->address);
+    return (a->address > b->address) < (a->address < b->address);
 
   /* Sort locations at the same address by their pspace number, keeping
      locations of the same inferior (in a multi-inferior environment)
@@ -11448,11 +11445,11 @@ 
 
   if (a->pspace->num != b->pspace->num)
     return ((a->pspace->num > b->pspace->num)
-	    - (a->pspace->num < b->pspace->num));
+	    < (a->pspace->num < b->pspace->num));
 
   /* Sort permanent breakpoints first.  */
   if (a->permanent != b->permanent)
-    return (a->permanent < b->permanent) - (a->permanent > b->permanent);
+    return (a->permanent < b->permanent) < (a->permanent > b->permanent);
 
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
@@ -11460,9 +11457,9 @@ 
 
   if (a->owner->number != b->owner->number)
     return ((a->owner->number > b->owner->number)
-	    - (a->owner->number < b->owner->number));
+	    < (a->owner->number < b->owner->number));
 
-  return (a > b) - (a < b);
+  return (a > b) < (a < b);
 }
 
 /* Set bp_locations_placed_address_before_address_max and
@@ -11677,8 +11674,8 @@ 
   ALL_BREAKPOINTS (b)
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
-  qsort (bp_locations, bp_locations_count, sizeof (*bp_locations),
-	 bp_locations_compare);
+  std::sort (bp_locations, bp_locations + bp_locations_count,
+	     bp_location_is_less_than);
 
   bp_locations_target_extensions_update ();
 
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 8e05706..954a610 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -49,8 +49,6 @@ 
     struct block *block;
   };
 
-static int compare_line_numbers (const void *ln1p, const void *ln2p);
-
 /* Initial sizes of data structures.  These are realloc'd larger if
    needed, and realloc'd down to the size actually used, when
    completed.  */
@@ -729,23 +727,20 @@ 
 
 /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
 
-static int
-compare_line_numbers (const void *ln1p, const void *ln2p)
+static bool
+lte_is_less_than (const linetable_entry &ln1, const linetable_entry &ln2)
 {
-  struct linetable_entry *ln1 = (struct linetable_entry *) ln1p;
-  struct linetable_entry *ln2 = (struct linetable_entry *) ln2p;
-
   /* Note: this code does not assume that CORE_ADDRs can fit in ints.
      Please keep it that way.  */
-  if (ln1->pc < ln2->pc)
-    return -1;
+  if (ln1.pc < ln2.pc)
+    return true;
 
-  if (ln1->pc > ln2->pc)
-    return 1;
+  if (ln1.pc > ln2.pc)
+    return false;
 
   /* If pc equal, sort by line.  I'm not sure whether this is optimum
      behavior (see comment at struct linetable in symtab.h).  */
-  return ln1->line - ln2->line;
+  return ln1.line < ln2.line;
 }
 
 /* Subroutine of end_symtab to simplify it.  Look for a subfile that
@@ -968,9 +963,10 @@ 
 	     scrambled in reordered executables.  Sort it if
 	     OBJF_REORDERED is true.  */
 	  if (m_objfile->flags & OBJF_REORDERED)
-	    qsort (subfile->line_vector->item,
-		   subfile->line_vector->nitems,
-		   sizeof (struct linetable_entry), compare_line_numbers);
+	    std::sort (subfile->line_vector->item,
+		       subfile->line_vector->item
+			 + subfile->line_vector->nitems,
+		       lte_is_less_than);
 	}
 
       /* Allocate a symbol table if necessary.  */
diff --git a/gdb/disasm.c b/gdb/disasm.c
index f483a5e..8d4ea1e 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -163,30 +163,27 @@ 
   print_address (self->arch (), addr, self->stream ());
 }
 
-static int
-compare_lines (const void *mle1p, const void *mle2p)
+static bool
+line_is_less_than (const deprecated_dis_line_entry &mle1,
+		   const deprecated_dis_line_entry &mle2)
 {
-  struct deprecated_dis_line_entry *mle1, *mle2;
   int val;
 
-  mle1 = (struct deprecated_dis_line_entry *) mle1p;
-  mle2 = (struct deprecated_dis_line_entry *) mle2p;
-
   /* End of sequence markers have a line number of 0 but don't want to
      be sorted to the head of the list, instead sort by PC.  */
-  if (mle1->line == 0 || mle2->line == 0)
+  if (mle1.line == 0 || mle2.line == 0)
     {
-      val = mle1->start_pc - mle2->start_pc;
+      val = mle1.start_pc - mle2.start_pc;
       if (val == 0)
-        val = mle1->line - mle2->line;
+        val = mle1.line - mle2.line;
     }
   else
     {
-      val = mle1->line - mle2->line;
+      val = mle1.line - mle2.line;
       if (val == 0)
-        val = mle1->start_pc - mle2->start_pc;
+        val = mle1.start_pc - mle2.start_pc;
     }
-  return val;
+  return val < 0;
 }
 
 /* See disasm.h.  */
@@ -404,8 +401,7 @@ 
   /* Now, sort mle by line #s (and, then by addresses within lines).  */
 
   if (out_of_order)
-    qsort (mle, newlines, sizeof (struct deprecated_dis_line_entry),
-	   compare_lines);
+    std::sort (mle, mle + newlines, line_is_less_than);
 
   /* Now, for each line entry, emit the specified lines (unless
      they have been emitted before), followed by the assembly code
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 34b8cbc..8bf5b19 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -44,6 +44,8 @@ 
 #include "selftest-arch.h"
 #endif
 
+#include <algorithm>
+
 struct comp_unit;
 
 /* Call Frame Information (CFI).  */
@@ -2181,25 +2183,22 @@ 
   return ret;
 }
 
-static int
-qsort_fde_cmp (const void *a, const void *b)
+static bool
+fde_is_less_than (const dwarf2_fde *aa, const dwarf2_fde *bb)
 {
-  struct dwarf2_fde *aa = *(struct dwarf2_fde **)a;
-  struct dwarf2_fde *bb = *(struct dwarf2_fde **)b;
-
   if (aa->initial_location == bb->initial_location)
     {
       if (aa->address_range != bb->address_range
           && aa->eh_frame_p == 0 && bb->eh_frame_p == 0)
         /* Linker bug, e.g. gold/10400.
            Work around it by keeping stable sort order.  */
-        return (a < b) ? -1 : 1;
+        return aa < bb;
       else
         /* Put eh_frame entries after debug_frame ones.  */
-        return aa->eh_frame_p - bb->eh_frame_p;
+        return aa->eh_frame_p < bb->eh_frame_p;
     }
 
-  return (aa->initial_location < bb->initial_location) ? -1 : 1;
+  return aa->initial_location < bb->initial_location;
 }
 
 void
@@ -2347,8 +2346,8 @@ 
       int i;
 
       /* Prepare FDE table for lookups.  */
-      qsort (fde_table.entries, fde_table.num_entries,
-             sizeof (fde_table.entries[0]), qsort_fde_cmp);
+      std::sort (fde_table.entries, fde_table.entries + fde_table.num_entries,
+		 fde_is_less_than);
 
       /* Check for leftovers from --gc-sections.  The GNU linker sets
 	 the relevant symbols to zero, but doesn't zero the FDE *end*
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index e76fe9a..a640f15 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -68,6 +68,8 @@ 
 
 #include "expression.h"
 
+#include <algorithm>
+
 /* Provide a way to test if we have both ECOFF and ELF symbol tables.
    We use this define in order to know whether we should override a 
    symbol's ECOFF section with its ELF section.  This is necessary in 
@@ -4560,17 +4562,15 @@ 
 
 /* Blocks with a smaller low bound should come first.  */
 
-static int
-compare_blocks (const void *arg1, const void *arg2)
+static bool
+block_is_less_than (const struct block *b1, const struct block *b2)
 {
   LONGEST addr_diff;
-  struct block **b1 = (struct block **) arg1;
-  struct block **b2 = (struct block **) arg2;
 
-  addr_diff = (BLOCK_START ((*b1))) - (BLOCK_START ((*b2)));
+  addr_diff = (BLOCK_START (b1)) - (BLOCK_START (b2));
   if (addr_diff == 0)
-    return (BLOCK_END ((*b2))) - (BLOCK_END ((*b1)));
-  return addr_diff;
+    return (BLOCK_END (b2)) < (BLOCK_END (b1));
+  return addr_diff < 0;
 }
 
 /* Sort the blocks of a symtab S.
@@ -4600,10 +4600,9 @@ 
    * to detect -O3 images in advance.
    */
   if (BLOCKVECTOR_NBLOCKS (bv) > FIRST_LOCAL_BLOCK + 1)
-    qsort (&BLOCKVECTOR_BLOCK (bv, FIRST_LOCAL_BLOCK),
-	   BLOCKVECTOR_NBLOCKS (bv) - FIRST_LOCAL_BLOCK,
-	   sizeof (struct block *),
-	   compare_blocks);
+    std::sort (&BLOCKVECTOR_BLOCK (bv, FIRST_LOCAL_BLOCK),
+	       &BLOCKVECTOR_BLOCK (bv, BLOCKVECTOR_NBLOCKS (bv)),
+	       block_is_less_than);
 
   {
     CORE_ADDR high = 0;
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index f9e7d20..a709fc0 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1019,18 +1019,16 @@ 
 
 /* Qsort comparison function.  */
 
-static int
-qsort_cmp (const void *a, const void *b)
+static bool
+sort_cmp (const struct obj_section *sect1, const obj_section *sect2)
 {
-  const struct obj_section *sect1 = *(const struct obj_section **) a;
-  const struct obj_section *sect2 = *(const struct obj_section **) b;
   const CORE_ADDR sect1_addr = obj_section_addr (sect1);
   const CORE_ADDR sect2_addr = obj_section_addr (sect2);
 
   if (sect1_addr < sect2_addr)
-    return -1;
+    return true;
   else if (sect1_addr > sect2_addr)
-    return 1;
+    return false;
   else
     {
       /* Sections are at the same address.  This could happen if
@@ -1047,12 +1045,12 @@ 
 	  /* Case A.  The ordering doesn't matter: separate debuginfo files
 	     will be filtered out later.  */
 
-	  return 0;
+	  return false;
 	}
 
       /* Case B.  Maintain stable sort order, so bugs in GDB are easier to
 	 triage.  This section could be slow (since we iterate over all
-	 objfiles in each call to qsort_cmp), but this shouldn't happen
+	 objfiles in each call to sort_cmp), but this shouldn't happen
 	 very often (GDB is already in a confused state; one hopes this
 	 doesn't happen at all).  If you discover that significant time is
 	 spent in the loops below, do 'set complaints 100' and examine the
@@ -1067,9 +1065,9 @@ 
 
 	  ALL_OBJFILE_OSECTIONS (objfile1, osect)
 	    if (osect == sect1)
-	      return -1;
+	      return true;
 	    else if (osect == sect2)
-	      return 1;
+	      return false;
 
 	  /* We should have found one of the sections before getting here.  */
 	  gdb_assert_not_reached ("section not found");
@@ -1080,9 +1078,9 @@ 
 
 	  for (objfile *objfile : current_program_space->objfiles ())
 	    if (objfile == objfile1)
-	      return -1;
+	      return true;
 	    else if (objfile == objfile2)
-	      return 1;
+	      return false;
 
 	  /* We should have found one of the objfiles before getting here.  */
 	  gdb_assert_not_reached ("objfile not found");
@@ -1091,7 +1089,7 @@ 
 
   /* Unreachable.  */
   gdb_assert_not_reached ("unexpected code path");
-  return 0;
+  return false;
 }
 
 /* Select "better" obj_section to keep.  We prefer the one that came from
@@ -1283,7 +1281,7 @@ 
       if (insert_section_p (objfile->obfd, s->the_bfd_section))
 	map[i++] = s;
 
-  qsort (map, alloc_size, sizeof (*map), qsort_cmp);
+  std::sort (map, map + alloc_size, sort_cmp);
   map_size = filter_debuginfo_sections(map, alloc_size);
   map_size = filter_overlapping_sections(map, map_size);
 
diff --git a/gdb/remote.c b/gdb/remote.c
index f616569..352582e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -75,6 +75,7 @@ 
 #include "gdbsupport/scoped_restore.h"
 #include "gdbsupport/environ.h"
 #include "gdbsupport/byte-vector.h"
+#include <algorithm>
 #include <unordered_map>
 
 /* The remote target.  */
@@ -1276,22 +1277,6 @@ 
 }
 
 static int
-compare_pnums (const void *lhs_, const void *rhs_)
-{
-  const struct packet_reg * const *lhs
-    = (const struct packet_reg * const *) lhs_;
-  const struct packet_reg * const *rhs
-    = (const struct packet_reg * const *) rhs_;
-
-  if ((*lhs)->pnum < (*rhs)->pnum)
-    return -1;
-  else if ((*lhs)->pnum == (*rhs)->pnum)
-    return 0;
-  else
-    return 1;
-}
-
-static int
 map_regcache_remote_table (struct gdbarch *gdbarch, struct packet_reg *regs)
 {
   int regnum, num_remote_regs, offset;
@@ -1321,8 +1306,9 @@ 
     if (regs[regnum].pnum != -1)
       remote_regs[num_remote_regs++] = &regs[regnum];
 
-  qsort (remote_regs, num_remote_regs, sizeof (struct packet_reg *),
-	 compare_pnums);
+  std::sort (remote_regs, remote_regs + num_remote_regs,
+	     [] (const packet_reg *a, const packet_reg *b)
+	      { return a->pnum < b->pnum; });
 
   for (regnum = 0, offset = 0; regnum < num_remote_regs; regnum++)
     {
diff --git a/gdb/utils.c b/gdb/utils.c
index e685cc2..d365441 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3050,14 +3050,6 @@ 
   m_argv = argv;
 }
 
-int
-compare_positive_ints (const void *ap, const void *bp)
-{
-  /* Because we know we're comparing two ints which are positive,
-     there's no danger of overflow here.  */
-  return * (int *) ap - * (int *) bp;
-}
-
 #define AMBIGUOUS_MESS1	".\nMatching formats:"
 #define AMBIGUOUS_MESS2	\
   ".\nUse \"set gnutarget format-name\" to specify the format."
diff --git a/gdb/utils.h b/gdb/utils.h
index 76f0da6..af8b461 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -101,8 +101,6 @@ 
 
 extern int subset_compare (const char *, const char *);
 
-int compare_positive_ints (const void *ap, const void *bp);
-
 /* Compare C strings for std::sort.  */
 
 static inline bool
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index aec1923..9a417b2 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -28,6 +28,7 @@ 
 #include <sys/file.h>
 #endif
 #include <sys/stat.h>
+#include <algorithm>
 
 #include "coff/internal.h"
 #include "libcoff.h"		/* FIXME, internal data from BFD */
@@ -234,8 +235,6 @@ 
 static void add_stab_to_list (char *, struct pending_stabs **);
 #endif
 
-static int compare_lte (const void *, const void *);
-
 static struct linetable *arrange_linetable (struct linetable *);
 
 static void record_include_end (struct coff_symbol *);
@@ -407,18 +406,6 @@ 
 /* *INDENT-ON* */
 
 
-
-/* compare line table entry addresses.  */
-
-static int
-compare_lte (const void *lte1p, const void *lte2p)
-{
-  struct linetable_entry *lte1 = (struct linetable_entry *) lte1p;
-  struct linetable_entry *lte2 = (struct linetable_entry *) lte2p;
-
-  return lte1->pc - lte2->pc;
-}
-
 /* Given a line table with function entries are marked, arrange its
    functions in ascending order and strip off function entry markers
    and return it in a newly created table.  If the old one is good
@@ -471,8 +458,9 @@ 
       return oldLineTb;
     }
   else if (function_count > 1)
-    qsort (fentry, function_count,
-	   sizeof (struct linetable_entry), compare_lte);
+    std::sort (fentry, fentry + function_count,
+	       [] (const linetable_entry &lte1, const linetable_entry& lte2)
+	        { return lte1.pc < lte2.pc; });
 
   /* Allocate a new line table.  */
   newLineTb = (struct linetable *)