From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) by sourceware.org (Postfix) with ESMTPS id BAAFE3858D32 for ; Fri, 13 Jan 2023 12:32:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BAAFE3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com Received: by mail-wm1-x32b.google.com with SMTP id m8-20020a05600c3b0800b003d96f801c48so18667902wms.0 for ; Fri, 13 Jan 2023 04:32:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=SXlKhFyPk9uLyr2mnS8kPw0atgIz1kn/7zrfgTkXdgM=; b=CGrxUJrA0CRs9veaywN8jQQZX5c1M0X27aO9eqqBPnqNnByb3jJy9lRI+0NHCBD0jN 9JlBqcxdezhZ0DYsVsQ3h0DqrsZzPjHyehklKRd7pmjjPu5/1Trugq1UO6mSJWG/eOwT 6EWpPBlu230UWcyUWpIo2psrihJR7QqKOJFrFUrPzLy3MwhV1EWY1CB527v5uCzWiEfd MK98BlmlnXo7w/fVCtgUZ1wyjafqXHAQrLZVGuGQoitl+9qzQbDJfgIa+jc9hhAhOufc Z9Gpl+l3jdipkgi8rdIZntI2AvtVIUwKhy4OwiQNOT1IkmSfyWxWTRlDVusAT1Tp60Z+ LLCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=SXlKhFyPk9uLyr2mnS8kPw0atgIz1kn/7zrfgTkXdgM=; b=uxBYk6zabxFM9DzmUQ606cDnG62vDmTtM4FJMR31rdsQhb5r7UBA1W/pPOmyNzDTCN UoK3Ud/ZMa0c5MYnaAVfQHoSJjl83ochvY5Yub5Gtgnc+taePP1nix5mZYkP24vVnpqQ Q7raYG0KSYMGK2URbA0cZ1KH4gnMnmcOWWCXX/JIkr6ZDhYUPCVZkDzvvHNziwCYXrtT 7WMNqZtHGY7Z5gLT5jlF7cKtBy0TXIHtmTWBTpdvMaqRNuJwYX/luAzwFbzG0iIBHHcs xt/gIoi00l2NYDy4INzu22253xLdhKEu962UEhojKFuyLk6sRsz3LKvLkgTPOOXK3ZvX CchQ== X-Gm-Message-State: AFqh2krN81Wx/mhwiR5XsCHvkEZHywUwHV/amG2H/Gxo2mo5TTOsI/MD qdql86OpfGpk2kkm+PV4iBzz9LVpI6u378pn X-Google-Smtp-Source: AMrXdXvG64Axa+PsIs2t2k9GnLwZTOC+hJ8aq9ym98P3Dybg97+1bJaExdtDGbazFSVg9GEXG9QoTA== X-Received: by 2002:a05:600c:1c90:b0:3cf:75a8:ecf4 with SMTP id k16-20020a05600c1c9000b003cf75a8ecf4mr58351972wms.24.1673613143135; Fri, 13 Jan 2023 04:32:23 -0800 (PST) Received: from legouguec-Precision-7550 ([2a01:e0a:253:fe0:df05:e325:4d96:7f79]) by smtp.gmail.com with ESMTPSA id m5-20020a05600c3b0500b003c6b7f5567csm2776645wms.0.2023.01.13.04.32.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 04:32:22 -0800 (PST) From: =?utf-8?Q?K=C3=A9vin_Le_Gouguec?= To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix Ada tasking for baremetal targets using Ravenscar threads In-Reply-To: <1a05a197-c38c-891b-108a-c8c7174562a9@simark.ca> (Simon Marchi's message of "Thu, 12 Jan 2023 13:19:04 -0500") References: <20230112171043.1159787-1-legouguec@adacore.com> <1a05a197-c38c-891b-108a-c8c7174562a9@simark.ca> Date: Fri, 13 Jan 2023 13:32:21 +0100 Message-ID: <877cxqipii.fsf@adacore.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: Hey Simon, thanks for the review! Simon Marchi 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 dir= ectly to >> + the Base_CPU recorded in the ATCB; see ada_get_runtime_cpu_index. = */ >> int base_cpu; >> }; >>=20=20 >> @@ -374,6 +378,8 @@ extern struct ada_task_info *ada_get_task_info_from_= ptid (ptid_t ptid); >>=20=20 >> extern int ada_get_task_number (thread_info *thread); >>=20=20 >> +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_E= very_Subprogram Will give this a re-read before submitting v2. >> + >> 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. */ >> } >>=20=20 >> +/* Translate a "target" CPU index into a "runtime" index suitable for a= ddressing >> + 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 >> + =3D 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 > =3D get_ada_tasks_pspace_data (current_program_space); > cpu =3D 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=E2=80=A6 (d) =E2=80=A6 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!