[PATCH] Factor out rbreak_command with concat

[PATCH] Factor out rbreak_command with concat

[Ciro Santilli]

The function behaviour should be unchanged.

len was always 0 , so I removed it.

xrealloc was always called on NULL string, so equivalent to xmalloc,
which concat uses.

---
 gdb/ChangeLog |  4 ++++
 gdb/symtab.c  | 35 +++++++++--------------------------
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 407dfb4..3d8c416 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+4015-07-20  Ciro Santilli  <ciro.santilli@gmail.com>
+
+    * symtab.c (rbreak_command): Factor with concat.
+
 2015-07-15  Markus Metzger  <markus.t.metzger@intel.com>
         Pedro Alves <palves@redhat.com>

diff --git a/gdb/symtab.c b/gdb/symtab.c
index decc5a9..015cb56 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4889,7 +4889,6 @@ rbreak_command (char *regexp, int from_tty)
   struct symbol_search *p;
   struct cleanup *old_chain;
   char *string = NULL;
-  int len = 0;
   const char **files = NULL;
   const char *file_name;
   int nfiles = 0;
@@ -4928,20 +4927,11 @@ rbreak_command (char *regexp, int from_tty)
     {
       struct symtab *symtab = symbol_symtab (p->symbol);
       const char *fullname = symtab_to_fullname (symtab);
-
-      int newlen = (strlen (fullname)
-            + strlen (SYMBOL_LINKAGE_NAME (p->symbol))
-            + 4);
-
-      if (newlen > len)
-        {
-          string = xrealloc (string, newlen);
-          len = newlen;
-        }
-      strcpy (string, fullname);
-      strcat (string, ":'");
-      strcat (string, SYMBOL_LINKAGE_NAME (p->symbol));
-      strcat (string, "'");
+      string = concat(fullname,
+              ":'",
+              SYMBOL_LINKAGE_NAME (p->symbol),
+              "'",
+              (char *)NULL);
       break_command (string, from_tty);
       print_symbol_info (FUNCTIONS_DOMAIN,
                  p->symbol,
@@ -4950,17 +4940,10 @@ rbreak_command (char *regexp, int from_tty)
     }
       else
     {
-      int newlen = (strlen (MSYMBOL_LINKAGE_NAME (p->msymbol.minsym)) + 3);
-
-      if (newlen > len)
-        {
-          string = xrealloc (string, newlen);
-          len = newlen;
-        }
-      strcpy (string, "'");
-      strcat (string, MSYMBOL_LINKAGE_NAME (p->msymbol.minsym));
-      strcat (string, "'");
-
+      string = concat("'",
+              MSYMBOL_LINKAGE_NAME (p->msymbol.minsym),
+              "'",
+              (char *)NULL);
       break_command (string, from_tty);
       printf_filtered ("<function, no debug info> %s;\n",
                MSYMBOL_PRINT_NAME (p->msymbol.minsym));
-- 
1.9.1

Re: [PATCH] Factor out rbreak_command with concat

[Doug Evans]

Ciro Santilli <ciro.santilli@gmail.com> writes:
> The function behaviour should be unchanged.
>
> len was always 0 , so I removed it.
>
> xrealloc was always called on NULL string, so equivalent to xmalloc,
> which concat uses.
>
> ---
>  gdb/ChangeLog |  4 ++++
>  gdb/symtab.c  | 35 +++++++++--------------------------
>  2 files changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 407dfb4..3d8c416 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +4015-07-20  Ciro Santilli  <ciro.santilli@gmail.com>
> +
> +    * symtab.c (rbreak_command): Factor with concat.
> +
>  2015-07-15  Markus Metzger  <markus.t.metzger@intel.com>
>          Pedro Alves <palves@redhat.com>
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index decc5a9..015cb56 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4889,7 +4889,6 @@ rbreak_command (char *regexp, int from_tty)
>    struct symbol_search *p;
>    struct cleanup *old_chain;
>    char *string = NULL;
> -  int len = 0;
>    const char **files = NULL;
>    const char *file_name;
>    int nfiles = 0;
> @@ -4928,20 +4927,11 @@ rbreak_command (char *regexp, int from_tty)
>      {
>        struct symtab *symtab = symbol_symtab (p->symbol);
>        const char *fullname = symtab_to_fullname (symtab);
> -
> -      int newlen = (strlen (fullname)
> -            + strlen (SYMBOL_LINKAGE_NAME (p->symbol))
> -            + 4);
> -
> -      if (newlen > len)
> -        {
> -          string = xrealloc (string, newlen);
> -          len = newlen;
> -        }
> -      strcpy (string, fullname);
> -      strcat (string, ":'");
> -      strcat (string, SYMBOL_LINKAGE_NAME (p->symbol));
> -      strcat (string, "'");
> +      string = concat(fullname,
> +              ":'",
> +              SYMBOL_LINKAGE_NAME (p->symbol),
> +              "'",
> +              (char *)NULL);
>        break_command (string, from_tty);
>        print_symbol_info (FUNCTIONS_DOMAIN,
>                   p->symbol,
> @@ -4950,17 +4940,10 @@ rbreak_command (char *regexp, int from_tty)
>      }
>        else
>      {
> -      int newlen = (strlen (MSYMBOL_LINKAGE_NAME (p->msymbol.minsym)) + 3);
> -
> -      if (newlen > len)
> -        {
> -          string = xrealloc (string, newlen);
> -          len = newlen;
> -        }
> -      strcpy (string, "'");
> -      strcat (string, MSYMBOL_LINKAGE_NAME (p->msymbol.minsym));
> -      strcat (string, "'");
> -
> +      string = concat("'",
> +              MSYMBOL_LINKAGE_NAME (p->msymbol.minsym),
> +              "'",
> +              (char *)NULL);
>        break_command (string, from_tty);
>        printf_filtered ("<function, no debug info> %s;\n",
>                 MSYMBOL_PRINT_NAME (p->msymbol.minsym));

Hi.

Thanks for the patch.

Things are a bit odd, no disagreement there.
They are that way because note that all of this is
done in a loop, and we reuse the buffer for each
iteration.

The above patch will leak memory.

We *could* just concat the string for each iteration,
but we have to ensure the string is freed, even if, say,
break_command throws an error, so that means setting
up a cleanup inside the loop. At this point it's
probably a toss-up as to which is simpler, so I'd say
let's leave things as is.

Sound ok?

Re: [PATCH] Factor out rbreak_command with concat

[Ciro Santilli]

I feel silly for not seeing the for loop..... :-)

What you say makes sense.

I wish there was a concat in libiberty that would reuse the buffer for us.

Patch withdrawn.

On Sun, Jul 26, 2015 at 7:12 PM, Doug Evans <xdje42@gmail.com> wrote:
> Ciro Santilli <ciro.santilli@gmail.com> writes:
>> The function behaviour should be unchanged.
>>
>> len was always 0 , so I removed it.
>>
>> xrealloc was always called on NULL string, so equivalent to xmalloc,
>> which concat uses.
>>
>> ---
>>  gdb/ChangeLog |  4 ++++
>>  gdb/symtab.c  | 35 +++++++++--------------------------
>>  2 files changed, 13 insertions(+), 26 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 407dfb4..3d8c416 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,7 @@
>> +4015-07-20  Ciro Santilli  <ciro.santilli@gmail.com>
>> +
>> +    * symtab.c (rbreak_command): Factor with concat.
>> +
>>  2015-07-15  Markus Metzger  <markus.t.metzger@intel.com>
>>          Pedro Alves <palves@redhat.com>
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index decc5a9..015cb56 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4889,7 +4889,6 @@ rbreak_command (char *regexp, int from_tty)
>>    struct symbol_search *p;
>>    struct cleanup *old_chain;
>>    char *string = NULL;
>> -  int len = 0;
>>    const char **files = NULL;
>>    const char *file_name;
>>    int nfiles = 0;
>> @@ -4928,20 +4927,11 @@ rbreak_command (char *regexp, int from_tty)
>>      {
>>        struct symtab *symtab = symbol_symtab (p->symbol);
>>        const char *fullname = symtab_to_fullname (symtab);
>> -
>> -      int newlen = (strlen (fullname)
>> -            + strlen (SYMBOL_LINKAGE_NAME (p->symbol))
>> -            + 4);
>> -
>> -      if (newlen > len)
>> -        {
>> -          string = xrealloc (string, newlen);
>> -          len = newlen;
>> -        }
>> -      strcpy (string, fullname);
>> -      strcat (string, ":'");
>> -      strcat (string, SYMBOL_LINKAGE_NAME (p->symbol));
>> -      strcat (string, "'");
>> +      string = concat(fullname,
>> +              ":'",
>> +              SYMBOL_LINKAGE_NAME (p->symbol),
>> +              "'",
>> +              (char *)NULL);
>>        break_command (string, from_tty);
>>        print_symbol_info (FUNCTIONS_DOMAIN,
>>                   p->symbol,
>> @@ -4950,17 +4940,10 @@ rbreak_command (char *regexp, int from_tty)
>>      }
>>        else
>>      {
>> -      int newlen = (strlen (MSYMBOL_LINKAGE_NAME (p->msymbol.minsym)) + 3);
>> -
>> -      if (newlen > len)
>> -        {
>> -          string = xrealloc (string, newlen);
>> -          len = newlen;
>> -        }
>> -      strcpy (string, "'");
>> -      strcat (string, MSYMBOL_LINKAGE_NAME (p->msymbol.minsym));
>> -      strcat (string, "'");
>> -
>> +      string = concat("'",
>> +              MSYMBOL_LINKAGE_NAME (p->msymbol.minsym),
>> +              "'",
>> +              (char *)NULL);
>>        break_command (string, from_tty);
>>        printf_filtered ("<function, no debug info> %s;\n",
>>                 MSYMBOL_PRINT_NAME (p->msymbol.minsym));
>
> Hi.
>
> Thanks for the patch.
>
> Things are a bit odd, no disagreement there.
> They are that way because note that all of this is
> done in a loop, and we reuse the buffer for each
> iteration.
>
> The above patch will leak memory.
>
> We *could* just concat the string for each iteration,
> but we have to ensure the string is freed, even if, say,
> break_command throws an error, so that means setting
> up a cleanup inside the loop. At this point it's
> probably a toss-up as to which is simpler, so I'd say
> let's leave things as is.
>
> Sound ok?