[review] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[review] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[Andrew Burgess (Code Review)]

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

gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

After the switch to use std::sort, if GDB is compiled with the
-D_GLIBCXX_DEBUG=1 flag then we see an error when using sort_cmp (in
objfiles.c) to sort obj_section objects.

The problem is that std::sort checks that the condition !(a < a)
holds, and currently this is not true.  GDB's sort_cmp is really
designed to sort lists in which no obj_section repeats, however, there
is some code in place to try and ensure we get a stable sort order if
there is a bug in GDB, unfortunately this code fails the above check.

By reordering some of the checks inside sort_cmp, it is pretty easy to
ensure that the !(a < a) condition holds.

I've not bothered to make this condition check optimal, like I said
this code is only in place to ensure that we get stable results if GDB
goes wrong, so I've made the smallest change needed to get the correct
behaviour.

After this commit I see no regressions when running GDB compiled with
-D_GLIBCXX_DEBUG=1.

gdb/ChangeLog:

	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.

Change-Id: I4b1e3e1640865104c0896cbb6c3fdbbc04d9645b
---
M gdb/ChangeLog
M gdb/objfiles.c
2 files changed, 14 insertions(+), 6 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6f1a1c0..8dea436 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2019-10-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.
+
 2019-10-21  Tom Tromey  <tom@tromey.com>
 
 	* configure.ac (nm.h): Conditionally create nm.h link.  Subst
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index fd1cbf7..18a3e98 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1044,19 +1044,23 @@
 	 doesn't happen at all).  If you discover that significant time is
 	 spent in the loops below, do 'set complaints 100' and examine the
 	 resulting complaints.  */
-
       if (objfile1 == objfile2)
 	{
-	  /* Both sections came from the same objfile.  We are really confused.
-	     Sort on sequence order of sections within the objfile.  */
+	  /* Both sections came from the same objfile.  We are really
+	     confused.  Sort on sequence order of sections within the
+	     objfile.  The order of checks is important here, if we find a
+	     match on SECT2 first then either SECT2 is before SECT1, or,
+	     SECT2 == SECT1, in both cases we should return false.  The
+	     second case shouldn't occur during normal use, but std::sort
+	     does check that '!(a < a)'.  */
 
 	  const struct obj_section *osect;
 
 	  ALL_OBJFILE_OSECTIONS (objfile1, osect)
-	    if (osect == sect1)
-	      return true;
-	    else if (osect == sect2)
+	    if (osect == sect2)
 	      return false;
+	    else if (osect == sect1)
+	      return true;
 
 	  /* We should have found one of the sections before getting here.  */
 	  gdb_assert_not_reached ("section not found");

[review] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

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


Patch Set 1: Code-Review+1

[review] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

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


Patch Set 1: Code-Review+2

(1 comment)

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

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/177/1/gdb/objfiles.c@1055 
PS1, Line 1055: does check that '!(a < a)'
Not sure if it's worth mentioning it, but to be exact, std::sort check when compiling with libstdc++'s debug mode.

[pushed] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

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

gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

After the switch to use std::sort, if GDB is compiled with the
-D_GLIBCXX_DEBUG=1 flag then we see an error when using sort_cmp (in
objfiles.c) to sort obj_section objects.

The problem is that std::sort checks that the condition !(a < a)
holds, and currently this is not true.  GDB's sort_cmp is really
designed to sort lists in which no obj_section repeats, however, there
is some code in place to try and ensure we get a stable sort order if
there is a bug in GDB, unfortunately this code fails the above check.

By reordering some of the checks inside sort_cmp, it is pretty easy to
ensure that the !(a < a) condition holds.

I've not bothered to make this condition check optimal, like I said
this code is only in place to ensure that we get stable results if GDB
goes wrong, so I've made the smallest change needed to get the correct
behaviour.

After this commit I see no regressions when running GDB compiled with
-D_GLIBCXX_DEBUG=1.

gdb/ChangeLog:

	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.

Change-Id: I4b1e3e1640865104c0896cbb6c3fdbbc04d9645b
---
M gdb/ChangeLog
M gdb/objfiles.c
2 files changed, 14 insertions(+), 6 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 44980f2..41ee329 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2019-10-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.
+
 2019-10-21  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui-winsource.h (tui_exec_info_content): Remove typedef.
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index fd1cbf7..b5bc09f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1044,19 +1044,23 @@
 	 doesn't happen at all).  If you discover that significant time is
 	 spent in the loops below, do 'set complaints 100' and examine the
 	 resulting complaints.  */
-
       if (objfile1 == objfile2)
 	{
-	  /* Both sections came from the same objfile.  We are really confused.
-	     Sort on sequence order of sections within the objfile.  */
+	  /* Both sections came from the same objfile.  We are really
+	     confused.  Sort on sequence order of sections within the
+	     objfile.  The order of checks is important here, if we find a
+	     match on SECT2 first then either SECT2 is before SECT1, or,
+	     SECT2 == SECT1, in both cases we should return false.  The
+	     second case shouldn't occur during normal use, but std::sort
+	     does check that '!(a < a)' when compiled in debug mode.  */
 
 	  const struct obj_section *osect;
 
 	  ALL_OBJFILE_OSECTIONS (objfile1, osect)
-	    if (osect == sect1)
-	      return true;
-	    else if (osect == sect2)
+	    if (osect == sect2)
 	      return false;
+	    else if (osect == sect1)
+	      return true;
 
 	  /* We should have found one of the sections before getting here.  */
 	  gdb_assert_not_reached ("section not found");

[pushed] gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Andrew Burgess.

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

gdb: Ensure that !(a < a) is true in sort_cmp on obj_section objects

After the switch to use std::sort, if GDB is compiled with the
-D_GLIBCXX_DEBUG=1 flag then we see an error when using sort_cmp (in
objfiles.c) to sort obj_section objects.

The problem is that std::sort checks that the condition !(a < a)
holds, and currently this is not true.  GDB's sort_cmp is really
designed to sort lists in which no obj_section repeats, however, there
is some code in place to try and ensure we get a stable sort order if
there is a bug in GDB, unfortunately this code fails the above check.

By reordering some of the checks inside sort_cmp, it is pretty easy to
ensure that the !(a < a) condition holds.

I've not bothered to make this condition check optimal, like I said
this code is only in place to ensure that we get stable results if GDB
goes wrong, so I've made the smallest change needed to get the correct
behaviour.

After this commit I see no regressions when running GDB compiled with
-D_GLIBCXX_DEBUG=1.

gdb/ChangeLog:

	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.

Change-Id: I4b1e3e1640865104c0896cbb6c3fdbbc04d9645b
---
M gdb/ChangeLog
M gdb/objfiles.c
2 files changed, 14 insertions(+), 6 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 44980f2..41ee329 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2019-10-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* objfiles.c (sort_cmp): Ensure that !(a < a) holds true.
+
 2019-10-21  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui-winsource.h (tui_exec_info_content): Remove typedef.
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index fd1cbf7..b5bc09f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1044,19 +1044,23 @@
 	 doesn't happen at all).  If you discover that significant time is
 	 spent in the loops below, do 'set complaints 100' and examine the
 	 resulting complaints.  */
-
       if (objfile1 == objfile2)
 	{
-	  /* Both sections came from the same objfile.  We are really confused.
-	     Sort on sequence order of sections within the objfile.  */
+	  /* Both sections came from the same objfile.  We are really
+	     confused.  Sort on sequence order of sections within the
+	     objfile.  The order of checks is important here, if we find a
+	     match on SECT2 first then either SECT2 is before SECT1, or,
+	     SECT2 == SECT1, in both cases we should return false.  The
+	     second case shouldn't occur during normal use, but std::sort
+	     does check that '!(a < a)' when compiled in debug mode.  */
 
 	  const struct obj_section *osect;
 
 	  ALL_OBJFILE_OSECTIONS (objfile1, osect)
-	    if (osect == sect1)
-	      return true;
-	    else if (osect == sect2)
+	    if (osect == sect2)
 	      return false;
+	    else if (osect == sect1)
+	      return true;
 
 	  /* We should have found one of the sections before getting here.  */
 	  gdb_assert_not_reached ("section not found");