[PATCH 07/11] Breakpoints, don't skip prologue of ifunc resolvers with debug info

[Pedro Alves <palves at redhat dot com>]

Without this patch, some of the tests added to gdb.base/gnu-ifunc.exp
by a following patch fail like so:

  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: before resolving: break gnu_ifunc
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: before resolving: info breakpoints
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: after resolving: break gnu_ifunc
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: after resolving: info breakpoints
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: before resolving: break gnu_ifunc
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: before resolving: info breakpoints
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: after resolving: break gnu_ifunc
  FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: after resolving: info breakpoints

All of them trigger iff:

 - you have debug info for the ifunc resolver.
 - the resolver and the user-visible symbol have the same name.

If you have an ifunc that has a resolver with the same name as the
user visible symbol, debug info for the resolver masks out the ifunc
minsym.  When you set a breakpoint by name on the user visible symbol,
GDB finds the DWARF symbol for the resolver, and thinking that it's a
regular function, sets a breakpoint location past its prologue.

Like so, location 1.2, before the ifunc is resolved by the inferior:

  (gdb) break gnu_ifunc
  Breakpoint 2 at 0x7ffff7bd36ea (2 locations)
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
  1.1                         y     0x00007ffff7bd36ea <gnu_ifunc>
  1.2                         y     0x00007ffff7bd36f2 in gnu_ifunc at src/gdb/testsuite/gdb.base/gnu-ifunc-lib.c:34
  (gdb)

And like so, location 2.2, if you set the breakpoint after the ifunc
is resolved by the inferior (to "final"):

  (gdb) break gnu_ifunc
  Breakpoint 5 at 0x400757 (2 locations)
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   <MULTIPLE>
  2.1                         y     0x000000000040075a in final at src/gdb/testsuite/gdb.base/gnu-ifunc-resd.c:21
  2.2                         y     0x00007ffff7bd36f2 in gnu_ifunc at src/gdb/testsuite/gdb.base/gnu-ifunc-lib.c:34
  (gdb)

I don't think this is right because when users set a breakpoint at an
ifunc, they don't care about debugging the resolver.  Instead what you
should is a single location for the ifunc in the first case, and a
single location of the ifunc target in the second case.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* linespec.c (struct bound_minimal_symbol_search_key): New.
	(convert_linespec_to_sals): Sort minimal symbols earlier.  Don't
	skip first line if we found a GNU ifunc minimal symbol by name.
	(compare_msymbols): Change parameters to work with a destructured
	lhs minsym.
	(compare_msymbols_for_qsort, compare_msymbols_for_bsearch): New
	functions.
---
 gdb/linespec.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 101 insertions(+), 20 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 1f1fb695c14..abf31559f54 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -402,7 +402,8 @@ static void minsym_found (struct linespec_state *self, struct objfile *objfile,
 
 static int compare_symbols (const void *a, const void *b);
 
-static int compare_msymbols (const void *a, const void *b);
+static int compare_msymbols_for_qsort (const void *a, const void *b);
+static int compare_msymbols_for_bsearch (const void *a, const void *b);
 
 /* Permitted quote characters for the parser.  This is different from the
    completer's quote characters to allow backward compatibility with the
@@ -2281,6 +2282,20 @@ convert_address_location_to_sals (struct linespec_state *self,
   return sals;
 }
 
+/* Search key passed to compare_msymbols_for_bsearch, a bsearch
+   callback.  */
+struct bound_minimal_symbol_search_key
+{
+  /* The program space.  */
+  program_space *pspace;
+
+  /* The minimal symbol's address.  */
+  CORE_ADDR address;
+
+  /* The minimal symbol's type.  */
+  enum minimal_symbol_type type;
+};
+
 /* Create and return SALs from the linespec LS.  */
 
 static std::vector<symtab_and_line>
@@ -2321,12 +2336,52 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 	  qsort (VEC_address (symbolp, ls->function_symbols),
 		 VEC_length (symbolp, ls->function_symbols),
 		 sizeof (symbolp), compare_symbols);
+	}
+
+      if (ls->minimal_symbols != NULL)
+	{
+	  /* Sort minimal symbols by program space, address, and type.
+	     Do this before the minsym bsearch, below.  */
+	  qsort (VEC_address (bound_minimal_symbol_d, ls->minimal_symbols),
+		 VEC_length (bound_minimal_symbol_d, ls->minimal_symbols),
+		 sizeof (bound_minimal_symbol_d), compare_msymbols_for_qsort);
+	}
 
+      if (ls->function_symbols != NULL)
+	{
 	  for (i = 0; VEC_iterate (symbolp, ls->function_symbols, i, sym); ++i)
 	    {
 	      pspace = SYMTAB_PSPACE (symbol_symtab (sym));
 	      set_current_program_space (pspace);
-	      if (symbol_to_sal (&sal, state->funfirstline, sym)
+
+	      /* Don't skip to the first line of the function if we
+		 had found an ifunc minimal symbol for this function,
+		 because that means that this function is an ifunc
+		 resolver with the same name as the ifunc itself.  */
+	      bound_minimal_symbol *ifunc = NULL;
+
+	      if (state->funfirstline
+		   && ls->minimal_symbols != NULL
+		   && SYMBOL_CLASS (sym) == LOC_BLOCK)
+		{
+		  const CORE_ADDR addr
+		    = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+		  const bound_minimal_symbol_search_key key
+		    = {pspace, addr, mst_text_gnu_ifunc};
+
+		  const bound_minimal_symbol *begin
+		    = VEC_address (bound_minimal_symbol_d, ls->minimal_symbols);
+		  ifunc
+		    = ((bound_minimal_symbol *)
+		       bsearch (&key, begin,
+				VEC_length (bound_minimal_symbol_d,
+					    ls->minimal_symbols),
+				sizeof (bound_minimal_symbol_d),
+				compare_msymbols_for_bsearch));
+		}
+
+	      if (ifunc == NULL
+		  && symbol_to_sal (&sal, state->funfirstline, sym)
 		  && maybe_add_address (state->addr_set, pspace, sal.pc))
 		add_sal_to_sals (state, &sals, &sal,
 				 SYMBOL_NATURAL_NAME (sym), 0);
@@ -2335,11 +2390,6 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 
       if (ls->minimal_symbols != NULL)
 	{
-	  /* Sort minimal symbols by program space, too.  */
-	  qsort (VEC_address (bound_minimal_symbol_d, ls->minimal_symbols),
-		 VEC_length (bound_minimal_symbol_d, ls->minimal_symbols),
-		 sizeof (bound_minimal_symbol_d), compare_msymbols);
-
 	  for (i = 0;
 	       VEC_iterate (bound_minimal_symbol_d, ls->minimal_symbols,
 			    i, elem);
@@ -3662,36 +3712,67 @@ compare_symbols (const void *a, const void *b)
   return 0;
 }
 
-/* Like compare_symbols but for minimal symbols.  */
+/* Helper for compare_symbols_for_qsort and
+   compare_symbols_for_bsearch.  Sorts msymbols by pspace, address and
+   then type.  */
 
 static int
-compare_msymbols (const void *a, const void *b)
+compare_msymbols (const program_space *l_pspace,
+		  CORE_ADDR l_address,
+		  enum minimal_symbol_type l_type,
+		  const bound_minimal_symbol *r_minsym)
 {
-  const struct bound_minimal_symbol *sa
-    = (const struct bound_minimal_symbol *) a;
-  const struct bound_minimal_symbol *sb
-    = (const struct bound_minimal_symbol *) b;
-  uintptr_t uia, uib;
+  const struct bound_minimal_symbol *sb = r_minsym;
 
-  uia = (uintptr_t) sa->objfile->pspace;
-  uib = (uintptr_t) sa->objfile->pspace;
+  uintptr_t uia = (uintptr_t) l_pspace;
+  uintptr_t uib = (uintptr_t) sb->objfile->pspace;
 
   if (uia < uib)
     return -1;
   if (uia > uib)
     return 1;
 
-  uia = (uintptr_t) sa->minsym;
-  uib = (uintptr_t) sb->minsym;
+  if (l_address < BMSYMBOL_VALUE_ADDRESS (*sb))
+    return -1;
+  if (l_address > BMSYMBOL_VALUE_ADDRESS (*sb))
+    return 1;
 
-  if (uia < uib)
+  if (l_type < MSYMBOL_TYPE (sb->minsym))
     return -1;
-  if (uia > uib)
+  if (l_type > MSYMBOL_TYPE (sb->minsym))
     return 1;
 
   return 0;
 }
 
+/* Like compare_symbols but for minimal symbols.  Version suitable for
+   use with qsort.  */
+
+static int
+compare_msymbols_for_qsort (const void *a, const void *b)
+{
+  const auto *sa = (bound_minimal_symbol *) a;
+  const auto *sb = (bound_minimal_symbol *) b;
+
+  return compare_msymbols (sa->objfile->pspace,
+			   BMSYMBOL_VALUE_ADDRESS (*sa),
+			   MSYMBOL_TYPE (sa->minsym),
+			   sb);
+}
+
+/* Like compare_symbols but for minimal symbols.  This version is
+   suitable for use with bsearch with bound_minimal_symbol_search_key
+   as key type.  */
+
+static int
+compare_msymbols_for_bsearch (const void *key, const void *bmsym)
+{
+  const auto *k = (bound_minimal_symbol_search_key *) key;
+  const auto *m = (bound_minimal_symbol *) bmsym;
+
+  return compare_msymbols (k->pspace, k->address, k->type, m);
+}
+
 /* Look for all the matching instances of each symbol in NAMES.  Only
    instances from PSPACE are considered; other program spaces are
    handled by our caller.  If PSPACE is NULL, then all program spaces
-- 
2.14.3