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

[Simon Marchi <simark@simark.ca>]

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