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

[Simon Marchi <simark@simark.ca>]

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