[PATCH 2/2] Make "list ambiguous" show symbol names too

[PATCH 2/2] Make "list ambiguous" show symbol names too

[Pedro Alves]

Currently, with an ambiguous "list first,last", we get:

  (gdb) list bar,main
  Specified first line 'bar' is ambiguous:
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98

This commit makes gdb's output above a bit clearer by printing the
symbol name as well:

  (gdb) list bar,main
  Specified first line 'bar' is ambiguous:
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97, symbol: "bar(A)"
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98, symbol: "bar(B)"

And while at it, makes gdb print the symbol name when actually listing
multiple locations too.  I.e., before (with "set listsize 2"):

  (gdb) list bar
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97
  96
  97      int bar (A) { return 11; }
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98
  97      int bar (A) { return 11; }
  98      int bar (B) { return 22; }

After:

  (gdb) list bar
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97, symbol: "bar(A)"
  96
  97      int bar (A) { return 11; }
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98, symbol: "bar(B)"
  97      int bar (A) { return 11; }
  98      int bar (B) { return 22; }

Currently, the result of decoding a linespec loses information about
the original symbol that was found.  All we end up with is an address.
This makes it difficult to find the original symbol again to get at
its print name.  Fix that by storing a pointer to the symbol in the
sal.  We already store the symtab and obj_section, so it feels like a
natural progression to me.  This avoids having to do any extra symbol
lookup too.

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

	* cli/cli-cmds.c (list_command): Use print_sal_location.
	(print_sal_location): New function.
	(ambiguous_line_spec): Use print_sal_location.
	* linespec.c (symbol_to_sal): Record the symbol in the sal.
	* symtab.c (find_function_start_sal): Likewise.
	* symtab.h (symtab_and_line::symbol): New field.

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

	* gdb.base/list-ambiguous.exp (test_list_ambiguous_symbol): Expect
	symbol names in gdb's output.
	* gdb.cp/overload.exp ("list all overloads"): Likewise.
---
 gdb/cli/cli-cmds.c                        | 29 +++++++++++++++++++++--------
 gdb/linespec.c                            |  2 ++
 gdb/symtab.c                              |  2 ++
 gdb/symtab.h                              |  1 +
 gdb/testsuite/gdb.base/list-ambiguous.exp |  4 ++--
 gdb/testsuite/gdb.cp/overload.exp         |  4 ++--
 6 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index b79ceb2..1dfaab9 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -90,6 +90,8 @@ static void list_command (char *, int);
 
 /* Prototypes for local utility functions */
 
+static void print_sal_location (const symtab_and_line &sal);
+
 static void ambiguous_line_spec (gdb::array_view<const symtab_and_line> sals,
 				 const char *format, ...)
   ATTRIBUTE_PRINTF (2, 3);
@@ -1094,11 +1096,7 @@ list_command (char *arg, int from_tty)
 	  if (first_line < 1)
 	    first_line = 1;
 	  if (sals.size () > 1)
-	    {
-	      printf_filtered (_("file: \"%s\", line number: %d\n"),
-			       symtab_to_filename_for_display (sal.symtab),
-			       sal.line);
-	    }
+	    print_sal_location (sal);
 	  print_source_lines (sal.symtab,
 			      first_line,
 			      first_line + get_lines_to_list (),
@@ -1516,6 +1514,23 @@ alias_command (char *args, int from_tty)
     }
 }
 \f
+/* Print the file / line number / symbol name of the location
+   specified by SAL.  */
+
+static void
+print_sal_location (const symtab_and_line &sal)
+{
+  scoped_restore_current_program_space restore_pspace;
+  set_current_program_space (sal.pspace);
+
+  const char *sym_name = NULL;
+  if (sal.symbol != NULL)
+    sym_name = SYMBOL_PRINT_NAME (sal.symbol);
+  printf_filtered (_("file: \"%s\", line number: %d, symbol: \"%s\"\n"),
+		   symtab_to_filename_for_display (sal.symtab),
+		   sal.line, sym_name != NULL ? sym_name : "???");
+}
+
 /* Print a list of files and line numbers which a user may choose from
    in order to list a function which was specified ambiguously (as
    with `list classname::overloadedfuncname', for example).  The SALS
@@ -1533,9 +1548,7 @@ ambiguous_line_spec (gdb::array_view<const symtab_and_line> sals,
   va_end (ap);
 
   for (const auto &sal : sals)
-    printf_filtered (_("file: \"%s\", line number: %d\n"),
-		     symtab_to_filename_for_display (sal.symtab),
-		     sal.line);
+    print_sal_location (sal);
 }
 
 /* Comparison function for filter_sals.  Returns a qsort-style
diff --git a/gdb/linespec.c b/gdb/linespec.c
index d72d19d..9398e9a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4627,6 +4627,7 @@ symbol_to_sal (struct symtab_and_line *result,
 	{
 	  *result = {};
 	  result->symtab = symbol_symtab (sym);
+	  result->symbol = sym;
 	  result->line = SYMBOL_LINE (sym);
 	  result->pc = SYMBOL_VALUE_ADDRESS (sym);
 	  result->pspace = SYMTAB_PSPACE (result->symtab);
@@ -4642,6 +4643,7 @@ symbol_to_sal (struct symtab_and_line *result,
 	  /* We know its line number.  */
 	  *result = {};
 	  result->symtab = symbol_symtab (sym);
+	  result->symbol = sym;
 	  result->line = SYMBOL_LINE (sym);
 	  result->pc = SYMBOL_VALUE_ADDRESS (sym);
 	  result->pspace = SYMTAB_PSPACE (result->symtab);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8492315..cea26b8 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3463,6 +3463,7 @@ find_function_start_sal (struct symbol *sym, int funfirstline)
   obj_section *section = SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym);
   symtab_and_line sal
     = find_pc_sect_line (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), section, 0);
+  sal.symbol = sym;
 
   if (funfirstline && sal.symtab != NULL
       && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
@@ -3486,6 +3487,7 @@ find_function_start_sal (struct symbol *sym, int funfirstline)
       sal.pspace = current_program_space;
       sal.pc = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
       sal.section = section;
+      sal.symbol = sym;
     }
 
   if (funfirstline)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 1bf77c1..8b429a8 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1421,6 +1421,7 @@ struct symtab_and_line
   struct program_space *pspace = NULL;
 
   struct symtab *symtab = NULL;
+  struct symbol *symbol = NULL;
   struct obj_section *section = NULL;
   /* Line number.  Line numbers start at 1 and proceed through symtab->nlines.
      0 is never a valid line number; it is used to indicate that line number
diff --git a/gdb/testsuite/gdb.base/list-ambiguous.exp b/gdb/testsuite/gdb.base/list-ambiguous.exp
index dd473ca..ace3494 100644
--- a/gdb/testsuite/gdb.base/list-ambiguous.exp
+++ b/gdb/testsuite/gdb.base/list-ambiguous.exp
@@ -48,8 +48,8 @@ proc test_list_ambiguous_symbol {symbol_line symbol} {
     set lines1_re [line_range_pattern [expr $lineno1 - 5] [expr $lineno1 + 4]]
 
     set any "\[^\r\n\]*"
-    set h0_re "file: \"${any}list-ambiguous0.c\", line number: $lineno0"
-    set h1_re "file: \"${any}list-ambiguous1.c\", line number: $lineno1"
+    set h0_re "file: \"${any}list-ambiguous0.c\", line number: $lineno0, symbol: \"$symbol\""
+    set h1_re "file: \"${any}list-ambiguous1.c\", line number: $lineno1, symbol: \"$symbol\""
     gdb_test "list $symbol" "${h0_re}${lines0_re}\r\n${h1_re}${lines1_re}"
 
     gdb_test "list main,$symbol" \
diff --git a/gdb/testsuite/gdb.cp/overload.exp b/gdb/testsuite/gdb.cp/overload.exp
index 8cb9311..3bf2a6a 100644
--- a/gdb/testsuite/gdb.cp/overload.exp
+++ b/gdb/testsuite/gdb.cp/overload.exp
@@ -351,8 +351,8 @@ with_test_prefix "list all overloads" {
     set lines2 [line_range_pattern [expr $line_bar_B - 5] [expr $line_bar_B + 4]]
 
     set any "\[^\r\n\]*"
-    set h1_re "file: \"${any}overload.cc\", line number: $line_bar_A"
-    set h2_re "file: \"${any}overload.cc\", line number: $line_bar_B"
+    set h1_re "file: \"${any}overload.cc\", line number: $line_bar_A, symbol: \"bar\\(A\\)\""
+    set h2_re "file: \"${any}overload.cc\", line number: $line_bar_B, symbol: \"bar\\(B\\)\""
     gdb_test "list bar" "${h1_re}${lines1}\r\n${h2_re}${lines2}"
 }
 
-- 
2.5.5

[PATCH 0/2] Make "list ambiguous" show symbol names too

[Pedro Alves]

This series makes "list ambiguous" a bit clearer by printing the
symbol name for each of the ambiguous locations as well:

  (gdb) list bar,main
  Specified first line 'bar' is ambiguous:
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97, symbol: "bar(A)"
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98, symbol: "bar(B)"
                                                               ^^^^^^^^^^^^^^^^^

  (gdb) list bar
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 97, symbol: "bar(A)"
  96
  97      int bar (A) { return 11; }
  file: "src/gdb/testsuite/gdb.cp/overload.cc", line number: 98, symbol: "bar(B)"
  97      int bar (A) { return 11; }
  98      int bar (B) { return 22; }
                                                               ^^^^^^^^^^^^^^^^^


The "symbol: ..." parts above are new.  That's patch #2.

While working on that, I decided to test also the case of "list
ambiguous_global_variable", which exposed the fact that that doesn't
really work...  So I fixed it too.  That's patch #1.

Tested on x86-64 Fedora 23.

Pedro Alves (2):
  Fix "list ambiguous_variable"
  Make "list ambiguous" show symbol names too

 gdb/cli/cli-cmds.c                        | 29 ++++++++++----
 gdb/linespec.c                            | 63 +++++++++++++++++++------------
 gdb/symtab.c                              |  2 +
 gdb/symtab.h                              |  1 +
 gdb/testsuite/gdb.base/list-ambiguous.exp | 41 +++++++++++---------
 gdb/testsuite/gdb.base/list-ambiguous0.c  |  5 ++-
 gdb/testsuite/gdb.base/list-ambiguous1.c  |  5 ++-
 gdb/testsuite/gdb.cp/overload.exp         |  4 +-
 8 files changed, 93 insertions(+), 57 deletions(-)

-- 
2.5.5

[PATCH 1/2] Fix "list ambiguous_variable"

[Pedro Alves]

The "list" command allows specifying the name of variables as
argument, not just functions, so that users can type "list
a_global_variable".

That support is a broken when it comes to ambiguous locations though.

If there's more than one such global variable in the program, e.g.,
static globals in different compilation units, GDB ends up listing the
source of the first variable it finds, only.

linespec.c does find both symbol and minsym locations for all the
globals.  The problem is that it ends up merging all the resulting
sals into one, because they all have address, zero.  I.e., all sals
end up with sal.pc == 0, so maybe_add_address returns false for all
but the first.

The zero addresses appear because:

- in the minsyms case, linespec.c:minsym_found incorrectly treats all
  minsyms as if they were function/text symbols.  In list mode we can
  end up with data symbols there, and we shouldn't be using
  find_pc_sect_line on data symbols.

- in the debug symbols case, symbol_to_sal misses recording an address
  (sal.pc) for non-block, non-label symbols.

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

	* linespec.c (minsym_found): Handle non-text minsyms.
	(symbol_to_sal): Record a sal.pc for non-block, non-label symbols.

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

	* gdb.base/list-ambiguous.exp (test_list_ambiguous_function):
	Rename to ...
	(test_list_ambiguous_symbol): ... this and add a symbol name
	parameter.  Adjust.
	(test_list_ambiguous_function): Reimplement on top of
	test_list_ambiguous_symbol and also test listing ambiguous
	variables.
	* gdb.base/list-ambiguous0.c (ambiguous): Rename to ...
	(ambiguous_fun): ... this.
	(ambiguous_var): New.
	* gdb.base/list-ambiguous1.c (ambiguous): Rename to ...
	(ambiguous_fun): ... this.
	(ambiguous_var): New.
---
 gdb/linespec.c                            | 61 ++++++++++++++++++-------------
 gdb/testsuite/gdb.base/list-ambiguous.exp | 37 +++++++++++--------
 gdb/testsuite/gdb.base/list-ambiguous0.c  |  5 ++-
 gdb/testsuite/gdb.base/list-ambiguous1.c  |  5 ++-
 4 files changed, 63 insertions(+), 45 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 4801808..d72d19d 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4318,35 +4318,45 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
   CORE_ADDR pc;
   struct symtab_and_line sal;
 
-  sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
-			   (struct obj_section *) 0, 0);
-  sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
+  if (msymbol_is_text (msymbol))
+    {
+      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
+			       (struct obj_section *) 0, 0);
+      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
 
-  /* The minimal symbol might point to a function descriptor;
-     resolve it to the actual code address instead.  */
-  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
-  if (pc != sal.pc)
-    sal = find_pc_sect_line (pc, NULL, 0);
+      /* The minimal symbol might point to a function descriptor;
+	 resolve it to the actual code address instead.  */
+      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
+      if (pc != sal.pc)
+	sal = find_pc_sect_line (pc, NULL, 0);
 
-  if (self->funfirstline)
-    {
-      if (sal.symtab != NULL
-	  && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
-	      || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
+      if (self->funfirstline)
 	{
-	  /* If gdbarch_convert_from_func_ptr_addr does not apply then
-	     sal.SECTION, sal.LINE&co. will stay correct from above.
-	     If gdbarch_convert_from_func_ptr_addr applies then
-	     sal.SECTION is cleared from above and sal.LINE&co. will
-	     stay correct from the last find_pc_sect_line above.  */
-	  sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
-	  sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
-						       &current_target);
-	  if (gdbarch_skip_entrypoint_p (gdbarch))
-	    sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+	  if (sal.symtab != NULL
+	      && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
+		  || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
+	    {
+	      /* If gdbarch_convert_from_func_ptr_addr does not apply then
+		 sal.SECTION, sal.LINE&co. will stay correct from above.
+		 If gdbarch_convert_from_func_ptr_addr applies then
+		 sal.SECTION is cleared from above and sal.LINE&co. will
+		 stay correct from the last find_pc_sect_line above.  */
+	      sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+	      sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
+							   &current_target);
+	      if (gdbarch_skip_entrypoint_p (gdbarch))
+		sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+	    }
+	  else
+	    skip_prologue_sal (&sal);
 	}
-      else
-	skip_prologue_sal (&sal);
+    }
+  else
+    {
+      sal.objfile = objfile;
+      sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+      sal.pspace = current_program_space;
+      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
     }
 
   if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
@@ -4633,6 +4643,7 @@ symbol_to_sal (struct symtab_and_line *result,
 	  *result = {};
 	  result->symtab = symbol_symtab (sym);
 	  result->line = SYMBOL_LINE (sym);
+	  result->pc = SYMBOL_VALUE_ADDRESS (sym);
 	  result->pspace = SYMTAB_PSPACE (result->symtab);
 	  return 1;
 	}
diff --git a/gdb/testsuite/gdb.base/list-ambiguous.exp b/gdb/testsuite/gdb.base/list-ambiguous.exp
index db9d028..dd473ca 100644
--- a/gdb/testsuite/gdb.base/list-ambiguous.exp
+++ b/gdb/testsuite/gdb.base/list-ambiguous.exp
@@ -39,36 +39,41 @@ proc line_range_pattern { range_start range_end } {
 # Test the "list" command with linespecs that expand to multiple
 # locations.
 
-proc test_list_ambiguous_function {} {
+proc test_list_ambiguous_symbol {symbol_line symbol} {
     global srcfile srcfile2
 
-    set lineno0 [gdb_get_line_number "ambiguous (void)" $srcfile]
-    set lineno1 [gdb_get_line_number "ambiguous (void)" $srcfile2]
+    set lineno0 [gdb_get_line_number $symbol_line $srcfile]
+    set lineno1 [gdb_get_line_number $symbol_line $srcfile2]
     set lines0_re [line_range_pattern [expr $lineno0 - 5] [expr $lineno0 + 4]]
     set lines1_re [line_range_pattern [expr $lineno1 - 5] [expr $lineno1 + 4]]
 
     set any "\[^\r\n\]*"
     set h0_re "file: \"${any}list-ambiguous0.c\", line number: $lineno0"
     set h1_re "file: \"${any}list-ambiguous1.c\", line number: $lineno1"
-    gdb_test "list ambiguous" "${h0_re}${lines0_re}\r\n${h1_re}${lines1_re}"
-
-    gdb_test "list main,ambiguous" \
-	"Specified last line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
-    gdb_test "list ,ambiguous" \
-	"Specified last line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
-    gdb_test "list ambiguous,main" \
-	"Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
-    gdb_test "list ambiguous,ambiguous" \
-	"Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
-    gdb_test "list ambiguous," \
-	"Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+    gdb_test "list $symbol" "${h0_re}${lines0_re}\r\n${h1_re}${lines1_re}"
+
+    gdb_test "list main,$symbol" \
+	"Specified last line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+    gdb_test "list ,$symbol" \
+	"Specified last line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+    gdb_test "list $symbol,main" \
+	"Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+    gdb_test "list $symbol,$symbol" \
+	"Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+    gdb_test "list $symbol," \
+	"Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
 
     # While at it, test the "edit" command as well, since it shares
     # code with "list".
-    gdb_test "edit ambiguous" \
+    gdb_test "edit $symbol" \
 	"Specified line is ambiguous:\r\n${h0_re}\r\n${h1_re}"
 }
 
+proc test_list_ambiguous_function {} {
+    test_list_ambiguous_symbol "ambiguous_fun (void)" "ambiguous_fun"
+    test_list_ambiguous_symbol "ambiguous_var" "ambiguous_var"
+}
+
 gdb_test_no_output "set listsize 10"
 
 test_list_ambiguous_function
diff --git a/gdb/testsuite/gdb.base/list-ambiguous0.c b/gdb/testsuite/gdb.base/list-ambiguous0.c
index 91f7fe9..afd9457 100644
--- a/gdb/testsuite/gdb.base/list-ambiguous0.c
+++ b/gdb/testsuite/gdb.base/list-ambiguous0.c
@@ -19,12 +19,13 @@
 
 
 
-/* This function is defined in both
+/* These symbols are defined in both
    list-ambiguous0.c/list-ambiguous1.c files, in order to test
    "list"'s behavior with ambiguous linespecs.  */
 
-static void __attribute__ ((used)) ambiguous (void) {}
+static void __attribute__ ((used)) ambiguous_fun (void) {}
 
+static int ambiguous_var;
 
 
 
diff --git a/gdb/testsuite/gdb.base/list-ambiguous1.c b/gdb/testsuite/gdb.base/list-ambiguous1.c
index 024299a..69db11e 100644
--- a/gdb/testsuite/gdb.base/list-ambiguous1.c
+++ b/gdb/testsuite/gdb.base/list-ambiguous1.c
@@ -23,11 +23,12 @@
 
 
 
-/* This function is defined in both
+/* These symbols are defined in both
    list-ambiguous0.c/list-ambiguous1.c files, in order to test
    "list"'s behavior with ambiguous linespecs.  */
-static void __attribute__ ((used)) ambiguous (void) {}
+static void __attribute__ ((used)) ambiguous_fun (void) {}
 
+static int ambiguous_var;
 
 
 
-- 
2.5.5

Re: [PATCH 1/2] Fix "list ambiguous_variable"

[Keith Seitz]

On 09/04/2017 11:47 AM, Pedro Alves wrote:
> The "list" command allows specifying the name of variables as
> argument, not just functions, so that users can type "list
> a_global_variable".
> 
> That support is a broken when it comes to ambiguous locations though.

Very nice!

FWIW, I have only one trivial nit (below).

Keith

PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!

(gdb) set listsize 3
(gdb) list ambiguous
file: "amb1.c", line number: 5, symbol: "ambiguous"
4	
5	static int ambiguous = 0;
6	
file: "amb2.c", line number: 3, symbol: "ambiguous"
2	ambiguous (void)
3	{
4	  return 0;
file: "amb3.c", line number: 3, symbol: "ambiguous"
2	ambiguous (void)
3	{
4	  return 0;
file: "amb4.c", line number: 1, symbol: "ambiguous"
1	static int ambiguous = 0;
2	
3	int


> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 4801808..d72d19d 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4318,35 +4318,45 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
>    CORE_ADDR pc;
>    struct symtab_and_line sal;
>  
> -  sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
> -			   (struct obj_section *) 0, 0);
> -  sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
> +  if (msymbol_is_text (msymbol))
> +    {
> +      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
> +			       (struct obj_section *) 0, 0);
> +      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
>  
> -  /* The minimal symbol might point to a function descriptor;
> -     resolve it to the actual code address instead.  */
> -  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
> -  if (pc != sal.pc)
> -    sal = find_pc_sect_line (pc, NULL, 0);
> +      /* The minimal symbol might point to a function descriptor;
> +	 resolve it to the actual code address instead.  */
> +      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);

This line exceeds the 80-char line length limit.

> +      if (pc != sal.pc)
> +	sal = find_pc_sect_line (pc, NULL, 0);
>  
> -  if (self->funfirstline)
> -    {
> -      if (sal.symtab != NULL
> -	  && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
> -	      || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
> +      if (self->funfirstline)
>  	{
> -	  /* If gdbarch_convert_from_func_ptr_addr does not apply then
> -	     sal.SECTION, sal.LINE&co. will stay correct from above.
> -	     If gdbarch_convert_from_func_ptr_addr applies then
> -	     sal.SECTION is cleared from above and sal.LINE&co. will
> -	     stay correct from the last find_pc_sect_line above.  */
> -	  sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> -	  sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
> -						       &current_target);
> -	  if (gdbarch_skip_entrypoint_p (gdbarch))
> -	    sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> +	  if (sal.symtab != NULL
> +	      && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
> +		  || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
> +	    {
> +	      /* If gdbarch_convert_from_func_ptr_addr does not apply then
> +		 sal.SECTION, sal.LINE&co. will stay correct from above.
> +		 If gdbarch_convert_from_func_ptr_addr applies then
> +		 sal.SECTION is cleared from above and sal.LINE&co. will
> +		 stay correct from the last find_pc_sect_line above.  */
> +	      sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> +	      sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
> +							   &current_target);
> +	      if (gdbarch_skip_entrypoint_p (gdbarch))
> +		sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> +	    }
> +	  else
> +	    skip_prologue_sal (&sal);
>  	}
> -      else
> -	skip_prologue_sal (&sal);
> +    }
> +  else
> +    {
> +      sal.objfile = objfile;
> +      sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> +      sal.pspace = current_program_space;
> +      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
>      }
>  
>    if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))

Re: [PATCH 2/2] Make "list ambiguous" show symbol names too

[Keith Seitz]

On 09/04/2017 11:47 AM, Pedro Alves wrote:
> 
> Fix that by storing a pointer to the symbol in the sal.

:-)

> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* cli/cli-cmds.c (list_command): Use print_sal_location.
> 	(print_sal_location): New function.
> 	(ambiguous_line_spec): Use print_sal_location.
> 	* linespec.c (symbol_to_sal): Record the symbol in the sal.
> 	* symtab.c (find_function_start_sal): Likewise.
> 	* symtab.h (symtab_and_line::symbol): New field.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.base/list-ambiguous.exp (test_list_ambiguous_symbol): Expect
> 	symbol names in gdb's output.
> 	* gdb.cp/overload.exp ("list all overloads"): Likewise.

FWIW, I looked this over and didn't see any problems.

Keith

Re: [PATCH 1/2] Fix "list ambiguous_variable"

[Pedro Alves]

On 09/06/2017 07:41 PM, Keith Seitz wrote:
> On 09/04/2017 11:47 AM, Pedro Alves wrote:
>> The "list" command allows specifying the name of variables as
>> argument, not just functions, so that users can type "list
>> a_global_variable".
>>
>> That support is a broken when it comes to ambiguous locations though.
> 
> Very nice!
> 
> FWIW, I have only one trivial nit (below).
> 
> Keith
> 
> PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!

Awesome.  :-)

> 
> (gdb) set listsize 3
> (gdb) list ambiguous
> file: "amb1.c", line number: 5, symbol: "ambiguous"
> 4	
> 5	static int ambiguous = 0;
> 6	
> file: "amb2.c", line number: 3, symbol: "ambiguous"
> 2	ambiguous (void)
> 3	{
> 4	  return 0;
> file: "amb3.c", line number: 3, symbol: "ambiguous"
> 2	ambiguous (void)
> 3	{
> 4	  return 0;
> file: "amb4.c", line number: 1, symbol: "ambiguous"
> 1	static int ambiguous = 0;
> 2	
> 3	int
> 

>> -  /* The minimal symbol might point to a function descriptor;
>> -     resolve it to the actual code address instead.  */
>> -  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
>> -  if (pc != sal.pc)
>> -    sal = find_pc_sect_line (pc, NULL, 0);
>> +      /* The minimal symbol might point to a function descriptor;
>> +	 resolve it to the actual code address instead.  */
>> +      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
> 
> This line exceeds the 80-char line length limit.

Indeed it does.  Fixed before pushing (both patches).

Thanks for the review!

-- 
Pedro Alves

Re: [PATCH 1/2] Fix "list ambiguous_variable"

[Simon Marchi]

On 2017-09-20 11:25 AM, Pedro Alves wrote:
> On 09/06/2017 07:41 PM, Keith Seitz wrote:
>> On 09/04/2017 11:47 AM, Pedro Alves wrote:
>>> The "list" command allows specifying the name of variables as
>>> argument, not just functions, so that users can type "list
>>> a_global_variable".
>>>
>>> That support is a broken when it comes to ambiguous locations though.
>>
>> Very nice!
>>
>> FWIW, I have only one trivial nit (below).
>>
>> Keith
>>
>> PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!
> 
> Awesome.  :-)
> 
>>
>> (gdb) set listsize 3
>> (gdb) list ambiguous
>> file: "amb1.c", line number: 5, symbol: "ambiguous"
>> 4	
>> 5	static int ambiguous = 0;
>> 6	
>> file: "amb2.c", line number: 3, symbol: "ambiguous"
>> 2	ambiguous (void)
>> 3	{
>> 4	  return 0;
>> file: "amb3.c", line number: 3, symbol: "ambiguous"
>> 2	ambiguous (void)
>> 3	{
>> 4	  return 0;
>> file: "amb4.c", line number: 1, symbol: "ambiguous"
>> 1	static int ambiguous = 0;
>> 2	
>> 3	int
>>
> 
>>> -  /* The minimal symbol might point to a function descriptor;
>>> -     resolve it to the actual code address instead.  */
>>> -  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
>>> -  if (pc != sal.pc)
>>> -    sal = find_pc_sect_line (pc, NULL, 0);
>>> +      /* The minimal symbol might point to a function descriptor;
>>> +	 resolve it to the actual code address instead.  */
>>> +      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
>>
>> This line exceeds the 80-char line length limit.
> 
> Indeed it does.  Fixed before pushing (both patches).
> 
> Thanks for the review!
> 

Hi Pedro,

The buildbot shows some failures on ppc64be:

PASS -> FAIL: gdb.base/dbx.exp: whereis my_list
PASS -> FAIL: gdb.mi/gdb669.exp: -thread-list-ids
PASS -> FAIL: gdb.mi/gdb669.exp: finding MI result string
PASS -> FAIL: gdb.mi/gdb669.exp: finding number of threads in MI output

I tested on gcc110, and bisect points to this patch here (for both tests).
A symptom of the problem is that "break main" generates two locations.

Before (at e5f25bc5^):

(gdb) b main
Breakpoint 1 at 0x10000560: file test.c, line 3.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000010000560 in main at test.c:3

After (at e5f25bc5)

(gdb) b main
Breakpoint 1 at 0x10000560: main. (2 locations)
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   <MULTIPLE>
1.1                         y     0x0000000010000560 in main at test.c:3
1.2                         y     0x0000000010020078 <main>

Simon

ppc64 regression: [PATCH 1/2] Fix "list ambiguous_variable"

[Jan Kratochvil]

On Mon, 16 Oct 2017 17:03:18 +0200, Simon Marchi wrote:
> The buildbot shows some failures on ppc64be:
> 
> PASS -> FAIL: gdb.base/dbx.exp: whereis my_list
> PASS -> FAIL: gdb.mi/gdb669.exp: -thread-list-ids
> PASS -> FAIL: gdb.mi/gdb669.exp: finding MI result string
> PASS -> FAIL: gdb.mi/gdb669.exp: finding number of threads in MI output
> 
> I tested on gcc110, and bisect points to this patch here (for both tests).
> A symptom of the problem is that "break main" generates two locations.
> 
> Before (at e5f25bc5^):
> 
> (gdb) b main
> Breakpoint 1 at 0x10000560: file test.c, line 3.
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x0000000010000560 in main at test.c:3
> 
> After (at e5f25bc5)
> 
> (gdb) b main
> Breakpoint 1 at 0x10000560: main. (2 locations)
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   <MULTIPLE>
> 1.1                         y     0x0000000010000560 in main at test.c:3
> 1.2                         y     0x0000000010020078 <main>

I have also hit this regression now on RHEL-7.5pre ppc64:

e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea is the first bad commit
commit e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Sep 20 16:12:54 2017 +0100
    Fix "list ambiguous_variable"

echo 'main(){}'|gcc -g -x c -;./gdb -batch ./a.out -ex start
	Temporary breakpoint 1 at 0x10000560: file <stdin>, line 1.
	Temporary breakpoint 1, main () at <stdin>:1
	1       <stdin>: No such file or directory.
->
	Temporary breakpoint 1 at 0x10000560: main. (2 locations)
	Program received signal SIGSEGV, Segmentation fault.
	0x7d82100810000554 in ?? ()


Jan

Re: ppc64 regression: [PATCH 1/2] Fix "list ambiguous_variable"

[Ulrich Weigand]

Jan Kratochvil wrote:
> On Mon, 16 Oct 2017 17:03:18 +0200, Simon Marchi wrote:
> > The buildbot shows some failures on ppc64be:
> > 
> > PASS -> FAIL: gdb.base/dbx.exp: whereis my_list
> > PASS -> FAIL: gdb.mi/gdb669.exp: -thread-list-ids
> > PASS -> FAIL: gdb.mi/gdb669.exp: finding MI result string
> > PASS -> FAIL: gdb.mi/gdb669.exp: finding number of threads in MI output
> > 
> > I tested on gcc110, and bisect points to this patch here (for both tests).
> > A symptom of the problem is that "break main" generates two locations.
> > 
> > Before (at e5f25bc5^):
> > 
> > (gdb) b main
> > Breakpoint 1 at 0x10000560: file test.c, line 3.
> > (gdb) info breakpoints
> > Num     Type           Disp Enb Address            What
> > 1       breakpoint     keep y   0x0000000010000560 in main at test.c:3
> > 
> > After (at e5f25bc5)
> > 
> > (gdb) b main
> > Breakpoint 1 at 0x10000560: main. (2 locations)
> > (gdb) info breakpoints
> > Num     Type           Disp Enb Address            What
> > 1       breakpoint     keep y   <MULTIPLE>
> > 1.1                         y     0x0000000010000560 in main at test.c:3
> > 1.2                         y     0x0000000010020078 <main>
> 
> I have also hit this regression now on RHEL-7.5pre ppc64:
> 
> e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea is the first bad commit
> commit e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea
> Author: Pedro Alves <palves@redhat.com>
> Date:   Wed Sep 20 16:12:54 2017 +0100
>     Fix "list ambiguous_variable"
> 
> echo 'main(){}'|gcc -g -x c -;./gdb -batch ./a.out -ex start
> 	Temporary breakpoint 1 at 0x10000560: file <stdin>, line 1.
> 	Temporary breakpoint 1, main () at <stdin>:1
> 	1       <stdin>: No such file or directory.
> ->
> 	Temporary breakpoint 1 at 0x10000560: main. (2 locations)
> 	Program received signal SIGSEGV, Segmentation fault.
> 	0x7d82100810000554 in ?? ()

I now see this as well on Cell/B.E.  This is a serious regression that
causes "start" to always fail for me ...

The problem seems to be that GDB sets a breakpoint into the function
descriptor for main, which is not a good idea.

Looking at the commit identified above, it seems that GDB now only
runs the address through gdbarch_convert_from_func_ptr_addr if
msymbol_is_text returns true.  However, if the symbol points to
a function descriptor, msymbol_is_text would be false since this
is in fact outside the text section.

So I think probably we need to still run the address through
gdbarch_convert_from_func_ptr_addr, and if that detects that it
was indeed a function descriptor, always treat the resulting
address as a function.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors) (was: Re: ppc64 regression: [PATCH 1/2] Fix "list ambiguous_variable")

[Pedro Alves]

On 11/26/2017 04:37 PM, Ulrich Weigand wrote:

> I now see this as well on Cell/B.E.  This is a serious regression that
> causes "start" to always fail for me ...
> 
> The problem seems to be that GDB sets a breakpoint into the function
> descriptor for main, which is not a good idea.
> 
> Looking at the commit identified above, it seems that GDB now only
> runs the address through gdbarch_convert_from_func_ptr_addr if
> msymbol_is_text returns true.  However, if the symbol points to
> a function descriptor, msymbol_is_text would be false since this
> is in fact outside the text section.
> 
> So I think probably we need to still run the address through
> gdbarch_convert_from_func_ptr_addr, and if that detects that it
> was indeed a function descriptor, always treat the resulting
> address as a function.

Thanks for the suggestion.  Here's a patch that implements
something like that.  WDYT?

I've pushed this to the users/palves/fix-ppc64-func-descriptors
branch as well.

From d1e2a9dc61a645ff29606f01f96a02b477761c23 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 29 Nov 2017 18:06:33 +0000
Subject: [PATCH] Fix setting-breakpoints regression on PPC64 (function
 descriptors)

The recent-ish commit e5f25bc5d6db ('Fix "list ambiguous_variable"')
caused a serious regression on PPC64.  See
<https://sourceware.org/ml/gdb-patches/2017-11/msg00666.html>.

Basically, after that patch, GDB sets breakpoints in function
descriptors instead of where the descriptors point to, which is
incorrect.

The problem is that GDB now only runs a minsym's address through
gdbarch_convert_from_func_ptr_addr if msymbol_is_text returns true.
However, if the symbol points to a function descriptor,
msymbol_is_text is false since function descriptors are in fact
outside the text section.

The fix is to also run a non-text address through
gdbarch_convert_from_func_ptr_addr, and if that detects that it was
indeed a function descriptor, treat the resulting address as a
function.

While implementing that directly in linespec.c:minsym_found (where the
bad msymbol_is_text check is) fixes the issue, I noticed that
linespec.c:add_minsym has some code that also basically needs to do
the same checks, however it's implemented differently.  Also,
add_minsym is calling find_pc_sect_line on non-function symbols, which
also doesn't look right.

So I introduced msymbol_is_function, so that we have a simple place to
consider minsyms and function descriptors.

And then, the only other use of msymbol_is_text is in
find_function_alias_target, which turns out to also be incorrect.
Changing that one to use msymbol_is_function, i.e., to consider
function descriptors too fixes (on PPC64):

  -FAIL: gdb.base/symbol-alias.exp: p func_alias
  -FAIL: gdb.base/symbol-alias.exp: p *func_alias()
  +PASS: gdb.base/symbol-alias.exp: p func_alias
  +PASS: gdb.base/symbol-alias.exp: p *func_alias()

And then after that, msymbol_is_text is no longer used anywhere, so it
can be removed.

Tested on x86_64 GNU/Linux, no regressions.  Tested on PPC64 GNU/Linux
and results compared to a testrun of e5f25bc5d6db^ (before the
offending commit), also no regressions.  (there's a couple new FAILs
and some new symbol name matching unit tests are crashing, but that
looks unrelated).

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

	* linespec.c (minsym_found, add_minsym): Use msymbol_is_function.
	* minsyms.c (msymbol_is_text): Delete.
	(msymbol_is_function): New function.
	* minsyms.h (msymbol_is_text): Delete.
	(msymbol_is_function): New declaration.
	* symtab.c (find_function_alias_target): Use msymbol_is_function.
---
 gdb/linespec.c | 83 ++++++++++++++++------------------------------------------
 gdb/minsyms.c  | 34 ++++++++++++++++++------
 gdb/minsyms.h  | 11 +++++---
 gdb/symtab.c   |  8 +++---
 4 files changed, 61 insertions(+), 75 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 3f7f171..83f9bbf 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4309,22 +4309,16 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
 	      struct minimal_symbol *msymbol,
 	      std::vector<symtab_and_line> *result)
 {
-  struct gdbarch *gdbarch = get_objfile_arch (objfile);
-  CORE_ADDR pc;
   struct symtab_and_line sal;
 
-  if (msymbol_is_text (msymbol))
-    {
-      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
-			       (struct obj_section *) 0, 0);
-      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
+  /* The minimal symbol might point to a function descriptor, which is
+     not a text symbol; try resolving it to the actual code
+     address.  */
 
-      /* The minimal symbol might point to a function descriptor;
-	 resolve it to the actual code address instead.  */
-      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
-					       &current_target);
-      if (pc != sal.pc)
-	sal = find_pc_sect_line (pc, NULL, 0);
+  CORE_ADDR func_addr;
+  if (msymbol_is_function (objfile, msymbol, &func_addr))
+    {
+      sal = find_pc_sect_line (func_addr, NULL, 0);
 
       if (self->funfirstline)
 	{
@@ -4332,14 +4326,9 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
 	      && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
 		  || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
 	    {
-	      /* If gdbarch_convert_from_func_ptr_addr does not apply then
-		 sal.SECTION, sal.LINE&co. will stay correct from above.
-		 If gdbarch_convert_from_func_ptr_addr applies then
-		 sal.SECTION is cleared from above and sal.LINE&co. will
-		 stay correct from the last find_pc_sect_line above.  */
-	      sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
-	      sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
-							   &current_target);
+	      struct gdbarch *gdbarch = get_objfile_arch (objfile);
+
+	      sal.pc = func_addr;
 	      if (gdbarch_skip_entrypoint_p (gdbarch))
 		sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
 	    }
@@ -4424,52 +4413,26 @@ static void
 add_minsym (struct minimal_symbol *minsym, void *d)
 {
   struct collect_minsyms *info = (struct collect_minsyms *) d;
-  bound_minimal_symbol_d mo;
-
-  mo.minsym = minsym;
-  mo.objfile = info->objfile;
 
   if (info->symtab != NULL)
     {
-      CORE_ADDR pc;
-      struct symtab_and_line sal;
-      struct gdbarch *gdbarch = get_objfile_arch (info->objfile);
-
-      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (info->objfile, minsym),
-			       NULL, 0);
-      sal.section = MSYMBOL_OBJ_SECTION (info->objfile, minsym);
-      pc
-	= gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, &current_target);
-      if (pc != sal.pc)
-	sal = find_pc_sect_line (pc, NULL, 0);
+      /* We're looking for a label for which we don't have debug
+	 info.  */
+      CORE_ADDR func_addr;
+      if (msymbol_is_function (info->objfile, minsym, &func_addr))
+	{
+	  symtab_and_line sal = find_pc_sect_line (func_addr, NULL, 0);
 
-      if (info->symtab != sal.symtab)
-	return;
+	  if (info->symtab != sal.symtab)
+	    return;
+	}
     }
 
-  /* Exclude data symbols when looking for breakpoint locations.   */
-  if (!info->list_mode)
-    switch (minsym->type)
-      {
-	case mst_slot_got_plt:
-	case mst_data:
-	case mst_bss:
-	case mst_abs:
-	case mst_file_data:
-	case mst_file_bss:
-	  {
-	    /* Make sure this minsym is not a function descriptor
-	       before we decide to discard it.  */
-	    struct gdbarch *gdbarch = get_objfile_arch (info->objfile);
-	    CORE_ADDR addr = gdbarch_convert_from_func_ptr_addr
-			       (gdbarch, BMSYMBOL_VALUE_ADDRESS (mo),
-				&current_target);
-
-	    if (addr == BMSYMBOL_VALUE_ADDRESS (mo))
-	      return;
-	  }
-      }
+  /* Exclude data symbols when looking for breakpoint locations.  */
+  if (!info->list_mode && !msymbol_is_function (info->objfile, minsym))
+    return;
 
+  bound_minimal_symbol_d mo = {minsym, info->objfile};
   VEC_safe_push (bound_minimal_symbol_d, info->msyms, &mo);
 }
 
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4898da1..d68fb65 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -57,17 +57,35 @@
 /* See minsyms.h.  */
 
 bool
-msymbol_is_text (minimal_symbol *msymbol)
+msymbol_is_function (struct objfile *objfile, minimal_symbol *minsym,
+		     CORE_ADDR *func_address_p)
 {
-  switch (MSYMBOL_TYPE (msymbol))
+  CORE_ADDR msym_addr = MSYMBOL_VALUE_ADDRESS (objfile, minsym);
+
+  switch (minsym->type)
     {
-    case mst_text:
-    case mst_text_gnu_ifunc:
-    case mst_solib_trampoline:
-    case mst_file_text:
-      return true;
+    case mst_slot_got_plt:
+    case mst_data:
+    case mst_bss:
+    case mst_abs:
+    case mst_file_data:
+    case mst_file_bss:
+      {
+	struct gdbarch *gdbarch = get_objfile_arch (objfile);
+	CORE_ADDR pc = gdbarch_convert_from_func_ptr_addr (gdbarch, msym_addr,
+							   &current_target);
+	if (pc != msym_addr)
+	  {
+	    if (func_address_p != NULL)
+	      *func_address_p = pc;
+	    return true;
+	  }
+	return false;
+      }
     default:
-      return false;
+      if (func_address_p != NULL)
+	*func_address_p = msym_addr;
+      return true;
     }
 }
 
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 5c0dde4..baa87f0 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -159,9 +159,14 @@ void terminate_minimal_symbol_table (struct objfile *objfile);
 
 \f
 
-/* Return whether MSYMBOL is a function/method.  */
-
-bool msymbol_is_text (minimal_symbol *msymbol);
+/* Return whether MSYMBOL is a function/method.  If FUNC_ADDRESS_P is
+   non-NULL, and the MSYMBOL is a function, then *FUNC_ADDRESS_P is
+   set to the function's address, already resolved if MINSYM points to
+   a function descriptor.  */
+
+bool msymbol_is_function (struct objfile *objfile,
+			  minimal_symbol *minsym,
+			  CORE_ADDR *func_address_p = NULL);
 
 /* Compute a hash code for the string argument.  */
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3d59367..a249a8d 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3927,14 +3927,14 @@ skip_prologue_using_sal (struct gdbarch *gdbarch, CORE_ADDR func_addr)
 symbol *
 find_function_alias_target (bound_minimal_symbol msymbol)
 {
-  if (!msymbol_is_text (msymbol.minsym))
+  CORE_ADDR func_addr;
+  if (!msymbol_is_function (msymbol.objfile, msymbol.minsym, &func_addr))
     return NULL;
 
-  CORE_ADDR addr = BMSYMBOL_VALUE_ADDRESS (msymbol);
-  symbol *sym = find_pc_function (addr);
+  symbol *sym = find_pc_function (func_addr);
   if (sym != NULL
       && SYMBOL_CLASS (sym) == LOC_BLOCK
-      && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == addr)
+      && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == func_addr)
     return sym;
 
   return NULL;
-- 
2.5.5

Re: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors) (was: Re: ppc64 regression: [PATCH 1/2] Fix "list amb

[Ulrich Weigand]

Pedro Alves wrote:
> On 11/26/2017 04:37 PM, Ulrich Weigand wrote:
> 
> > I now see this as well on Cell/B.E.  This is a serious regression that
> > causes "start" to always fail for me ...
> > 
> > The problem seems to be that GDB sets a breakpoint into the function
> > descriptor for main, which is not a good idea.
> > 
> > Looking at the commit identified above, it seems that GDB now only
> > runs the address through gdbarch_convert_from_func_ptr_addr if
> > msymbol_is_text returns true.  However, if the symbol points to
> > a function descriptor, msymbol_is_text would be false since this
> > is in fact outside the text section.
> > 
> > So I think probably we need to still run the address through
> > gdbarch_convert_from_func_ptr_addr, and if that detects that it
> > was indeed a function descriptor, always treat the resulting
> > address as a function.
> 
> Thanks for the suggestion.  Here's a patch that implements
> something like that.  WDYT?

Yes, this looks all good to me.  Thanks for taking care of this!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors) (was: Re: ppc64 regression: [PATCH 1/2] Fix "list amb

[Pedro Alves]

On 11/29/2017 07:19 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:

>> Thanks for the suggestion.  Here's a patch that implements
>> something like that.  WDYT?
> 
> Yes, this looks all good to me.  Thanks for taking care of this!

Alright, now pushed.  N.p., and sorry for the breakage in the
first place.

Thanks,
Pedro Alves

Re: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors)

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> @@ -4309,22 +4309,16 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
>  	      struct minimal_symbol *msymbol,
>  	      std::vector<symtab_and_line> *result)
>  {
> -  struct gdbarch *gdbarch = get_objfile_arch (objfile);
> -  CORE_ADDR pc;
>    struct symtab_and_line sal;
>  
> -  if (msymbol_is_text (msymbol))
> -    {
> -      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
> -			       (struct obj_section *) 0, 0);
> -      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);

This line is removed, so that sal.section can be null.  It causes
PR 22567.  How is the patch below?

> +  /* The minimal symbol might point to a function descriptor, which is
> +     not a text symbol; try resolving it to the actual code
> +     address.  */
>  
> -      /* The minimal symbol might point to a function descriptor;
> -	 resolve it to the actual code address instead.  */
> -      pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
> -					       &current_target);
> -      if (pc != sal.pc)
> -	sal = find_pc_sect_line (pc, NULL, 0);
> +  CORE_ADDR func_addr;
> +  if (msymbol_is_function (objfile, msymbol, &func_addr))
> +    {
> +      sal = find_pc_sect_line (func_addr, NULL, 0);
>  
>        if (self->funfirstline)
>  	{

-- 
Yao (齐尧)
From b8e112071a75e7227dc3ee5f35cfa874f3b41da7 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 7 Dec 2017 16:53:13 +0000
Subject: [PATCH] Fix PR 22567: set SAL .section in minsym_found

PR 22567 is that breakpoint location can't correct gdbarch from SAL,
because its fields .section and .symtab is NULL.  We use to have code
setting .section, but was removed by 4024cf2

-  if (msymbol_is_text (msymbol))
+  CORE_ADDR func_addr;
+  if (msymbol_is_function (objfile, msymbol, &func_addr))
     {
-      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
-                              (struct obj_section *) 0, 0);
-      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);

this patch adds this back by moving it to the common place at the bottom
of the function.

gdb:

2017-12-07  Yao Qi  <yao.qi@linaro.org>

	PR breakpionts/22567
	* linespec.c (minsym_found): Set sal. objfile and section.

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 09758762..8c36f2a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4365,9 +4365,10 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
       sal.objfile = objfile;
       sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
       sal.pspace = current_program_space;
-      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
     }
 
+  sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
+
   if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
     add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0);
 }

Re: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors)

[Pedro Alves]

On 12/08/2017 09:44 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> @@ -4309,22 +4309,16 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
>>  	      struct minimal_symbol *msymbol,
>>  	      std::vector<symtab_and_line> *result)
>>  {
>> -  struct gdbarch *gdbarch = get_objfile_arch (objfile);
>> -  CORE_ADDR pc;
>>    struct symtab_and_line sal;
>>  
>> -  if (msymbol_is_text (msymbol))
>> -    {
>> -      sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
>> -			       (struct obj_section *) 0, 0);
>> -      sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
> 
> This line is removed, so that sal.section can be null.  It causes
> PR 22567.  How is the patch below?

That's fine with me.  I guess we end up with the wrong section
in the function descriptor / PPC64 case (".opd" instead of some kind
of  ".text" where the resolved function lives), but it shouldn't
matter, since the old code did that as well, AFAICT.

(I noticed that get_sal_arch doesn't consider sal.objfile, probably
because it predates addition of the 'obfile' field.  We could probably
fill in / use that field more, but we don't need to do that now.)

Pedro Alves

Re: [PATCH] Fix setting-breakpoints regression on PPC64 (function descriptors)

[Yao Qi]

On Fri, Dec 8, 2017 at 11:34 AM, Pedro Alves <palves@redhat.com> wrote:
> That's fine with me.  I guess we end up with the wrong section
> in the function descriptor / PPC64 case (".opd" instead of some kind
> of  ".text" where the resolved function lives), but it shouldn't
> matter, since the old code did that as well, AFAICT.

I tested the patch on gcc110, there is no regression.  I pushed it in.

>
> (I noticed that get_sal_arch doesn't consider sal.objfile, probably
> because it predates addition of the 'obfile' field.  We could probably
> fill in / use that field more, but we don't need to do that now.)

I noticed that too, but one thing I am not sure is that sal.objfile is
*only* used for probe,

  /* The probe associated with this symtab_and_line.  */
  probe *prob = NULL;
  /* If PROBE is not NULL, then this is the objfile in which the probe
     originated.  */
  struct objfile *objfile = NULL;

so can we use it in other cases (when probe is not used)?  I don't
know.  If so, are sal.objfile and sal.section->objfile different or same?
I need to look at the code there.

-- 
Yao (齐尧)