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

Nicholas Piggin npiggin at gmail.com
Fri Aug 10 13:33:20 AEST 2018


On Fri, 10 Aug 2018 13:05:14 +1000
Alistair Popple <alistair at popple.id.au> wrote:

> > > > 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.

Yeah that may be a fair point.

> 
> > > 
> > > 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.

Yeah my vote is rip out the cache and make it a call, that makes things
easier and less error prone. We can think about how to set validity for
the state printing some other way (another array or something would be
fine).

Thanks,
Nick


More information about the Pdbg mailing list