* [rfa] add_symbol_file_command, avoid memory leak.
@ 2011-02-27 1:56 Michael Snyder
2011-02-27 20:35 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Michael Snyder @ 2011-02-27 1:56 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 114 bytes --]
This looks straightforward, but I'd appreciate review.
I don't really understand why sect_opts never gets freed?
[-- Attachment #2: symfile.txt --]
[-- Type: text/plain, Size: 938 bytes --]
2011-02-26 Michael Snyder <msnyder@vmware.com>
* symfile.c (add_symbol_file_command): Avoid memory leak.
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.307
diff -u -p -u -p -r1.307 symfile.c
--- symfile.c 26 Feb 2011 02:07:09 -0000 1.307
+++ symfile.c 27 Feb 2011 01:35:32 -0000
@@ -2169,15 +2169,15 @@ add_symbol_file_command (char *args, int
size_t num_sect_opts = 0;
struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
+ if (args == NULL)
+ error (_("add-symbol-file takes a file name and an address"));
+
num_sect_opts = 16;
sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
* sizeof (struct sect_opt));
dont_repeat ();
- if (args == NULL)
- error (_("add-symbol-file takes a file name and an address"));
-
argv = gdb_buildargv (args);
make_cleanup_freeargv (argv);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [rfa] add_symbol_file_command, avoid memory leak.
2011-02-27 1:56 [rfa] add_symbol_file_command, avoid memory leak Michael Snyder
@ 2011-02-27 20:35 ` Jan Kratochvil
2011-02-27 20:42 ` Michael Snyder
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2011-02-27 20:35 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
On Sun, 27 Feb 2011 02:38:04 +0100, Michael Snyder wrote:
> This looks straightforward, but I'd appreciate review.
> I don't really understand why sect_opts never gets freed?
It is a bug, sect_opts should be freed. It is last time used by the lines:
char *val = sect_opts[i].value;
char *sec = sect_opts[i].name;
The strings in sect_opts[i].name are last time used there by:
symbol_file_add (filename, from_tty ? SYMFILE_VERBOSE : 0,
section_addrs, flags);
This is also the reason why the NAMEs do not have strdup() and they get safely
freed through make_cleanup_freeargv there.
> --- symfile.c 26 Feb 2011 02:07:09 -0000 1.307
> +++ symfile.c 27 Feb 2011 01:35:32 -0000
> @@ -2169,15 +2169,15 @@ add_symbol_file_command (char *args, int
> size_t num_sect_opts = 0;
> struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
>
> + if (args == NULL)
> + error (_("add-symbol-file takes a file name and an address"));
> +
> num_sect_opts = 16;
> sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
> * sizeof (struct sect_opt));
>
> dont_repeat ();
>
> - if (args == NULL)
> - error (_("add-symbol-file takes a file name and an address"));
> -
> argv = gdb_buildargv (args);
> make_cleanup_freeargv (argv);
>
While a nitpick it is a regression, that dont_repeat should be called even in
the case of that error.
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [rfa] add_symbol_file_command, avoid memory leak.
2011-02-27 20:35 ` Jan Kratochvil
@ 2011-02-27 20:42 ` Michael Snyder
2011-02-27 20:51 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Michael Snyder @ 2011-02-27 20:42 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches
Jan Kratochvil wrote:
> On Sun, 27 Feb 2011 02:38:04 +0100, Michael Snyder wrote:
>> This looks straightforward, but I'd appreciate review.
>> I don't really understand why sect_opts never gets freed?
>
> It is a bug, sect_opts should be freed. It is last time used by the lines:
> char *val = sect_opts[i].value;
> char *sec = sect_opts[i].name;
>
> The strings in sect_opts[i].name are last time used there by:
> symbol_file_add (filename, from_tty ? SYMFILE_VERBOSE : 0,
> section_addrs, flags);
>
> This is also the reason why the NAMEs do not have strdup() and they get safely
> freed through make_cleanup_freeargv there.
>
>
>> --- symfile.c 26 Feb 2011 02:07:09 -0000 1.307
>> +++ symfile.c 27 Feb 2011 01:35:32 -0000
>> @@ -2169,15 +2169,15 @@ add_symbol_file_command (char *args, int
>> size_t num_sect_opts = 0;
>> struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
>>
>> + if (args == NULL)
>> + error (_("add-symbol-file takes a file name and an address"));
>> +
>> num_sect_opts = 16;
>> sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
>> * sizeof (struct sect_opt));
>>
>> dont_repeat ();
>>
>> - if (args == NULL)
>> - error (_("add-symbol-file takes a file name and an address"));
>> -
>> argv = gdb_buildargv (args);
>> make_cleanup_freeargv (argv);
>>
>
> While a nitpick it is a regression, that dont_repeat should be called even in
> the case of that error.
No, it's a fix. I moved the error before dont_repeat.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [rfa] add_symbol_file_command, avoid memory leak.
2011-02-27 20:42 ` Michael Snyder
@ 2011-02-27 20:51 ` Jan Kratochvil
2011-02-27 22:00 ` Michael Snyder
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2011-02-27 20:51 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
On Sun, 27 Feb 2011 21:35:46 +0100, Michael Snyder wrote:
> Jan Kratochvil wrote:
> > While a nitpick it is a regression, that dont_repeat should be called even
> > in the case of that error.
>
> No, it's a fix. I moved the error before dont_repeat.
Why? The manual says:
`add-symbol-file' does not repeat if you press <RET> after using it.
Other part of the manual says:
and you can repeat certain GDB commands by typing just <RET>.
There is nothing said that whether the command is / is not repated depends on
its entered arguments.
(Not sure if it matters to continue the discussion, I do not mind either way.)
Regards,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [rfa] add_symbol_file_command, avoid memory leak.
2011-02-27 20:51 ` Jan Kratochvil
@ 2011-02-27 22:00 ` Michael Snyder
2011-02-27 22:12 ` Jan Kratochvil
0 siblings, 1 reply; 6+ messages in thread
From: Michael Snyder @ 2011-02-27 22:00 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 737 bytes --]
Jan Kratochvil wrote:
> On Sun, 27 Feb 2011 21:35:46 +0100, Michael Snyder wrote:
>> Jan Kratochvil wrote:
>>> While a nitpick it is a regression, that dont_repeat should be called even
>>> in the case of that error.
>> No, it's a fix. I moved the error before dont_repeat.
>
> Why? The manual says:
> `add-symbol-file' does not repeat if you press <RET> after using it.
> Other part of the manual says:
> and you can repeat certain GDB commands by typing just <RET>.
>
> There is nothing said that whether the command is / is not repated depends on
> its entered arguments.
>
> (Not sure if it matters to continue the discussion, I do not mind either way.)
Ah, sorry, I misunderstood you. I'm with you now.
How about this?
[-- Attachment #2: symfile2.txt --]
[-- Type: text/plain, Size: 968 bytes --]
2011-02-26 Michael Snyder <msnyder@vmware.com>
* symfile.c (add_symbol_file_command): Avoid memory leak.
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.307
diff -u -p -u -p -r1.307 symfile.c
--- symfile.c 26 Feb 2011 02:07:09 -0000 1.307
+++ symfile.c 27 Feb 2011 21:56:20 -0000
@@ -2169,15 +2169,15 @@ add_symbol_file_command (char *args, int
size_t num_sect_opts = 0;
struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
- num_sect_opts = 16;
- sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
- * sizeof (struct sect_opt));
-
dont_repeat ();
if (args == NULL)
error (_("add-symbol-file takes a file name and an address"));
+ num_sect_opts = 16;
+ sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
+ * sizeof (struct sect_opt));
+
argv = gdb_buildargv (args);
make_cleanup_freeargv (argv);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [rfa] add_symbol_file_command, avoid memory leak.
2011-02-27 22:00 ` Michael Snyder
@ 2011-02-27 22:12 ` Jan Kratochvil
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kratochvil @ 2011-02-27 22:12 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
On Sun, 27 Feb 2011 22:57:09 +0100, Michael Snyder wrote:
> @@ -2169,15 +2169,15 @@ add_symbol_file_command (char *args, int
> size_t num_sect_opts = 0;
> struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
>
> - num_sect_opts = 16;
> - sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
> - * sizeof (struct sect_opt));
> -
> dont_repeat ();
>
> if (args == NULL)
> error (_("add-symbol-file takes a file name and an address"));
>
> + num_sect_opts = 16;
> + sect_opts = (struct sect_opt *) xmalloc (num_sect_opts
> + * sizeof (struct sect_opt));
> +
> argv = gdb_buildargv (args);
> make_cleanup_freeargv (argv);
>
> How about this?
Yes, the fix is OK now. There could be also added some
make_cleanup (xfree, sect_opts);
as discussed before; but it may be a different patch.
Thanks,
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-27 22:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-27 1:56 [rfa] add_symbol_file_command, avoid memory leak Michael Snyder
2011-02-27 20:35 ` Jan Kratochvil
2011-02-27 20:42 ` Michael Snyder
2011-02-27 20:51 ` Jan Kratochvil
2011-02-27 22:00 ` Michael Snyder
2011-02-27 22:12 ` Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox