Re: [PATCH] Minor Ada task cleanups

Joel Brobecker
2019-02-17T14:57:18Z

Hi Tom,

On Fri, Feb 15, 2019 at 02:22:53PM -0700, Tom Tromey wrote:
> While working on the Ada task code, I noticed a few things that could
> be cleaned up:
>
> * task_list_valid_p was not set in all cases in ada_build_task_list.
> This causes many needless re-fetches of the task list.
>
> * task_list_valid_p can be bool, and various functions can also return
> bool.
>
> * Nothing checks the return value of read_known_tasks, so it can be
> changed to return void.
>
> * The call to ada_build_task_list in
> ravenscar_thread_target::update_thread_list is redundant, because
> this is the first thing done by iterate_over_live_ada_tasks.
>
> Tested using the internal AdaCore test suite against a ravenscar
> target.

Thanks for doing this. Nice cleanup :).

One question independent of this patch first: I am wondering - how
do you deal with submissions that you want to "git am" but have
a ChangeLog patch in it that cause a merge failure? I've needed to
apply your patch a few times, and the only option I knew was to
remove the hungs for the ChangeLog files.

> gdb/ChangeLog
> 2019-02-15 Tom Tromey <tromey@adacore.com>
>
> * ravenscar-thread.c
> (ravenscar_thread_target::update_thread_list): Don't call
> ada_build_task_list.
> * ada-lang.h (ada_build_task_list): Don't declare.
> * ada-tasks.c (struct ada_tasks_inferior_data)
> <task_list_valid_p>: Now bool.
> (read_known_tasks, ada_task_list_changed)
> (ada_tasks_invalidate_inferior_data): Update.
> (read_known_tasks_array): Return bool.
> (read_known_tasks_list): Likewise.
> (read_known_tasks): Return void.
> (ada_build_task_list): Now static.

Since this code is used on all platforms and native platforms
in particular have a lot more testcases that exercise this code,
it would be nice to exercise it both with the build-bot and
AdaCore's testsuite.

One minor nit below.

Other than that, the patch looks good to me; if the patch with
the change I suggested passes testing, it's OK to push.

Thank you!

> +static void
> +read_known_tasks ()
> {
> struct ada_tasks_inferior_data *data =
> get_ada_tasks_inferior_data (current_inferior ());
> @@ -965,29 +966,30 @@ read_known_tasks (void)
> ada_tasks_inferior_data_sniffer (data);
> gdb_assert (data->known_tasks_kind != ADA_TASKS_UNKNOWN);
>
> + /* Step 3: Set task_list_valid_p, to avoid re-reading the Known_Tasks
> + array unless needed. */
> switch (data->known_tasks_kind)
> {
> - case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior. */
> - return 0;
> - case ADA_TASKS_ARRAY:
> - return read_known_tasks_array (data);
> - case ADA_TASKS_LIST:
> - return read_known_tasks_list (data);
> + case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior. */
> + break;
> + case ADA_TASKS_ARRAY:
> + data->task_list_valid_p = read_known_tasks_array (data);
> + break;
> + case ADA_TASKS_LIST:
> + data->task_list_valid_p = read_known_tasks_list (data);
> + break;
> + case ADA_TASKS_UNKNOWN:
> + data->task_list_valid_p = true;
> + break;

The last case is unnecessary, as we have just asserted above
that data->known_tasks_kind != ADA_TASKS_UNKNOWN.

--
Joel