Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB

[Pedro Alves <palves@redhat.com>]

On 01/11/2013 05:11 PM, Jan Kratochvil wrote:
> On Fri, 11 Jan 2013 18:03:01 +0100, Sergio Durigan Junior wrote:
>> On Friday, January 11 2013, Pedro Alves wrote:
>>> On 01/11/2013 03:53 AM, Sergio Durigan Junior wrote:
>>>>>> +  /* Generating and copying the program's arguments.  `get_inferior_args'
>>>>>> +     may throw, but we want to continue the execution anyway.  */
>>>>>> +  TRY_CATCH (ex, RETURN_MASK_ERROR)
>>>>>> +    {
>>>>>> +      infargs = get_inferior_args ();
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> Hmm?  We were not doing that before.  What exception is that?
>>>>
>>>> `get_inferior_args' calls `construct_inferior_arguments', which can call
>>>> `error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
>>>> contain spaces).
>>>
>>> This is an example of something that should be split into
>>> its own change, along with its own rationale.  This is
>>> independent of any refactoring of PRPSINFO handling.
>>> We're already calling get_inferior_args nowadays, and I don't
>>> ever remember this error being reported as a problem.
>>
>> My first version of the patch didn't contain the TRY_CATCH part.  It was
>> Jan who made this suggestion, and I thought it made sense.

Sorry, I missed that.

>> I really think a TRY_CATCH does not cause any harm here, but if you
>> insist, I can easily remove it from the patch.
> 
> I do not think there should be code leaking memory in a case of throwsn
> exception when the callee contains an error() call.  

Agreed, of course.

> Even if the error() call is only in a conditional which with
> the current setup around can never happen.

Agree.  IMO, strictly considering the leak, a cleanup for whatever
is leaking would be better though.

> I agree one can improve get_inferior_args in a way it no longer throws.
> That would be another way to fix it.  I do not mind which way the memory leak
> of linux_nat_fill_prpsinfo gets fixed.

-- 
Pedro Alves