[PATCH] Don't print symbol definition's line number in rbreak output

[PATCH] Don't print symbol definition's line number in rbreak output

[Andreas Arnez]

This commit:

  b744723f57 -- Show line numbers in output for "info var/func/type"

added the symbol definition's line number to the output of certain GDB
commands.  It also changes the `rbreak' command's output, although it
shouldn't.  This is fixed.

In order to distinguish when to print location information, the meaning of
print_symbol_info()'s parameter `last' is changed.  Now NULL means to skip
any filename or line number information.  Previously NULL meant to always
print the filename.

gdb/ChangeLog:

	* symtab.c (print_symbol_info): Skip printing filename and line
	number when `last' is NULL.
	(symtab_symbol_info): Use empty string instead of NULL for first
	invocation of print_symbol_info.
	(rbreak_command): Pass NULL to `last' parameter of
	print_symbol_info.
---
 gdb/symtab.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index f66b6f00f0..cebfdac46c 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4500,7 +4500,9 @@ search_symbols (const char *regexp, enum search_domain kind,
 
 /* Helper function for symtab_symbol_info, this function uses
    the data returned from search_symbols() to print information
-   regarding the match to gdb_stdout.  */
+   regarding the match to gdb_stdout.  If LAST is not NULL,
+   print file- and line number information for the symbol as
+   well.  Skip printing the filename if it matches LAST.  */
 
 static void
 print_symbol_info (enum search_domain kind,
@@ -4508,19 +4510,23 @@ print_symbol_info (enum search_domain kind,
 		   int block, const char *last)
 {
   struct symtab *s = symbol_symtab (sym);
-  const char *s_filename = symtab_to_filename_for_display (s);
 
-  if (last == NULL || filename_cmp (last, s_filename) != 0)
+  if (last != NULL)
     {
-      fputs_filtered ("\nFile ", gdb_stdout);
-      fputs_filtered (s_filename, gdb_stdout);
-      fputs_filtered (":\n", gdb_stdout);
-    }
+      const char *s_filename = symtab_to_filename_for_display (s);
 
-  if (SYMBOL_LINE (sym) != 0)
-    printf_filtered ("%d:\t", SYMBOL_LINE (sym));
-  else
-    puts_filtered ("\t");
+      if (filename_cmp (last, s_filename) != 0)
+	{
+	  fputs_filtered ("\nFile ", gdb_stdout);
+	  fputs_filtered (s_filename, gdb_stdout);
+	  fputs_filtered (":\n", gdb_stdout);
+	}
+
+      if (SYMBOL_LINE (sym) != 0)
+	printf_filtered ("%d:\t", SYMBOL_LINE (sym));
+      else
+	puts_filtered ("\t");
+    }
 
   if (kind != TYPES_DOMAIN && block == STATIC_BLOCK)
     printf_filtered ("static ");
@@ -4573,7 +4579,7 @@ symtab_symbol_info (const char *regexp, enum search_domain kind, int from_tty)
 {
   static const char * const classnames[] =
     {"variable", "function", "type"};
-  const char *last_filename = NULL;
+  const char *last_filename = "";
   int first = 1;
 
   gdb_assert (kind <= TYPES_DOMAIN);
@@ -4684,10 +4690,7 @@ rbreak_command (const char *regexp, int from_tty)
 	  string = string_printf ("%s:'%s'", fullname,
 				  SYMBOL_LINKAGE_NAME (p.symbol));
 	  break_command (&string[0], from_tty);
-	  print_symbol_info (FUNCTIONS_DOMAIN,
-			     p.symbol,
-			     p.block,
-			     symtab_to_filename_for_display (symtab));
+	  print_symbol_info (FUNCTIONS_DOMAIN, p.symbol, p.block, NULL);
 	}
       else
 	{
-- 
2.14.3

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Pedro Alves]

On 04/16/2018 07:44 PM, Andreas Arnez wrote:
> This commit:
> 
>   b744723f57 -- Show line numbers in output for "info var/func/type"
> 
> added the symbol definition's line number to the output of certain GDB
> commands.  It also changes the `rbreak' command's output, although it
> shouldn't.  This is fixed.

Could you update this to include an example of before/after gdb
output in the commit log?

Is this a regression in 8.1?

Thanks,
Pedro Alves

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Andreas Arnez]

On Mon, Apr 16 2018, Pedro Alves wrote:

> On 04/16/2018 07:44 PM, Andreas Arnez wrote:
>> This commit:
>> 
>>   b744723f57 -- Show line numbers in output for "info var/func/type"
>> 
>> added the symbol definition's line number to the output of certain GDB
>> commands.  It also changes the `rbreak' command's output, although it
>> shouldn't.  This is fixed.
>
> Could you update this to include an example of before/after gdb
> output in the commit log?

Sure.  How about the updated commit message below?

>
> Is this a regression in 8.1?

No, I just caused the regression myself on Friday with the commit above.
For some reason I had not noticed the impact on the `rbreak' command
before.

--
Andreas

-- >8 --
Subject: [PATCH] Don't print symbol definition's line number in rbreak output

This commit:

  b744723f57 -- Show line numbers in output for "info var/func/type"

added the symbol declaration's line number to the output of certain GDB
commands.  It also changes the `rbreak' command's output, like this:

  (gdb) rbreak foo
  Breakpoint 1 at 0x40049b: file rbreak.c, line 6.
  4:      static int foo1(void);
  Breakpoint 2 at 0x4004b1: file rbreak.c, line 12.
  10:     static int foo2(void);
  (gdb)

where the function declaration is now prefixed by its source line number,
followed by a colon.  But without showing the declaration's file name, the
line number is useless and can possibly cause severe confusion.

No declaration line number was shown before.  Instead, the function
declaration started at the first column.  This old behavior is restored.

In order to distinguish when to print location information, the meaning of
print_symbol_info()'s parameter `last' is changed.  Now NULL means to skip
any filename or line number information.  Previously NULL meant to always
print the filename.

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Pedro Alves]

On 04/17/2018 01:28 PM, Andreas Arnez wrote:
> On Mon, Apr 16 2018, Pedro Alves wrote:
> 
>> On 04/16/2018 07:44 PM, Andreas Arnez wrote:
>>> This commit:
>>>
>>>   b744723f57 -- Show line numbers in output for "info var/func/type"
>>>
>>> added the symbol definition's line number to the output of certain GDB
>>> commands.  It also changes the `rbreak' command's output, although it
>>> shouldn't.  This is fixed.
>>
>> Could you update this to include an example of before/after gdb
>> output in the commit log?
> 
> Sure.  How about the updated commit message below?
> 

Thanks.

>>
>> Is this a regression in 8.1?
> 
> No, I just caused the regression myself on Friday with the commit above.
> For some reason I had not noticed the impact on the `rbreak' command
> before.

Ahah, thanks, it's much clearer now.

> No declaration line number was shown before.  Instead, the function
> declaration started at the first column.  This old behavior is restored.

IMHO it's always better to also paste the output after, so the
reader doesn't have to imagine it from the description.

>  /* Helper function for symtab_symbol_info, this function uses
>     the data returned from search_symbols() to print information
> -   regarding the match to gdb_stdout.  */
> +   regarding the match to gdb_stdout.  If LAST is not NULL,
> +   print file- and line number information for the symbol as
> +   well.  Skip printing the filename if it matches LAST.  */

Nit, I don't think use of suspense hyphen in this case is common
in English: <http://grammartips.homestead.com/hyphens3.html>.
I'd just write plain "file".

The patch looks fine to me.  However, I notice now that this doesn't
tweak any existing testcase or add any new one.  Was the problem
caught by some existing testcase?

Thanks,
Pedro Alves

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Andreas Arnez]

On Tue, Apr 17 2018, Pedro Alves wrote:

> On 04/17/2018 01:28 PM, Andreas Arnez wrote:

[...]

>> No declaration line number was shown before.  Instead, the function
>> declaration started at the first column.  This old behavior is restored.
>
> IMHO it's always better to also paste the output after, so the
> reader doesn't have to imagine it from the description.

OK.

>
>>  /* Helper function for symtab_symbol_info, this function uses
>>     the data returned from search_symbols() to print information
>> -   regarding the match to gdb_stdout.  */
>> +   regarding the match to gdb_stdout.  If LAST is not NULL,
>> +   print file- and line number information for the symbol as
>> +   well.  Skip printing the filename if it matches LAST.  */
>
> Nit, I don't think use of suspense hyphen in this case is common
> in English: <http://grammartips.homestead.com/hyphens3.html>.
> I'd just write plain "file".

Hehe, good catch.

>
> The patch looks fine to me.  However, I notice now that this doesn't
> tweak any existing testcase or add any new one.  Was the problem
> caught by some existing testcase?

Yes, it was.  The attached v2 now provides this information and should
also address all your previous comments.

OK to push?

--
Andreas


-- >8 --
Subject: [PATCH v2] Don't print symbol declaration's line number in rbreak output

This commit:

  b744723f57 -- Show line numbers in output for "info var/func/type"

adds the symbol declaration's line number to the output of certain GDB
commands.  It also (inadvertently) changes the `rbreak' command's output,
like this:

  (gdb) rbreak foo
  Breakpoint 1 at 0x40049b: file rbreak.c, line 6.
  4:      static int foo1(void);
  Breakpoint 2 at 0x4004b1: file rbreak.c, line 12.
  10:     static int foo2(void);
  (gdb)

where the function declaration is now prefixed by its source line number,
followed by a colon.  But without showing the declaration's file name, the
line number is useless and can possibly cause severe confusion.

No declaration line number was shown before.  Instead, the function
declaration started at the first column:

  (gdb) rbreak foo
  Breakpoint 1 at 0x40049b: file rbreak.c, line 6.
  static int foo1(void);
  Breakpoint 2 at 0x4004b1: file rbreak.c, line 12.
  static int foo2(void);
  (gdb)

This old behavior is restored, fixing some FAILs in fullpath-expand.exp,
realname-expand.exp, and pr10179.exp.

In order to distinguish when to print location information, the meaning of
print_symbol_info()'s parameter `last' is changed.  Now NULL means to skip
any filename or line number information.  Previously NULL meant to always
print the filename.

gdb/ChangeLog:

	* symtab.c (print_symbol_info): Skip printing filename and line
	number when `last' is NULL.
	(symtab_symbol_info): Use empty string instead of NULL for first
	invocation of print_symbol_info.
	(rbreak_command): Pass NULL to `last' parameter of
	print_symbol_info.
---
 gdb/symtab.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index f66b6f00f0..c1ead701ec 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4500,7 +4500,9 @@ search_symbols (const char *regexp, enum search_domain kind,
 
 /* Helper function for symtab_symbol_info, this function uses
    the data returned from search_symbols() to print information
-   regarding the match to gdb_stdout.  */
+   regarding the match to gdb_stdout.  If LAST is not NULL,
+   print file and line number information for the symbol as
+   well.  Skip printing the filename if it matches LAST.  */
 
 static void
 print_symbol_info (enum search_domain kind,
@@ -4508,19 +4510,23 @@ print_symbol_info (enum search_domain kind,
 		   int block, const char *last)
 {
   struct symtab *s = symbol_symtab (sym);
-  const char *s_filename = symtab_to_filename_for_display (s);
 
-  if (last == NULL || filename_cmp (last, s_filename) != 0)
+  if (last != NULL)
     {
-      fputs_filtered ("\nFile ", gdb_stdout);
-      fputs_filtered (s_filename, gdb_stdout);
-      fputs_filtered (":\n", gdb_stdout);
-    }
+      const char *s_filename = symtab_to_filename_for_display (s);
 
-  if (SYMBOL_LINE (sym) != 0)
-    printf_filtered ("%d:\t", SYMBOL_LINE (sym));
-  else
-    puts_filtered ("\t");
+      if (filename_cmp (last, s_filename) != 0)
+	{
+	  fputs_filtered ("\nFile ", gdb_stdout);
+	  fputs_filtered (s_filename, gdb_stdout);
+	  fputs_filtered (":\n", gdb_stdout);
+	}
+
+      if (SYMBOL_LINE (sym) != 0)
+	printf_filtered ("%d:\t", SYMBOL_LINE (sym));
+      else
+	puts_filtered ("\t");
+    }
 
   if (kind != TYPES_DOMAIN && block == STATIC_BLOCK)
     printf_filtered ("static ");
@@ -4573,7 +4579,7 @@ symtab_symbol_info (const char *regexp, enum search_domain kind, int from_tty)
 {
   static const char * const classnames[] =
     {"variable", "function", "type"};
-  const char *last_filename = NULL;
+  const char *last_filename = "";
   int first = 1;
 
   gdb_assert (kind <= TYPES_DOMAIN);
@@ -4684,10 +4690,7 @@ rbreak_command (const char *regexp, int from_tty)
 	  string = string_printf ("%s:'%s'", fullname,
 				  SYMBOL_LINKAGE_NAME (p.symbol));
 	  break_command (&string[0], from_tty);
-	  print_symbol_info (FUNCTIONS_DOMAIN,
-			     p.symbol,
-			     p.block,
-			     symtab_to_filename_for_display (symtab));
+	  print_symbol_info (FUNCTIONS_DOMAIN, p.symbol, p.block, NULL);
 	}
       else
 	{
-- 
2.14.3

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Pedro Alves]

On 04/17/2018 05:38 PM, Andreas Arnez wrote:

>> The patch looks fine to me.  However, I notice now that this doesn't
>> tweak any existing testcase or add any new one.  Was the problem
>> caught by some existing testcase?
> 
> Yes, it was.  The attached v2 now provides this information and should
> also address all your previous comments.

Ah, cool.

> 
> OK to push?

OK.  Thanks for the patience.

Pedro Alves

Re: [PATCH] Don't print symbol definition's line number in rbreak output

[Andreas Arnez]

On Tue, Apr 17 2018, Pedro Alves wrote:

> On 04/17/2018 05:38 PM, Andreas Arnez wrote:
>
>>> The patch looks fine to me.  However, I notice now that this doesn't
>>> tweak any existing testcase or add any new one.  Was the problem
>>> caught by some existing testcase?
>> 
>> Yes, it was.  The attached v2 now provides this information and should
>> also address all your previous comments.
>
> Ah, cool.
>
>> 
>> OK to push?
>
> OK.  Thanks for the patience.

NP, thanks for your comments.  I've pushed this now.

--
Andreas