Minor Ada task cleanups

Message ID 20190215212253.25409-1-tromey@adacore.com
State New
Headers show
Series
  • Minor Ada task cleanups
Related show

Commit Message

Tom Tromey Feb. 15, 2019, 9:22 p.m.
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(-)

-- 
2.20.1

Comments

Joel Brobecker Feb. 17, 2019, 2:57 p.m. | #1
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
Tom Tromey Feb. 19, 2019, 1:13 p.m. | #2
>>>>> "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
Tom Tromey Feb. 19, 2019, 7:25 p.m. | #3
>>>>> "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

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3ef09fd733b..b1f69402a26 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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)
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 14470d5d434..ee03dbd2aad 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -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);
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index e994147a669..beef575c669 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -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.  */
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 05a83200a1e..cb4f26a3472 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -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