Commit Message
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.
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.
---
gdb/ChangeLog | 15 +++++++++++
gdb/ada-lang.h | 2 --
gdb/ada-tasks.c | 58 ++++++++++++++++++++++--------------------
gdb/ravenscar-thread.c | 2 --
4 files changed, 45 insertions(+), 32 deletions(-)
Comments
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" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> One question independent of this patch first: I am wondering - how
Joel> do you deal with submissions that you want to "git am" but have
Joel> a ChangeLog patch in it that cause a merge failure?
This was a bug in my script to send email -- it failed to remove the
ChangeLog hunk when sending from a git worktree. I've just fixed it.
You can see my scripts here: https://github.com/tromey/git-gnu-changelog
For the case of applying a patch, I use git-merge-changelog, from
gnulib. Fedora packages it so you can just "dnf install" it. This
avoids most problems arising from applying patches with ChangeLogs (I
think I had one problem in the last few years).
Mark once wrote up some instructions that I still follow when setting
this up:
https://gnu.wildebeest.org/blog/mjw/2012/03/16/automagically-merging-changelog-files-with-mercurial-or-git/
Tom
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> Since this code is used on all platforms and native platforms
Joel> in particular have a lot more testcases that exercise this code,
Joel> it would be nice to exercise it both with the build-bot and
Joel> AdaCore's testsuite.
I ran it through the buildbot as well.
Fedora's buildbot install is broken :-(
https://bugzilla.redhat.com/show_bug.cgi?id=1661951
I had previous made a virtualenv for buildbot, but today the
straightforward approach didn't work. Here's the recipe I ended up
using:
cd ~
python2 -m virtualenv buildbot
. buildbot/bin/activate
pip install buildbot==1.7.0
Joel> Other than that, the patch looks good to me; if the patch with
Joel> the change I suggested passes testing, it's OK to push.
I made the change & I'm going to check it in momentarily.
Tom
@@ -1,3 +1,18 @@
+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.
+
2019-02-15 Tom Tromey <tromey@adacore.com>
* ravenscar-thread.c (ravenscar_thread_target::resume)
@@ -411,8 +411,6 @@ extern void iterate_over_live_ada_tasks
extern const char *ada_get_tcb_types_info (void);
-extern int ada_build_task_list (void);
-
extern void print_ada_task_info (struct ui_out *uiout,
char *taskno_str,
struct inferior *inf);
@@ -26,6 +26,8 @@
#include "progspace.h"
#include "objfiles.h"
+static int ada_build_task_list ();
+
/* The name of the array in the GNAT runtime where the Ada Task Control
Block of each task is stored. */
#define KNOWN_TASKS_NAME "system__tasking__debug__known_tasks"
@@ -221,7 +223,7 @@ struct ada_tasks_inferior_data
/* When nonzero, this flag indicates that the task_list field
below is up to date. When set to zero, the list has either
not been initialized, or has potentially become stale. */
- int task_list_valid_p = 0;
+ bool task_list_valid_p = false;
/* The list of Ada tasks.
@@ -785,9 +787,9 @@ add_ada_task (CORE_ADDR task_id, struct inferior *inf)
}
/* Read the Known_Tasks array from the inferior memory, and store
- it in the current inferior's TASK_LIST. Return non-zero upon success. */
+ it in the current inferior's TASK_LIST. Return true upon success. */
-static int
+static bool
read_known_tasks_array (struct ada_tasks_inferior_data *data)
{
const int target_ptr_byte = TYPE_LENGTH (data->known_tasks_element);
@@ -808,13 +810,13 @@ read_known_tasks_array (struct ada_tasks_inferior_data *data)
add_ada_task (task_id, current_inferior ());
}
- return 1;
+ return true;
}
/* Read the known tasks from the inferior memory, and store it in
- the current inferior's TASK_LIST. Return non-zero upon success. */
+ the current inferior's TASK_LIST. Return true upon success. */
-static int
+static bool
read_known_tasks_list (struct ada_tasks_inferior_data *data)
{
const int target_ptr_byte = TYPE_LENGTH (data->known_tasks_element);
@@ -825,7 +827,7 @@ read_known_tasks_list (struct ada_tasks_inferior_data *data)
/* Sanity check. */
if (pspace_data->atcb_fieldno.activation_link < 0)
- return 0;
+ return false;
/* Build a new list by reading the ATCBs. Read head of the list. */
read_memory (data->known_tasks_addr, known_tasks, target_ptr_byte);
@@ -846,7 +848,7 @@ read_known_tasks_list (struct ada_tasks_inferior_data *data)
pspace_data->atcb_fieldno.activation_link));
}
- return 1;
+ return true;
}
/* Set all fields of the current inferior ada-tasks data pointed by DATA.
@@ -944,11 +946,10 @@ ada_tasks_inferior_data_sniffer (struct ada_tasks_inferior_data *data)
}
/* Read the known tasks from the current inferior's memory, and store it
- in the current inferior's data TASK_LIST.
- Return non-zero upon success. */
+ in the current inferior's data TASK_LIST. */
-static int
-read_known_tasks (void)
+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;
}
-
- /* Step 3: Set task_list_valid_p, to avoid re-reading the Known_Tasks
- array unless needed. Then report a success. */
- data->task_list_valid_p = 1;
-
- return 1;
}
/* Build the task_list by reading the Known_Tasks array from
the inferior, and return the number of tasks in that list
(zero means that the program is not using tasking at all). */
-int
-ada_build_task_list (void)
+static int
+ada_build_task_list ()
{
struct ada_tasks_inferior_data *data;
@@ -1343,7 +1345,7 @@ ada_task_list_changed (struct inferior *inf)
{
struct ada_tasks_inferior_data *data = get_ada_tasks_inferior_data (inf);
- data->task_list_valid_p = 0;
+ data->task_list_valid_p = false;
}
/* Invalidate the per-program-space data. */
@@ -1362,7 +1364,7 @@ ada_tasks_invalidate_inferior_data (struct inferior *inf)
struct ada_tasks_inferior_data *data = get_ada_tasks_inferior_data (inf);
data->known_tasks_kind = ADA_TASKS_UNKNOWN;
- data->task_list_valid_p = 0;
+ data->task_list_valid_p = false;
}
/* The 'normal_stop' observer notification callback. */
@@ -374,8 +374,6 @@ ravenscar_add_thread (struct ada_task_info *task)
void
ravenscar_thread_target::update_thread_list ()
{
- ada_build_task_list ();
-
/* Do not clear the thread list before adding the Ada task, to keep
the thread that the process stratum has included into it
(m_base_ptid) and the running thread, that may not have been included