From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 487663858C66 for ; Thu, 12 Jan 2023 18:19:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 487663858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 72E4E1E110; Thu, 12 Jan 2023 13:19:04 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1673547544; bh=7D4HbDX8nuKGHowpSqNvz5HRDPjEd4lzHEiyhFosHnQ=; h=Date:Subject:To:References:From:In-Reply-To:From; b=PFIp7i9KLW62o8Gfw3i3y1qtSpgGFOYHSTglblrqTQxx+z3k3yvuVWMItu8LpE18P T57uzM5COnTmiJPU1aNsZznFVCA6s6zY5bWxKxjWNW4SxZyRG5RE+E+W6se2LY2i3S ye5QUBEgTA76ZG2UYxBssQy8psmJ3npzrQqrECJw= Message-ID: <1a05a197-c38c-891b-108a-c8c7174562a9@simark.ca> Date: Thu, 12 Jan 2023 13:19:04 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads Content-Language: en-US To: =?UTF-8?Q?K=c3=a9vin_Le_Gouguec?= , gdb-patches@sourceware.org References: <20230112171043.1159787-1-legouguec@adacore.com> From: Simon Marchi In-Reply-To: <20230112171043.1159787-1-legouguec@adacore.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > 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