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

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

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!