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

["Kévin Le Gouguec" <legouguec@adacore.com>]

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.