[PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[Kévin Le Gouguec]

For some boards, the Ada tasking runtime defines a __gnat_gdb_cpu_first_id
symbol to let GDB map the ATCB's "Base_CPU" indices (ranging from 1 to
System.Multiprocessors.CPU) onto the CPU numbers that we read from the
remote stub and record in our ptids.

In other words, this symbol lets GDB translate "Ada CPUs" into "target
CPUs".  For example, for the Microchip PolarFire board, the runtime defines
this offset as:

  package System.BB.Board_Parameters is
     -- [...]
     GDB_First_CPU_Id : constant Interfaces.Unsigned_32 := 1;
     pragma Export (C, GDB_First_CPU_Id, "__gnat_gdb_cpu_first_id");

This is because on this board, CPU#1 is the "monitor" CPU; CPUs #2-5 are the
"application" CPUs that run user code.  So while the ATCB shows Base_CPUs
equal to 1, QEMU reports that our code is running on CPU #2.

We convert the ATCB's Base_CPUs into "target" CPUs before recording them in
struct ada_task_info.base_cpu; this is what we need in most of
ravenscar-thread.c, except in one specific spot: when reading the active
thread from __gnat_running_thread_table, which is defined as:

  package System.BB.Threads.Queues is
     -- [...]
     Running_Thread_Table : array (System.Multiprocessors.CPU) of Thread_Id

On baremetal targets, System.Multiprocessors.CPU is defined as:

  package System.Multiprocessors is
     -- [...]
     type CPU_Range is range 0 .. System.BB.Parameters.Max_Number_Of_CPUs;

     subtype CPU is CPU_Range range 1 .. CPU_Range'Last;

     Not_A_Specific_CPU : constant CPU_Range := 0;

Thus __gnat_running_thread_table has Max_Number_Of_CPUs elements; for the
Microchip PolarFire board, the runtime define this as:

  package System.BB.Parameters is
     -- [...]
     Max_Number_Of_CPUs : constant := 1;

So the table has just one element, but ravenscar-thread.c attempts to index
it using the "target" CPU ID, i.e. 2.

This remained undetected because as luck would have it, with the specific
compiler we were using, *(__gnat_running_thread_table+8) happened to contain
exactly the same content as *__gnat_running_thread_table.  After bumping the
compiler, the layout of the tasking runtime changed, and so did the content
of *(__gnat_running_thread_table+8).

This commit introduces a new function to let GDB convert a "target" CPU back
to an "Ada" CPU.

Tested on x86_64-linux and riscv64-elf with AdaCore's internal testsuite,
which has more extensive coverage of Ada tasking and "Ravenscar thread"
features.
---
 gdb/ada-lang.h         |  8 +++++++-
 gdb/ada-tasks.c        | 14 ++++++++++++++
 gdb/ravenscar-thread.c |  8 +++++++-
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 9fb7ac7f384..bca0deea67f 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -145,7 +145,11 @@ struct ada_task_info
   /* The CPU on which the task is running.  This is dependent on
      the runtime actually providing that info, which is not always
      the case.  Normally, we should be able to count on it on
-     bare-metal targets.  */
+     bare-metal targets.
+
+     NB: This CPU number has been normalized to match the IDs reported by the
+     target, as recorded in the LWP field of PTIDs.  It may not map directly to
+     the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index.  */
   int base_cpu;
 };
 
@@ -374,6 +378,8 @@ extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
 
 extern int ada_get_task_number (thread_info *thread);
 
+extern int ada_get_runtime_cpu_index (int target_cpu);
+
 typedef gdb::function_view<void (struct ada_task_info *task)>
   ada_task_list_iterator_ftype;
 extern void iterate_over_live_ada_tasks
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index a3a28063daa..1e5605f9b62 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -346,6 +346,20 @@ ada_get_task_number (thread_info *thread)
   return 0;  /* No matching task found.  */
 }
 
+/* Translate a "target" CPU index into a "runtime" index suitable for addressing
+   arrays dimensioned with System.Multiprocessors.CPU.  */
+
+int
+ada_get_runtime_cpu_index (int target_cpu)
+{
+  const struct ada_tasks_pspace_data *pspace_data
+    = get_ada_tasks_pspace_data (current_program_space);
+
+  gdb_assert (pspace_data->initialized_p);
+
+  return target_cpu - pspace_data->cpu_id_offset;
+}
+
 /* Return the task number of the task running in inferior INF which
    matches TASK_ID , or zero if the task could not be found.  */
  
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 22fbdbe9662..52908eb2bb5 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -193,7 +193,11 @@ struct ravenscar_thread_target final : public target_ops
   /* This maps a TID to the CPU on which it was running.  This is
      needed because sometimes the runtime will report an active task
      that hasn't yet been put on the list of tasks that is read by
-     ada-tasks.c.  */
+     ada-tasks.c.
+
+     NB: These CPU numbers correspond to those reported by the target,
+     which may differ from the numbers recorded in the ATCB.  See
+     ada_get_runtime_cpu_index.  */
   std::unordered_map<ULONGEST, int> m_cpu_map;
 };
 
@@ -377,6 +381,8 @@ ravenscar_thread_target::runtime_initialized ()
 static CORE_ADDR
 get_running_thread_id (int cpu)
 {
+  cpu = ada_get_runtime_cpu_index (cpu);
+
   struct bound_minimal_symbol object_msym = get_running_thread_msymbol ();
   int object_size;
   int buf_size;
-- 
2.25.1

Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[Simon Marchi]

On 1/12/23 12:10, Kévin Le Gouguec via Gdb-patches wrote:
> For some boards, the Ada tasking runtime defines a __gnat_gdb_cpu_first_id
> symbol to let GDB map the ATCB's "Base_CPU" indices (ranging from 1 to
> System.Multiprocessors.CPU) onto the CPU numbers that we read from the
> remote stub and record in our ptids.
> 
> In other words, this symbol lets GDB translate "Ada CPUs" into "target
> CPUs".  For example, for the Microchip PolarFire board, the runtime defines
> this offset as:
> 
>   package System.BB.Board_Parameters is
>      -- [...]
>      GDB_First_CPU_Id : constant Interfaces.Unsigned_32 := 1;
>      pragma Export (C, GDB_First_CPU_Id, "__gnat_gdb_cpu_first_id");
> 
> This is because on this board, CPU#1 is the "monitor" CPU; CPUs #2-5 are the
> "application" CPUs that run user code.  So while the ATCB shows Base_CPUs
> equal to 1, QEMU reports that our code is running on CPU #2.
> 
> We convert the ATCB's Base_CPUs into "target" CPUs before recording them in
> struct ada_task_info.base_cpu; this is what we need in most of
> ravenscar-thread.c, except in one specific spot: when reading the active
> thread from __gnat_running_thread_table, which is defined as:
> 
>   package System.BB.Threads.Queues is
>      -- [...]
>      Running_Thread_Table : array (System.Multiprocessors.CPU) of Thread_Id
> 
> On baremetal targets, System.Multiprocessors.CPU is defined as:
> 
>   package System.Multiprocessors is
>      -- [...]
>      type CPU_Range is range 0 .. System.BB.Parameters.Max_Number_Of_CPUs;
> 
>      subtype CPU is CPU_Range range 1 .. CPU_Range'Last;
> 
>      Not_A_Specific_CPU : constant CPU_Range := 0;
> 
> Thus __gnat_running_thread_table has Max_Number_Of_CPUs elements; for the
> Microchip PolarFire board, the runtime define this as:
> 
>   package System.BB.Parameters is
>      -- [...]
>      Max_Number_Of_CPUs : constant := 1;
> 
> So the table has just one element, but ravenscar-thread.c attempts to index
> it using the "target" CPU ID, i.e. 2.
> 
> This remained undetected because as luck would have it, with the specific
> compiler we were using, *(__gnat_running_thread_table+8) happened to contain
> exactly the same content as *__gnat_running_thread_table.  After bumping the
> compiler, the layout of the tasking runtime changed, and so did the content
> of *(__gnat_running_thread_table+8).
> 
> This commit introduces a new function to let GDB convert a "target" CPU back
> to an "Ada" CPU.
> 
> Tested on x86_64-linux and riscv64-elf with AdaCore's internal testsuite,
> which has more extensive coverage of Ada tasking and "Ravenscar thread"
> features.

Hi Kevin,

I can't speak about the Ada-specific bits, but at least the commit
message makes it like you know what you're doing, so that's convincing
:).

> ---
>  gdb/ada-lang.h         |  8 +++++++-
>  gdb/ada-tasks.c        | 14 ++++++++++++++
>  gdb/ravenscar-thread.c |  8 +++++++-
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
> index 9fb7ac7f384..bca0deea67f 100644
> --- a/gdb/ada-lang.h
> +++ b/gdb/ada-lang.h
> @@ -145,7 +145,11 @@ struct ada_task_info
>    /* The CPU on which the task is running.  This is dependent on
>       the runtime actually providing that info, which is not always
>       the case.  Normally, we should be able to count on it on
> -     bare-metal targets.  */
> +     bare-metal targets.
> +
> +     NB: This CPU number has been normalized to match the IDs reported by the
> +     target, as recorded in the LWP field of PTIDs.  It may not map directly to
> +     the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index.  */
>    int base_cpu;
>  };
>  
> @@ -374,6 +378,8 @@ extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
>  
>  extern int ada_get_task_number (thread_info *thread);
>  
> +extern int ada_get_runtime_cpu_index (int target_cpu);

To conform with the current standards, please move the function doc
here.

> +
>  typedef gdb::function_view<void (struct ada_task_info *task)>
>    ada_task_list_iterator_ftype;
>  extern void iterate_over_live_ada_tasks
> diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
> index a3a28063daa..1e5605f9b62 100644
> --- a/gdb/ada-tasks.c
> +++ b/gdb/ada-tasks.c
> @@ -346,6 +346,20 @@ ada_get_task_number (thread_info *thread)
>    return 0;  /* No matching task found.  */
>  }
>  
> +/* Translate a "target" CPU index into a "runtime" index suitable for addressing
> +   arrays dimensioned with System.Multiprocessors.CPU.  */


And here, put:

/* See ada-lang.h.  */

(Ideally there would be an ada-tasks.h to match ada-tasks.c.)

> +
> +int
> +ada_get_runtime_cpu_index (int target_cpu)
> +{
> +  const struct ada_tasks_pspace_data *pspace_data
> +    = get_ada_tasks_pspace_data (current_program_space);

I would prefer if you added either a `program_space *` or
`ada_tasks_pspace_data *` to this function, and pushed the reference to
the global state to the caller.  My life goal is to reduce the number of
places that reference the global state in GDB.

Or, maybe, make this a method of ada_tasks_pspace_data?  The caller
would then to:

  const ada_tasks_pspace_data *pspace_data
    = get_ada_tasks_pspace_data (current_program_space);
  cpu = pspace_data->get_runtime_cpu_index (cpu);

Simon

Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[Kévin Le Gouguec]

Hey Simon, thanks for the review!

Simon Marchi <simark@simark.ca> writes:

> I can't speak about the Ada-specific bits, but at least the commit
> message makes it like you know what you're doing, so that's convincing
> :).

(I've slowly come to believe I know what I am doing, but if not,
hopefully this commit message will be enough to trigger Cunningham's
Law)

>> diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
>> index 9fb7ac7f384..bca0deea67f 100644
>> --- a/gdb/ada-lang.h
>> +++ b/gdb/ada-lang.h
>> @@ -145,7 +145,11 @@ struct ada_task_info
>>    /* The CPU on which the task is running.  This is dependent on
>>       the runtime actually providing that info, which is not always
>>       the case.  Normally, we should be able to count on it on
>> -     bare-metal targets.  */
>> +     bare-metal targets.
>> +
>> +     NB: This CPU number has been normalized to match the IDs reported by the
>> +     target, as recorded in the LWP field of PTIDs.  It may not map directly to
>> +     the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index.  */
>>    int base_cpu;
>>  };
>>  
>> @@ -374,6 +378,8 @@ extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
>>  
>>  extern int ada_get_task_number (thread_info *thread);
>>  
>> +extern int ada_get_runtime_cpu_index (int target_cpu);
>
> To conform with the current standards, please move the function doc
> here.

ACK, traced back to

https://sourceware.org/gdb/wiki/Internals GDB-C-Coding-Standards#Document_Every_Subprogram

Will give this a re-read before submitting v2.

>> +
>>  typedef gdb::function_view<void (struct ada_task_info *task)>
>>    ada_task_list_iterator_ftype;
>>  extern void iterate_over_live_ada_tasks
>> diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
>> index a3a28063daa..1e5605f9b62 100644
>> --- a/gdb/ada-tasks.c
>> +++ b/gdb/ada-tasks.c
>> @@ -346,6 +346,20 @@ ada_get_task_number (thread_info *thread)
>>    return 0;  /* No matching task found.  */
>>  }
>>  
>> +/* Translate a "target" CPU index into a "runtime" index suitable for addressing
>> +   arrays dimensioned with System.Multiprocessors.CPU.  */
>
>
> And here, put:
>
> /* See ada-lang.h.  */
>
> (Ideally there would be an ada-tasks.h to match ada-tasks.c.)

Agreed, might explore that in a preliminary commit for v2.

>> +
>> +int
>> +ada_get_runtime_cpu_index (int target_cpu)
>> +{
>> +  const struct ada_tasks_pspace_data *pspace_data
>> +    = get_ada_tasks_pspace_data (current_program_space);
>
> I would prefer if you added either a `program_space *` or
> `ada_tasks_pspace_data *` to this function, and pushed the reference to
> the global state to the caller.  My life goal is to reduce the number of
> places that reference the global state in GDB.
>
> Or, maybe, make this a method of ada_tasks_pspace_data?  The caller
> would then to:
>
>   const ada_tasks_pspace_data *pspace_data
>     = get_ada_tasks_pspace_data (current_program_space);
>   cpu = pspace_data->get_runtime_cpu_index (cpu);

I like the idea, although right now ada_tasks_pspace_data is completely
internal to ada-tasks.c.  ravenscar-thread.c only has access to
ada_task_info instances; we could stick the CPU offset in there, but
conceptually it's not really a property of the task.

AFAICT this patch follows the current pattern of ravenscar-thread.c
letting ada-tasks.c access globals: ada-tasks.c currently has 18
accesses to current_\(program_space\|inferior\), vs 3 calls to
current_inferior in ravenscar-thread.c, which does not even include
progspace.h.

So by my reckoning:

(a) this v1 is no worse than the status quo in terms of accessing
globals,

(b) naively pushing current_program_space to the caller would mean
making ravenscar-thread.c aware of current_program_space, so a net loss
according to the "reduce the number of references to global state in
GDB" metric,

(c) publishing struct ada_tasks_pspace_data and
get_ada_tasks_pspace_data wouldn't be much better (ravenscar-thread.c
would still need to learn about current_program_space), but it does feel
like a step in the right direction?  I.e. it would then "just" be a
matter of…

(d) … somehow adding a struct ada_tasks_pspace_data member to
ravenscar_thread_target.  That would allow
ravenscar_thread_target::active_task to call
m_pspace_data->get_runtime_cpu_index (cpu).

So, with utmost consideration for your life goal, it seems to me that
the solutions, ranked by decreasing order of preference, would be

  (c)+(d) > (a) > (b)

Does that make sense?  If so, I'd like to advocate for (a) (+ the doc
fixes you noted), and write "figure out how to pass an
ada_tasks_pspace_data instance to ravenscar_thread_target" on a post-it
and stick it to my monitor, in order to unblock (d).


Let me know if you'd rather I go with (b), or if I've misread the
situation.  Again, thanks for the review!

Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[Simon Marchi]

On 1/13/23 07:32, Kévin Le Gouguec wrote:
> Hey Simon, thanks for the review!
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>> I can't speak about the Ada-specific bits, but at least the commit
>> message makes it like you know what you're doing, so that's convincing
>> :).
> 
> (I've slowly come to believe I know what I am doing, but if not,
> hopefully this commit message will be enough to trigger Cunningham's
> Law)
> 
>>> diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
>>> index 9fb7ac7f384..bca0deea67f 100644
>>> --- a/gdb/ada-lang.h
>>> +++ b/gdb/ada-lang.h
>>> @@ -145,7 +145,11 @@ struct ada_task_info
>>>    /* The CPU on which the task is running.  This is dependent on
>>>       the runtime actually providing that info, which is not always
>>>       the case.  Normally, we should be able to count on it on
>>> -     bare-metal targets.  */
>>> +     bare-metal targets.
>>> +
>>> +     NB: This CPU number has been normalized to match the IDs reported by the
>>> +     target, as recorded in the LWP field of PTIDs.  It may not map directly to
>>> +     the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index.  */
>>>    int base_cpu;
>>>  };
>>>  
>>> @@ -374,6 +378,8 @@ extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
>>>  
>>>  extern int ada_get_task_number (thread_info *thread);
>>>  
>>> +extern int ada_get_runtime_cpu_index (int target_cpu);
>>
>> To conform with the current standards, please move the function doc
>> here.
> 
> ACK, traced back to
> 
> https://sourceware.org/gdb/wiki/Internals GDB-C-Coding-Standards#Document_Every_Subprogram
> 
> Will give this a re-read before submitting v2.
> 
>>> +
>>>  typedef gdb::function_view<void (struct ada_task_info *task)>
>>>    ada_task_list_iterator_ftype;
>>>  extern void iterate_over_live_ada_tasks
>>> diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
>>> index a3a28063daa..1e5605f9b62 100644
>>> --- a/gdb/ada-tasks.c
>>> +++ b/gdb/ada-tasks.c
>>> @@ -346,6 +346,20 @@ ada_get_task_number (thread_info *thread)
>>>    return 0;  /* No matching task found.  */
>>>  }
>>>  
>>> +/* Translate a "target" CPU index into a "runtime" index suitable for addressing
>>> +   arrays dimensioned with System.Multiprocessors.CPU.  */
>>
>>
>> And here, put:
>>
>> /* See ada-lang.h.  */
>>
>> (Ideally there would be an ada-tasks.h to match ada-tasks.c.)
> 
> Agreed, might explore that in a preliminary commit for v2.
> 
>>> +
>>> +int
>>> +ada_get_runtime_cpu_index (int target_cpu)
>>> +{
>>> +  const struct ada_tasks_pspace_data *pspace_data
>>> +    = get_ada_tasks_pspace_data (current_program_space);
>>
>> I would prefer if you added either a `program_space *` or
>> `ada_tasks_pspace_data *` to this function, and pushed the reference to
>> the global state to the caller.  My life goal is to reduce the number of
>> places that reference the global state in GDB.
>>
>> Or, maybe, make this a method of ada_tasks_pspace_data?  The caller
>> would then to:
>>
>>   const ada_tasks_pspace_data *pspace_data
>>     = get_ada_tasks_pspace_data (current_program_space);
>>   cpu = pspace_data->get_runtime_cpu_index (cpu);
> 
> I like the idea, although right now ada_tasks_pspace_data is completely
> internal to ada-tasks.c.  ravenscar-thread.c only has access to
> ada_task_info instances; we could stick the CPU offset in there, but
> conceptually it's not really a property of the task.

Indeed.

> AFAICT this patch follows the current pattern of ravenscar-thread.c
> letting ada-tasks.c access globals: ada-tasks.c currently has 18
> accesses to current_\(program_space\|inferior\), vs 3 calls to
> current_inferior in ravenscar-thread.c, which does not even include
> progspace.h.
> 
> So by my reckoning:
> 
> (a) this v1 is no worse than the status quo in terms of accessing
> globals,

That's true.

> (b) naively pushing current_program_space to the caller would mean
> making ravenscar-thread.c aware of current_program_space, so a net loss
> according to the "reduce the number of references to global state in
> GDB" metric,

ravenscar-thread.c is already implicitly aware of the current program
space and global state in general, since it calls functions that are
dependent on the global state.  And as you add a new function dependent
on the global state, that's one more function dependent on the global
state in GDB, so I would consider that a "net loss".

When possible, I try to change leaf functions to make them "pure",
receiving the context as parameters rather than using the global
context.  And slowly progress up the call tree like this, making
incremental progress.  It's true that the absolute number of literal
"current_program_space" reference can increase.  Like if I change a
function to accept a program_space parameter, rather than using
"current_program_space" directly, and inline "current_program_space" in
all its callers.  But that's a necessary step towards the end goal.

> 
> (c) publishing struct ada_tasks_pspace_data and
> get_ada_tasks_pspace_data wouldn't be much better (ravenscar-thread.c
> would still need to learn about current_program_space), but it does feel
> like a step in the right direction?  I.e. it would then "just" be a
> matter of…

Whether we have a ada_get_runtime_cpu_index free function or we expose
ada_tasks_pspace_data and add a get_runtime_cpu_index method does not
change anything w.r.t. references to global state.

> 
> (d) … somehow adding a struct ada_tasks_pspace_data member to
> ravenscar_thread_target.  That would allow
> ravenscar_thread_target::active_task to call
> m_pspace_data->get_runtime_cpu_index (cpu).

I don't think that makes sense, we don't want to create intances of
ada_tasks_pspace_data not managed by ada_tasks_pspace_data_handle.

> 
> So, with utmost consideration for your life goal, it seems to me that
> the solutions, ranked by decreasing order of preference, would be
> 
>   (c)+(d) > (a) > (b)

I disagree, I don't think (d) is right.

> Does that make sense?  If so, I'd like to advocate for (a) (+ the doc
> fixes you noted), and write "figure out how to pass an
> ada_tasks_pspace_data instance to ravenscar_thread_target" on a post-it
> and stick it to my monitor, in order to unblock (d).

I'm fine with doing (a) if you prefer, it doesn't change much in the
grand scheme of things.  However, I might just notice that function in
the future being a good candidate for becoming "pure", and change it to
push the global state reference up the call tree :).

Simon

Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads

[Kévin Le Gouguec]

Simon Marchi <simark@simark.ca> writes:

>> (b) naively pushing current_program_space to the caller would mean
>> making ravenscar-thread.c aware of current_program_space, so a net loss
>> according to the "reduce the number of references to global state in
>> GDB" metric,
>
> ravenscar-thread.c is already implicitly aware of the current program
> space and global state in general, since it calls functions that are
> dependent on the global state.  And as you add a new function dependent
> on the global state, that's one more function dependent on the global
> state in GDB, so I would consider that a "net loss".
>
> When possible, I try to change leaf functions to make them "pure",
> receiving the context as parameters rather than using the global
> context.  And slowly progress up the call tree like this, making
> incremental progress.  It's true that the absolute number of literal
> "current_program_space" reference can increase.  Like if I change a
> function to accept a program_space parameter, rather than using
> "current_program_space" directly, and inline "current_program_space" in
> all its callers.  But that's a necessary step towards the end goal.

OK, I see the value of endeavoring to keep newly introduced leaf
functions pure, and "revealing" the implicit dependencies by pushing
references to global state up to the callers.

>> (d) … somehow adding a struct ada_tasks_pspace_data member to
>> ravenscar_thread_target.  That would allow
>> ravenscar_thread_target::active_task to call
>> m_pspace_data->get_runtime_cpu_index (cpu).
>
> I don't think that makes sense, we don't want to create intances of
> ada_tasks_pspace_data not managed by ada_tasks_pspace_data_handle.

ACK; naively I assumed that ravenscar_thread_target holding a reference
to an ada_tasks_pspace_data would be fine as long as the latter outlived
the former, but I had no reason to believe that this condition holds
when I wrote that.

>> Does that make sense?  If so, I'd like to advocate for (a) (+ the doc
>> fixes you noted), and write "figure out how to pass an
>> ada_tasks_pspace_data instance to ravenscar_thread_target" on a post-it
>> and stick it to my monitor, in order to unblock (d).
>
> I'm fine with doing (a) if you prefer, it doesn't change much in the
> grand scheme of things.  However, I might just notice that function in
> the future being a good candidate for becoming "pure", and change it to
> push the global state reference up the call tree :).

I'm game for keeping this new function (ada_get_runtime_cpu_index) pure
from the get go.

So my current plan for v2 (taking the other remarks into account) is
thus to…

1. introduce ada-tasks.h and move stuff defined in ada-tasks.c there,
2. make ada_tasks_pspace_data and its getter public,
3. introduce a get_runtime_cpu_index method,
4. rebase v1 on top of that,

… on the hunch that exposing ada_tasks_pspace_data will encourage giving
it pure(r) methods and keeping references to global state confined to
its callers.

Not sure what I'm basing this hunch on though, so let me know if this
sounds like overkill, and I should just leave it at:

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index a9c45ab6b9c..e9a52937966 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -350,10 +350,10 @@ ada_get_task_number (thread_info *thread)
    arrays dimensioned with System.Multiprocessors.CPU.  */
 
 int
-ada_get_runtime_cpu_index (int target_cpu)
+ada_get_runtime_cpu_index (struct program_space *pspace, int target_cpu)
 {
   const struct ada_tasks_pspace_data *pspace_data
-    = get_ada_tasks_pspace_data (current_program_space);
+    = get_ada_tasks_pspace_data (pspace);
 
   gdb_assert (pspace_data->initialized_p);
 

Thanks for your patience.

[PATCH v2 0/3] Fix Ada tasking for baremetal targets using Ravenscar threads

[Kévin Le Gouguec]

Changes since v1:

* New patch to add a header for functions defined in ada-tasks.c.
* New patch to expose a getter for ada_tasks_pspace_data.
* Tweak original patch to use that getter from ravenscar-thread.c.

This ensures we do not introduce a function that refers to global
state; instead, we let ada_get_runtime_cpu_index's caller get its
hands "dirty".


I considered exposing ada_tasks_pspace_data entirely and making
ada_get_runtime_cpu_index a method as discussed, but AFAIU this would
have required either

  (a) keeping all members of that struct public so that existing
  functions can keep accessing them,

  (b) ponder which members ought to remain public, which should not,
  and which portions of ada-tasks.c should be turned into methods for
  ada_tasks_pspace_data.

(a) does not sound like progress in terms of encapsulation; I am not
confident I can get (b) done soon.  Hence the opaque pointer as a
first step toward (b).

Will stay on the lookout for opportunities to refactor uses of that
struct into "method-like" functions, make members private when it
makes sense, and finally, expose the struct definition, turning the
"method-like" functions into methods.


Tried to follow the coding conventions re. #include strategies;
apologies if I've missed something there, or anywhere else.


Kévin Le Gouguec (3):
  gdb: Introduce ada-tasks.h for functions defined in ada-tasks.c
  gdb/ada-tasks: Make the ada_tasks_pspace_data getter public
  gdb: Fix Ada tasking for baremetal targets using Ravenscar threads

 gdb/ada-lang.h             | 25 +++--------
 gdb/ada-tasks.c            | 44 +++++++++---------
 gdb/ada-tasks.h            | 91 ++++++++++++++++++++++++++++++++++++++
 gdb/breakpoint.c           |  1 +
 gdb/guile/scm-breakpoint.c |  2 +-
 gdb/mi/mi-main.c           |  2 +-
 gdb/python/py-breakpoint.c |  2 +-
 gdb/ravenscar-thread.c     | 17 +++++--
 8 files changed, 134 insertions(+), 50 deletions(-)
 create mode 100644 gdb/ada-tasks.h

-- 
2.25.1

[PATCH v2 1/3] gdb: Introduce ada-tasks.h for functions defined in ada-tasks.c

[Kévin Le Gouguec]

Also move the documentation for these functions to the new header, instead
of at the top of their definition, to align with our current practice.

I tried to honor our guideline that .c files should not rely on indirect
inclusion, hence I only removed #include "ada-lang.h" directives in .c files
that, as far as I could tell, only rely on symbols declared in ada-tasks.h.
---
 gdb/ada-lang.h             | 19 ----------
 gdb/ada-tasks.c            | 26 ++++----------
 gdb/ada-tasks.h            | 72 ++++++++++++++++++++++++++++++++++++++
 gdb/breakpoint.c           |  1 +
 gdb/guile/scm-breakpoint.c |  2 +-
 gdb/mi/mi-main.c           |  2 +-
 gdb/python/py-breakpoint.c |  2 +-
 gdb/ravenscar-thread.c     |  1 +
 8 files changed, 84 insertions(+), 41 deletions(-)
 create mode 100644 gdb/ada-tasks.h

diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 9fb7ac7f384..e0d44756218 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -366,25 +366,6 @@ struct ada_exc_info
 
 extern std::vector<ada_exc_info> ada_exceptions_list (const char *regexp);
 
-/* Tasking-related: ada-tasks.c */
-
-extern int valid_task_id (int);
-
-extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
-
-extern int ada_get_task_number (thread_info *thread);
-
-typedef gdb::function_view<void (struct ada_task_info *task)>
-  ada_task_list_iterator_ftype;
-extern void iterate_over_live_ada_tasks
-  (ada_task_list_iterator_ftype iterator);
-
-extern const char *ada_get_tcb_types_info (void);
-
-extern void print_ada_task_info (struct ui_out *uiout,
-				 const char *taskno_str,
-				 struct inferior *inf);
-
 /* Look for a symbol for an overloaded operator for the operation OP.
    PARSE_COMPLETION is true if currently parsing for completion.
    NARGS and ARGVEC describe the arguments to the call.  Returns a
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index a3a28063daa..bdabc0909cb 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -16,6 +16,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "ada-tasks.h"
 #include "observable.h"
 #include "gdbcmd.h"
 #include "target.h"
@@ -327,8 +328,7 @@ get_ada_tasks_inferior_data (struct inferior *inf)
   return data;
 }
 
-/* Return the task number of the task whose thread is THREAD, or zero
-   if the task could not be found.  */
+/* See ada-tasks.h.  */
 
 int
 ada_get_task_number (thread_info *thread)
@@ -364,7 +364,7 @@ get_task_number_from_id (CORE_ADDR task_id, struct inferior *inf)
   return 0;
 }
 
-/* Return non-zero if TASK_NUM is a valid task number.  */
+/* See ada-tasks.h.  */
 
 int
 valid_task_id (int task_num)
@@ -385,8 +385,7 @@ ada_task_is_alive (const struct ada_task_info *task_info)
   return (task_info->state != Terminated);
 }
 
-/* Search through the list of known tasks for the one whose ptid is
-   PTID, and return it.  Return NULL if the task was not found.  */
+/* See ada-tasks.h.  */
 
 struct ada_task_info *
 ada_get_task_info_from_ptid (ptid_t ptid)
@@ -405,8 +404,7 @@ ada_get_task_info_from_ptid (ptid_t ptid)
   return NULL;
 }
 
-/* Call the ITERATOR function once for each Ada task that hasn't been
-   terminated yet.  */
+/* See ada-tasks.h.  */
 
 void
 iterate_over_live_ada_tasks (ada_task_list_iterator_ftype iterator)
@@ -490,14 +488,7 @@ read_fat_string_value (char *dest, struct value *val, int max_len)
   dest[len] = '\0';
 }
 
-/* Get, from the debugging information, the type description of all types
-   related to the Ada Task Control Block that are needed in order to
-   read the list of known tasks in the Ada runtime.  If all of the info
-   needed to do so is found, then save that info in the module's per-
-   program-space data, and return NULL.  Otherwise, if any information
-   cannot be found, leave the per-program-space data untouched, and
-   return an error message explaining what was missing (that error
-   message does NOT need to be deallocated).  */
+/* See ada-tasks.h.  */
 
 const char *
 ada_get_tcb_types_info (void)
@@ -1056,10 +1047,7 @@ ada_build_task_list ()
   return data->task_list.size ();
 }
 
-/* Print a table providing a short description of all Ada tasks
-   running inside inferior INF.  If ARG_STR is set, it will be
-   interpreted as a task number, and the table will be limited to
-   that task only.  */
+/* See ada-tasks.h.  */
 
 void
 print_ada_task_info (struct ui_out *uiout,
diff --git a/gdb/ada-tasks.h b/gdb/ada-tasks.h
new file mode 100644
index 00000000000..5a948c27a44
--- /dev/null
+++ b/gdb/ada-tasks.h
@@ -0,0 +1,72 @@
+/* Helper routines for Ada tasking support in GBD.
+
+   Copyright (C) 2023 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef ADA_TASKS_H
+#define ADA_TASKS_H
+
+#include "gdbsupport/function-view.h"
+#include "ada-lang.h"
+#include "gdbthread.h"
+#include "inferior.h"
+#include "ui-out.h"
+
+/* Return non-zero if TASK_NUM is a valid task number.  */
+
+extern int valid_task_id (int task_num);
+
+/* Search through the list of known tasks for the one whose ptid is
+   PTID, and return it.  Return NULL if the task was not found.  */
+
+extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
+
+/* Return the task number of the task whose thread is THREAD, or zero
+   if the task could not be found.  */
+
+extern int ada_get_task_number (thread_info *thread);
+
+typedef gdb::function_view<void (struct ada_task_info *task)>
+  ada_task_list_iterator_ftype;
+
+/* Call the ITERATOR function once for each Ada task that hasn't been
+   terminated yet.  */
+
+extern void iterate_over_live_ada_tasks
+  (ada_task_list_iterator_ftype iterator);
+
+/* Get, from the debugging information, the type description of all types
+   related to the Ada Task Control Block that are needed in order to
+   read the list of known tasks in the Ada runtime.  If all of the info
+   needed to do so is found, then save that info in the module's per-
+   program-space data, and return NULL.  Otherwise, if any information
+   cannot be found, leave the per-program-space data untouched, and
+   return an error message explaining what was missing (that error
+   message does NOT need to be deallocated).  */
+
+extern const char *ada_get_tcb_types_info (void);
+
+/* Print a table providing a short description of all Ada tasks
+   running inside inferior INF.  If ARG_STR is set, it will be
+   interpreted as a task number, and the table will be limited to
+   that task only.  */
+
+extern void print_ada_task_info (struct ui_out *uiout,
+				 const char *taskno_str,
+				 struct inferior *inf);
+
+#endif /* ADA_TASKS_H */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 00cc2ab401c..a21f8d5f8ae 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -52,6 +52,7 @@
 #include "observable.h"
 #include "memattr.h"
 #include "ada-lang.h"
+#include "ada-tasks.h"
 #include "top.h"
 #include "valprint.h"
 #include "jit.h"
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index a7e043d847b..6b7b91cd322 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -27,7 +27,7 @@
 #include "gdbthread.h"
 #include "observable.h"
 #include "cli/cli-script.h"
-#include "ada-lang.h"
+#include "ada-tasks.h"
 #include "arch-utils.h"
 #include "language.h"
 #include "guile-internal.h"
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index e0cade2edc3..73f917f6071 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -46,7 +46,7 @@
 #include "osdata.h"
 #include "gdbsupport/gdb_splay_tree.h"
 #include "tracepoint.h"
-#include "ada-lang.h"
+#include "ada-tasks.h"
 #include "linespec.h"
 #include "extension.h"
 #include "gdbcmd.h"
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 1b10ccd5415..5189b3af0ad 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -27,7 +27,7 @@
 #include "gdbthread.h"
 #include "observable.h"
 #include "cli/cli-script.h"
-#include "ada-lang.h"
+#include "ada-tasks.h"
 #include "arch-utils.h"
 #include "language.h"
 #include "location.h"
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 22fbdbe9662..57ac9634233 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -21,6 +21,7 @@
 #include "gdbcore.h"
 #include "gdbthread.h"
 #include "ada-lang.h"
+#include "ada-tasks.h"
 #include "target.h"
 #include "inferior.h"
 #include "command.h"
-- 
2.25.1

[PATCH v2 2/3] gdb/ada-tasks: Make the ada_tasks_pspace_data getter public

[Kévin Le Gouguec]

A future commit will make ravenscar-thread.c call a new function that
operates on this object.
---
 gdb/ada-tasks.c |  7 ++-----
 gdb/ada-tasks.h | 13 +++++++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index bdabc0909cb..a1afd25f9a5 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -287,12 +287,9 @@ task_to_str (int taskno, const ada_task_info *task_info)
     return string_printf ("%d \"%s\"", taskno, task_info->name);
 }
 
-/* Return the ada-tasks module's data for the given program space (PSPACE).
-   If none is found, add a zero'ed one now.
-
-   This function always returns a valid object.  */
+/* See ada-tasks.h.  */
 
-static struct ada_tasks_pspace_data *
+struct ada_tasks_pspace_data *
 get_ada_tasks_pspace_data (struct program_space *pspace)
 {
   struct ada_tasks_pspace_data *data;
diff --git a/gdb/ada-tasks.h b/gdb/ada-tasks.h
index 5a948c27a44..aa85c853a47 100644
--- a/gdb/ada-tasks.h
+++ b/gdb/ada-tasks.h
@@ -24,6 +24,7 @@
 #include "ada-lang.h"
 #include "gdbthread.h"
 #include "inferior.h"
+#include "progspace.h"
 #include "ui-out.h"
 
 /* Return non-zero if TASK_NUM is a valid task number.  */
@@ -69,4 +70,16 @@ extern void print_ada_task_info (struct ui_out *uiout,
 				 const char *taskno_str,
 				 struct inferior *inf);
 
+/* An opaque type for this module's per-program-space data.  */
+
+struct ada_tasks_pspace_data;
+
+/* Return the ada-tasks module's data for the given program space (PSPACE).
+   If none is found, add a zero'ed one now.
+
+   This function always returns a valid object.  */
+
+struct ada_tasks_pspace_data *
+get_ada_tasks_pspace_data (struct program_space *pspace);
+
 #endif /* ADA_TASKS_H */
-- 
2.25.1

[PATCH v2 3/3] gdb: Fix Ada tasking for baremetal targets using Ravenscar threads

[Kévin Le Gouguec]

For some boards, the Ada tasking runtime defines a __gnat_gdb_cpu_first_id
symbol to let GDB map the ATCB's "Base_CPU" indices (ranging from 1 to
System.Multiprocessors.CPU) onto the CPU numbers that we read from the
remote stub and record in our ptids.

In other words, this symbol lets GDB translate "Ada CPUs" into "target
CPUs".  For example, for the Microchip PolarFire board, the runtime defines
this offset as:

  package System.BB.Board_Parameters is
     -- [...]
     GDB_First_CPU_Id : constant Interfaces.Unsigned_32 := 1;
     pragma Export (C, GDB_First_CPU_Id, "__gnat_gdb_cpu_first_id");

This is because on this board, CPU#1 is the "monitor" CPU; CPUs #2-5 are the
"application" CPUs that run user code.  So while the ATCB shows Base_CPUs
equal to 1, QEMU reports that our code is running on CPU #2.

We convert the ATCB's Base_CPUs into "target" CPUs before recording them in
struct ada_task_info.base_cpu; this is what we need in most of
ravenscar-thread.c, except in one specific spot: when reading the active
thread from __gnat_running_thread_table, which is defined as:

  package System.BB.Threads.Queues is
     -- [...]
     Running_Thread_Table : array (System.Multiprocessors.CPU) of Thread_Id

On baremetal targets, System.Multiprocessors.CPU is defined as:

  package System.Multiprocessors is
     -- [...]
     type CPU_Range is range 0 .. System.BB.Parameters.Max_Number_Of_CPUs;

     subtype CPU is CPU_Range range 1 .. CPU_Range'Last;

     Not_A_Specific_CPU : constant CPU_Range := 0;

Thus __gnat_running_thread_table has Max_Number_Of_CPUs elements; for the
Microchip PolarFire board, the runtime define this as:

  package System.BB.Parameters is
     -- [...]
     Max_Number_Of_CPUs : constant := 1;

So the table has just one element, but ravenscar-thread.c attempts to index
it using the "target" CPU ID, i.e. 2.

This remained undetected because as luck would have it, with the specific
compiler we were using, *(__gnat_running_thread_table+8) happened to contain
exactly the same content as *__gnat_running_thread_table.  After bumping the
compiler, the layout of the tasking runtime changed, and so did the content
of *(__gnat_running_thread_table+8).

This commit introduces a new function to let GDB convert a "target" CPU back
to an "Ada" CPU.

Tested on x86_64-linux and riscv64-elf with AdaCore's internal testsuite,
which has more extensive coverage of Ada tasking and "Ravenscar thread"
features.
---
 gdb/ada-lang.h         |  6 +++++-
 gdb/ada-tasks.c        | 11 +++++++++++
 gdb/ada-tasks.h        |  6 ++++++
 gdb/ravenscar-thread.c | 16 +++++++++++++---
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index e0d44756218..e634a8d1d40 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -145,7 +145,11 @@ struct ada_task_info
   /* The CPU on which the task is running.  This is dependent on
      the runtime actually providing that info, which is not always
      the case.  Normally, we should be able to count on it on
-     bare-metal targets.  */
+     bare-metal targets.
+
+     NB: This CPU number has been normalized to match the IDs reported by the
+     target, as recorded in the LWP field of PTIDs.  It may not map directly to
+     the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index.  */
   int base_cpu;
 };
 
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index a1afd25f9a5..7d98a16228d 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -301,6 +301,17 @@ get_ada_tasks_pspace_data (struct program_space *pspace)
   return data;
 }
 
+/* See ada-tasks.h.  */
+
+int
+ada_get_runtime_cpu_index (struct ada_tasks_pspace_data *pspace_data,
+			   int target_cpu)
+{
+  gdb_assert (pspace_data->initialized_p);
+
+  return target_cpu - pspace_data->cpu_id_offset;
+}
+
 /* Return the ada-tasks module's data for the given inferior (INF).
    If none is found, add a zero'ed one now.
 
diff --git a/gdb/ada-tasks.h b/gdb/ada-tasks.h
index aa85c853a47..325f99ca0f3 100644
--- a/gdb/ada-tasks.h
+++ b/gdb/ada-tasks.h
@@ -82,4 +82,10 @@ struct ada_tasks_pspace_data;
 struct ada_tasks_pspace_data *
 get_ada_tasks_pspace_data (struct program_space *pspace);
 
+/* Translate a "target" CPU index into a "runtime" index suitable for addressing
+   arrays dimensioned with System.Multiprocessors.CPU.  */
+
+extern int ada_get_runtime_cpu_index (struct ada_tasks_pspace_data *pspace_data,
+				      int target_cpu);
+
 #endif /* ADA_TASKS_H */
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 57ac9634233..8289b9999bc 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -31,6 +31,7 @@
 #include "top.h"
 #include "regcache.h"
 #include "objfiles.h"
+#include "progspace.h"
 #include <unordered_map>
 
 /* This module provides support for "Ravenscar" tasks (Ada) when
@@ -194,7 +195,11 @@ struct ravenscar_thread_target final : public target_ops
   /* This maps a TID to the CPU on which it was running.  This is
      needed because sometimes the runtime will report an active task
      that hasn't yet been put on the list of tasks that is read by
-     ada-tasks.c.  */
+     ada-tasks.c.
+
+     NB: These CPU numbers correspond to those reported by the target,
+     which may differ from the numbers recorded in the ATCB.  See
+     ada_get_runtime_cpu_index.  */
   std::unordered_map<ULONGEST, int> m_cpu_map;
 };
 
@@ -376,8 +381,13 @@ ravenscar_thread_target::runtime_initialized ()
    Return 0 if the ID could not be determined.  */
 
 static CORE_ADDR
-get_running_thread_id (int cpu)
+get_running_thread_id (int target_cpu)
 {
+  struct ada_tasks_pspace_data *pspace_data
+    = get_ada_tasks_pspace_data (current_program_space);
+
+  int runtime_cpu = ada_get_runtime_cpu_index (pspace_data, target_cpu);
+
   struct bound_minimal_symbol object_msym = get_running_thread_msymbol ();
   int object_size;
   int buf_size;
@@ -391,7 +401,7 @@ get_running_thread_id (int cpu)
 
   object_size = builtin_type_void_data_ptr->length ();
   object_addr = (object_msym.value_address ()
-		 + (cpu - 1) * object_size);
+		 + (runtime_cpu - 1) * object_size);
   buf_size = object_size;
   buf = (gdb_byte *) alloca (buf_size);
   read_memory (object_addr, buf, buf_size);
-- 
2.25.1