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

[Alistair Popple]

Thanks Nick,

On Thursday, 9 August 2018 2:28:45 PM AEST Nicholas Piggin wrote:
> Thread state is not queried from the target each time, but cached.
> This patch adds a valid flag in the thread state cache to ensure
> this is being probed proprly before use.

I really need to add some comments/documentation but there is already a
pdbg_target_status() function which is supposed to indicate if a particular
target has been initialised or not (and therefore if thread_status() is
initialised).

So rather than adding a target specific valid flag I think it would be best to
just check that. There is also an implicit (ie. undocumented :-) rule that
target functions other than target_probe() should only be called on targets with
status == PDBG_TARGET_ENABLED.

> This also skips threads that were not targetted when printing thread
> status.

This should already have been dealt with by the for_each_target calls in
src/main.c. Ie:

> @@ -28,6 +29,7 @@ static int print_thread_status(struct pdbg_target *target, uint32_t index, uint6
>  	struct thread_state *status = (struct thread_state *) arg;
>  
>  	status[index] = thread_status(target);

This should only get called for selected targets, so really the problem is that
some elements of status[] may not be initialised due to the target not being
selected/probed/found.

>  	return 1;
>  }
>  
> @@ -36,12 +38,19 @@ static int print_core_thread_status(struct pdbg_target *core_target, uint32_t in
>  	struct thread_state status[8];
>  	int i, rc;
>  
> +	memset(status, 0, sizeof(status));
> +
>  	printf("c%02d:  ", index);
>  
>  	/* TODO: This cast is gross. Need to rewrite for_each_child_target as an iterator. */
>  	rc = for_each_child_target("thread", core_target, print_thread_status, (uint64_t *) &status[0], NULL);
>  	for (i = 0; i <= *maxindex; i++) {

I think the correct fix is probably to use the second argument in the callback
to detect if status[i] is initialised/selected/etc. Yes that TODO statement
still needs to done :-) It would likely make this code much clearer.

- Alistair

> +		if (!status[i].valid) {
> +			printf("    ");
> +			continue;
> +		}
> +
>  		if (status[i].active)
>  			printf("A");
>  		else
>