Re: [RFC] Implement -list-thread-groups.

[Michael Snyder <msnyder@vmware.com>]

Vladimir Prus wrote:
> On Friday 14 November 2008 22:41:58 Michael Snyder wrote:
>> Vladimir Prus wrote:
>>> On Friday 14 November 2008 21:54:46 Michael Snyder wrote:
>>>
>>>>>> I'm puzzled by this assert.
>>>>>> You don't think we'll ever want to specify both the pid and the thread?
>>>>> I think that makes no sense. If a thread is specified, then there's no
>>>>> possible use of 'pid'. Threads are globally numbered.
>>>> Even if it makes no sense in the sense that
>>>> it's not required, that doesn't necessarily make it
>>>> an error.  Suppose somebody specifies both the pid and
>>>> the thread?  What's the harm?  If they're inconsistent
>>>> (this pid does not contain this thread), THEN we'll
>>>> return an error.
>>> I think it's better to make functions have as tight preconditions as possible. 
>>> In this case, passing both thread and pid does not serve any possible purpose,
>>> so it's likely that caller is doing this by mistake. It's best to assert 
>>> immediately, rather than spending time and code space verifying if those
>>> parameters are consistent.
>> I respect your opinion, but MI is not the only caller of this function.
>>
>>  > Checking if a thread belongs to a process is not
>>> the part of this this function purpose.
>> It's input validation.  What you're doing is also input
>> validation, it's just imposing a more stringent requirement.
>>
>> I feel that an assert is excessively stringent in this context.
>> An assert implies an internal gdb error.  These potentially
>> conflicting inputs could come about as a result of (foreseeable)
>> user input, rather than internal error.  Admittedly not any
>> user input that could be given now, but the CLI (or other
>> potential clients) could change.
>>
>> I feel that if it's possible for these inputs to violate
>> the assert without actually reflecting an internally
>> inconsistant state, then the assert is too strong.
> 
> This is not the question of what *external* inputs, or user-defined
> inputs can be meaningful. It's the question of what the function
> promises. In my original patch, the function, in its comment, did not
> say anything about behaviour in the case where both thread and pid
> are not -1. Therefore, any caller of this function that can possible
> pass thread!=-1 and pid!=-1 gets undefined behaviour. There are 3 ways
> from here:
> 
> 1. Document that thread!=-1 && pid!=-1 is invalid parameter set of this function.
> Add gdb_assert.
> 
> 2. Document, exactly, the behaviour in thread!=-1 && pid !=-1 case.
> 
> 3. Leave everything as is -- e.g. with undefined behaviour.
> 
> (3) is not good, for obvious reasons. If you don't like (1), then can you specify
> what behaviour you want from this function in the thread!=-1 && pid !=-1 case,
> so that I can document and implement it?

Sounds good, and well summarized.

What about this for #2:
   1) Look up the thread based on TID as you already do.
   2) Compare the thread's PID with the supplied PID.
   3) If they match, print the result.  If not, return error / not found.

Sound reasonable?