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

[Vladimir Prus <vladimir at codesourcery dot com>]

On Sunday 16 November 2008 00:12:21 Michael Snyder wrote:
> 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?

The function does not have a return value, so 'error' is the
only way to do reporting. Is that what you suggest?

- Volodya