[RFA] fix thinko in sym info

[RFA] fix thinko in sym info

[Doug Evans]

Hi.  I noticed this while testing a thinko of my own (blech).

asm-source.exp is now failing because it doesn't expect the offset
to be printed if its zero.
One could update the testcase, but the old behaviour is more user-friendly.

The outstanding question is, of course, whether

  "%s + %u", msym_name, offset

needs i18n.  This patch assumes it doesn't.

2008-11-19  Doug Evans  <dje@google.com>

	* printcmd.c (sym_info): Don't print the offset if it's zero.

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.137
diff -u -p -r1.137 printcmd.c
--- printcmd.c	18 Nov 2008 21:31:26 -0000	1.137
+++ printcmd.c	19 Nov 2008 22:47:41 -0000
@@ -1013,6 +1013,7 @@ sym_info (char *arg, int from_tty)
 	&& (msymbol = lookup_minimal_symbol_by_pc_section (sect_addr, osect)))
       {
 	const char *obj_name, *mapped, *sec_name, *msym_name;
+	char *loc_string;
 
 	matches = 1;
 	offset = sect_addr - SYMBOL_VALUE_ADDRESS (msymbol);
@@ -1020,43 +1021,51 @@ sym_info (char *arg, int from_tty)
 	sec_name = osect->the_bfd_section->name;
 	msym_name = SYMBOL_PRINT_NAME (msymbol);
 
+	/* Don't print the offset if it is zero.
+	   We assume there's no need to handle i18n of "sym + offset".  */
+	if (offset)
+	  xasprintf (&loc_string, "%s + %u", msym_name, offset);
+	else
+	  xasprintf (&loc_string, "%s", msym_name);
+
 	gdb_assert (osect->objfile && osect->objfile->name);
 	obj_name = osect->objfile->name;
 
 	if (MULTI_OBJFILE_P ())
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "%s overlay section %s of %s\n"),
-			       msym_name, offset,
-			       mapped, sec_name, obj_name);
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+			       loc_string, sec_name, obj_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s of %s\n"),
-			       msym_name, offset, mapped, sec_name, obj_name);
+	      printf_filtered (_("%s in %s overlay section %s of %s\n"),
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+	      printf_filtered (_("%s in section %s of %s\n"),
+			       loc_string, sec_name, obj_name);
 	else
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of %s overlay "
+	      printf_filtered (_("%s in load address range of %s overlay "
 				 "section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in load address range of section %s\n"),
+			       loc_string, sec_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+	      printf_filtered (_("%s in %s overlay section %s\n"),
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in section %s\n"),
+			       loc_string, sec_name);
+
+	free (loc_string);
       }
   }
   if (matches == 0)

Re: [RFA] fix thinko in sym info

[Paul Pluzhnikov]

On Wed, Nov 19, 2008 at 2:52 PM, Doug Evans <dje@google.com> wrote:

> asm-source.exp is now failing because it doesn't expect the offset
> to be printed if its zero.
> One could update the testcase, but the old behaviour is more user-friendly.

Hmm, why didn't I notice this failure ...
It's UNTESTED for me both before and after. I'll investigate why.

> The outstanding question is, of course, whether
>  "%s + %u", msym_name, offset
> needs i18n.  This patch assumes it doesn't.

From: http://sourceware.org/ml/gdb-patches/2008-11/msg00378.html

  In addition, translating each part without seeing the whole
  sentence is very difficult.

I didn't want to add 8 more clauses to this if statement.

Regards,
-- 
Paul Pluzhnikov

Re: [RFA] fix thinko in sym info

[Doug Evans]

On Wed, Nov 19, 2008 at 3:02 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Wed, Nov 19, 2008 at 2:52 PM, Doug Evans <dje@google.com> wrote:
>
>> asm-source.exp is now failing because it doesn't expect the offset
>> to be printed if its zero.
>> One could update the testcase, but the old behaviour is more user-friendly.
>
> Hmm, why didn't I notice this failure ...
> It's UNTESTED for me both before and after. I'll investigate why.
>
>> The outstanding question is, of course, whether
>>  "%s + %u", msym_name, offset
>> needs i18n.  This patch assumes it doesn't.
>
> From: http://sourceware.org/ml/gdb-patches/2008-11/msg00378.html
>
>  In addition, translating each part without seeing the whole
>  sentence is very difficult.

Sure, but this part of the sentence seems self-contained enough that
the translation could consider it as a unit.  I could be wrong of
course, but will pedantic translation correctness in this particular
case be worth it.

> I didn't want to add 8 more clauses to this if statement.

No disagreement there.

Re: [RFA] fix thinko in sym info

[Joel Brobecker]

> The outstanding question is, of course, whether
> 
>   "%s + %u", msym_name, offset
> 
> needs i18n.  This patch assumes it doesn't.

I would tend to agree, although perhaps someone who has knowledge with
languages that write right-to-left might come up with a case where
they'd translate "SYMBOL + OFFSET" differently from the original.
In other words, I'm hoping that "SYMBOL + OFFSET" is international.

Also, I would think that the "SYMBOL + OFFSET" part is one entity
that would not be split during translation to any language. This is
derived from the assumption above.

> 2008-11-19  Doug Evans  <dje@google.com>
> 
> 	* printcmd.c (sym_info): Don't print the offset if it's zero.

In my opinion, that's fine. We can always fix the problem later if
someone comes up with a situation where the current approach is
too limited - that'd be pretty easy to do.

Any objection to this patch being applied?

-- 
Joel

Re: [RFA] fix thinko in sym info

[Eli Zaretskii]

> Date: Wed, 19 Nov 2008 18:31:15 -0500
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: ppluzhnikov@google.com, gdb-patches@sourceware.org
> 
> > The outstanding question is, of course, whether
> > 
> >   "%s + %u", msym_name, offset
> > 
> > needs i18n.  This patch assumes it doesn't.
> 
> I would tend to agree, although perhaps someone who has knowledge with
> languages that write right-to-left might come up with a case where
> they'd translate "SYMBOL + OFFSET" differently from the original.

No problem here, as long as SYMBOL uses only ASCII characters: the
Unicode Bidirectional Algorithm mandates that a text that includes
ASCII characters, digits, and a plus sign be rendered left to right.

Re: [RFA] fix thinko in sym info

[Pedro Alves]

This should be:

> +       if (offset)
> +         xasprintf (&loc_string, "%s + %u", msym_name, offset);
> +       else
> +         xasprintf (&loc_string, "%s", msym_name);
> +

        ... = make_cleanup (xfree, loc_string);


> +       free (loc_string);

        do_cleanups (...);

>        }

... instead of raw 'free'.

The easiest exception you can get here, is if the user quits on a
pagination request inside print_filtered.

There, see, a huge leak is plugged fixed.  :-)

(would be s/free/xfree/, btw.)

Other than that, it also looks good to me.

I'd say, go ahead!

-- 
Pedro Alves

Re: [RFA] fix thinko in sym info

[Pedro Alves]

On Thursday 20 November 2008 06:13:19, Doug Evans wrote:

> Can you (or someone) review this patch which includes the suggested changes?

Looks fine, thanks.

-- 
Pedro Alves