sim: Be sure of calling freeargv() after successfully call buildargv().
Commit Message
buildargv() and freeargv() are pairs, so need be sure of them always
paired to avoid memory leak.
2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
* common/sim-options.c (sim_args_command): Call freeargv() when
failure occurs.
* mcore/interp.c (sim_do_command): Call freeargv() before return.
* microblaze/interp.c (sim_do_command): Call freeargv() before
return.
---
sim/ChangeLog | 8 ++++++++
sim/common/sim-options.c | 5 ++++-
sim/mcore/interp.c | 3 +++
sim/microblaze/interp.c | 3 +++
4 files changed, 18 insertions(+), 1 deletion(-)
Comments
On 01/28/15 03:45, Chen Gang S wrote:
> buildargv() and freeargv() are pairs, so need be sure of them always
> paired to avoid memory leak.
>
> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>
> * common/sim-options.c (sim_args_command): Call freeargv() when
> failure occurs.
> * mcore/interp.c (sim_do_command): Call freeargv() before return.
> * microblaze/interp.c (sim_do_command): Call freeargv() before
> return.
OK for Microblaze.
There appear to be other places where buildargv() is not followed by
freeargv(). See sim/common/run.c. There may be others.
On 1/28/15 23:53, Michael Eager wrote:
> On 01/28/15 03:45, Chen Gang S wrote:
>> buildargv() and freeargv() are pairs, so need be sure of them always
>> paired to avoid memory leak.
>>
>> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> * common/sim-options.c (sim_args_command): Call freeargv() when
>> failure occurs.
>> * mcore/interp.c (sim_do_command): Call freeargv() before return.
>> * microblaze/interp.c (sim_do_command): Call freeargv() before
>> return.
>
>
> OK for Microblaze.
>
> There appear to be other places where buildargv() is not followed by
> freeargv(). See sim/common/run.c. There may be others.
>
For me, I intended to skip buildargv() in "sim/common/run.c", because it
may contents read only memory. It is in main(), also main() often uses
exit(), so I skip it, it doesn't matter.
gdb also uses buildargv(), but it just wraps buildargv() and freeargv(),
so at least it is another patch (either, at least now, I do not find any
issues about it in gdb).
Except the above 2, for me, there are no any other places to use
buildargv().
Does the patch comments need to mention about the contents above?
Thanks.
On 01/28/15 14:19, Chen Gang S wrote:
> On 1/28/15 23:53, Michael Eager wrote:
>> On 01/28/15 03:45, Chen Gang S wrote:
>>> buildargv() and freeargv() are pairs, so need be sure of them always
>>> paired to avoid memory leak.
>>>
>>> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>>>
>>> * common/sim-options.c (sim_args_command): Call freeargv() when
>>> failure occurs.
>>> * mcore/interp.c (sim_do_command): Call freeargv() before return.
>>> * microblaze/interp.c (sim_do_command): Call freeargv() before
>>> return.
>>
>>
>> OK for Microblaze.
>>
>> There appear to be other places where buildargv() is not followed by
>> freeargv(). See sim/common/run.c. There may be others.
>>
>
> For me, I intended to skip buildargv() in "sim/common/run.c", because it
> may contents read only memory. It is in main(), also main() often uses
> exit(), so I skip it, it doesn't matter.
>
> gdb also uses buildargv(), but it just wraps buildargv() and freeargv(),
> so at least it is another patch (either, at least now, I do not find any
> issues about it in gdb).
>
> Except the above 2, for me, there are no any other places to use
> buildargv().
>
>
> Does the patch comments need to mention about the contents above?
No, you don't need to mention what is not modified.
On 1/29/15 08:04, Michael Eager wrote:
> On 01/28/15 14:19, Chen Gang S wrote:
>> On 1/28/15 23:53, Michael Eager wrote:
>>> On 01/28/15 03:45, Chen Gang S wrote:
>>>> buildargv() and freeargv() are pairs, so need be sure of them always
>>>> paired to avoid memory leak.
>>>>
>>>> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>>>>
>>>> * common/sim-options.c (sim_args_command): Call freeargv() when
>>>> failure occurs.
>>>> * mcore/interp.c (sim_do_command): Call freeargv() before return.
>>>> * microblaze/interp.c (sim_do_command): Call freeargv() before
>>>> return.
>>>
>>>
>>> OK for Microblaze.
>>>
>>> There appear to be other places where buildargv() is not followed by
>>> freeargv(). See sim/common/run.c. There may be others.
>>>
>>
>> For me, I intended to skip buildargv() in "sim/common/run.c", because it
>> may contents read only memory. It is in main(), also main() often uses
>> exit(), so I skip it, it doesn't matter.
>>
>> gdb also uses buildargv(), but it just wraps buildargv() and freeargv(),
>> so at least it is another patch (either, at least now, I do not find any
>> issues about it in gdb).
>>
>> Except the above 2, for me, there are no any other places to use
>> buildargv().
>>
Oh, sorry, the 3rd sentence is incorrect, it should be:
"Except the above 2, for me, there are no any other places to cause
buildargv() related issues or doubts".
>>
>> Does the patch comments need to mention about the contents above?
>
> No, you don't need to mention what is not modified.
>
>
OK, thanks.
[binutils does not need to be copied in this case, as you're only
touching files in the simulator, which is part of the GDB project]
> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>
> * common/sim-options.c (sim_args_command): Call freeargv() when
> failure occurs.
> * mcore/interp.c (sim_do_command): Call freeargv() before return.
> * microblaze/interp.c (sim_do_command): Call freeargv() before
> return.
Small procedural request, Chen. Those 3 changes are pretty much
independent, so it's highly preferable to submit them separately.
This has a number of advantages: We can review each one of them
individually, with possibly different reviewers, and that makes
tracking of which part has been reviewed a lot easier. Also, by
having them submitted separately, you can have one patch per piece,
which means that if one patch turns out to be incorrect, we can
easily revert just that patch using git, rather than doing a semi-
revert by hand.
On 1/29/15 12:49, Joel Brobecker wrote:
> [binutils does not need to be copied in this case, as you're only
> touching files in the simulator, which is part of the GDB project]
>
>> 2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> * common/sim-options.c (sim_args_command): Call freeargv() when
>> failure occurs.
>> * mcore/interp.c (sim_do_command): Call freeargv() before return.
>> * microblaze/interp.c (sim_do_command): Call freeargv() before
>> return.
>
> Small procedural request, Chen. Those 3 changes are pretty much
> independent, so it's highly preferable to submit them separately.
> This has a number of advantages: We can review each one of them
> individually, with possibly different reviewers, and that makes
> tracking of which part has been reviewed a lot easier. Also, by
> having them submitted separately, you can have one patch per piece,
> which means that if one patch turns out to be incorrect, we can
> easily revert just that patch using git, rather than doing a semi-
> revert by hand.
>
OK, thanks. I shall send 3 separated patches for it within this month.
Thanks.
On 29 Jan 2015 06:19, Chen Gang S wrote:
> On 1/28/15 23:53, Michael Eager wrote:
> > On 01/28/15 03:45, Chen Gang S wrote:
> >> buildargv() and freeargv() are pairs, so need be sure of them always
> >> paired to avoid memory leak.
> >
> > There appear to be other places where buildargv() is not followed by
> > freeargv(). See sim/common/run.c. There may be others.
>
> For me, I intended to skip buildargv() in "sim/common/run.c", because it
> may contents read only memory. It is in main(), also main() often uses
> exit(), so I skip it, it doesn't matter.
i'm not too worried about run.c. in fact, the more it bitrots the better ... it
gets people to switch to nrun.c :).
-mike
@@ -1,3 +1,11 @@
+2015-01-28 Chen Gang <gang.chen.5i5j@gmail.com>
+
+ * common/sim-options.c (sim_args_command): Call freeargv() when
+ failure occurs.
+ * mcore/interp.c (sim_do_command): Call freeargv() before return.
+ * microblaze/interp.c (sim_do_command): Call freeargv() before
+ return.
+
2014-07-01 Chen Gang <gang.chen.5i5j@gmail.com>
* sim/microblaze/interp.c: Use long int format instead of int
@@ -993,7 +993,10 @@ sim_args_command (SIM_DESC sd, const char *cmd)
sim_cpu *cpu;
if (argv [0] == NULL)
- return SIM_RC_OK; /* FIXME - perhaps help would be better */
+ {
+ freeargv (argv);
+ return SIM_RC_OK; /* FIXME - perhaps help would be better */
+ }
/* First check for a cpu selector. */
{
@@ -2143,6 +2143,7 @@ sim_do_command (sd, cmd)
if ((simargv[1] == NULL) || (simargv[2] == NULL))
{
fprintf (stderr, "Error: missing argument to watch cmd.\n");
+ freeargv (simargv);
return;
}
@@ -2187,6 +2188,8 @@ sim_do_command (sd, cmd)
fprintf (stderr,"Error: \"%s\" is not a valid M.CORE simulator command.\n",
cmd);
}
+
+ freeargv (simargv);
}
else
{
@@ -1019,6 +1019,7 @@ sim_do_command (SIM_DESC sd, const char *cmd)
if ((simargv[1] == NULL) || (simargv[2] == NULL))
{
fprintf (stderr, "Error: missing argument to watch cmd.\n");
+ freeargv (simargv);
return;
}
@@ -1062,6 +1063,8 @@ sim_do_command (SIM_DESC sd, const char *cmd)
fprintf (stderr,"Error: \"%s\" is not a valid M.CORE simulator command.\n",
cmd);
}
+
+ freeargv (simargv);
}
else
{