[Pdbg] [PATCH] libpdbg: ensure thread state validity, only print requested targets

[Alistair Popple]

> > > Oh it's not the target status field but the thread_state one (which is
> > > of course target specific), so I think it's okay. Using the target
> > > status maybe works, but we pass around that thread_state structure
> > > without the target in some places, also could target status be used
> > > with things like ramming that wants to check status of non-target
> > > siblings? (not that I've added the valid check there, but I should)  
> > 
> > Not quite sure I follow, but what I meant is we should assume:
> > 
> > 1) If pdbg_target_status(target) == PDBG_TARGET_ENABLED then
> >    thread_status(target) will always return a valid thread_state.
> > 
> > 2) If pdbg_target_status(target) != PDBG_TARGET_ENABLED then we shouldn't be
> >    calling thread_status(target) as the target hasn't been initialised and
> >    doesn't even exist so it can't have a status.
> > 
> > Therefore I didn't quite understand why we needed a seperate bit tracking
> > thread_state validity as it should always be valid or non-existant.
> 
> Well we don't need it if we can do it another way. It's not a target
> specific bit through, it's specifically part of the thread status which
> So I don't think that should exclude this approach. Actually a
> positive point considering we pass that structure around a bit
> (although arguably it's a bit ugly when we do that).

I guess my point is that the structure is part of the library API, and if a
thread_state is obtained from the library it must be valid as I don't think it
makes sense to have an API that would return an invalid thread_state. If the
application (src/*) wants to pass it around in ugly ways that's fine - but it's
up to the application to track if it has initialised a particular instance of
thread_state or not rather than embedding application concerns in the library
API.

> > 
> > For checking status of all thread siblings on a given core you could do
> > something like:
> > 
> > pdbg_for_each_target("thread", core, sibling_thread) {
> > 	assert(pdbg_target_probe(sibling_thread) == PDBG_TARGET_ENABLED);
> > 	thread_state = thread_status(sibling_thread);
> > }
> > 
> > This is effectively the check we do in p9_ram_setup() today, although that code
> > is a little old and needs some cleanup to make it look more like the above which
> > is what I'm doing now.
> 
> I suppose so. I wonder if we should go the other way though and
> take thread status out of probing. Do it on demand the first time
> we're asked for a status. Maybe that doesn't save many scoms in
> most cases.
> 
> Taking a step back I wonder if we shouldn't query status with scoms
> every time rather than cache it. I wonder how much the cache buys,
> it certainly makes it more complex and error prone to ensure it is up
> to date properly.

I was wondering that as well :-)

Looking back I think historically it was being used to track some other private
thread state during instruction ramming prior to cleaning up targetting, etc. I
certainly can't come up with a good reason for it to exist today so will happily
look at ripping it out.

With regards to the issue here I am happy to come up with an alternate.

Thanks,

Alistair

> Thanks,
> Nick
> 
> 
>