From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id E4106383FB93 for ; Fri, 13 Jan 2023 17:16:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E4106383FB93 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-wr1-x42c.google.com with SMTP id r2so21684881wrv.7 for ; Fri, 13 Jan 2023 09:16:08 -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=KBadclp4QsDzJmsUBj72D/RIcM8bO8OuFILF2fs6BTM=; b=kRqrgLCuDxy9x0YJkzgn2t4lDJyRPc7a03RLZxOVJL2lqgKIRuLju2TiZNcHqaO3Qp sJZHCEGs14vtSXL49V4nD7a2rJ01iqYLgEyp227wDUF9La9zouygNiGD9aWWkexQfF4Q LPX46AoQJlFiO8lN2YsBo2spmAPSu7PD/t2sOXXdejL2RR8AX6IZXKHAt/fYmJLIjeuU AE4sIvTh2Cj/oI4wtLxr39taf0L1BHCdveOnuwePhxelI7DIeK0ETb+v3H9L3UZ7vO1A hWegL0VuT3vxg6HY20u2vw2eo6v1a7QmzwnjQHozX5Q9tzPGhMVVSpCIIryDwpJp+Hdy EunQ== 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=KBadclp4QsDzJmsUBj72D/RIcM8bO8OuFILF2fs6BTM=; b=JcObOMT1tJtet3c9QRk8VslcoNO+x5iG4srqI9LoRl/W6o56vo6E5Vf2uwCD8STHOR EY1MdjXyTnANJOLrX7wfK/O6qkBfZEBD022GRGP3+r0/PDGZhIHbWocJ6hya2UqptZhy tT69+XM5shzJ5vX5BaNYnry5cbDa5IwUa6bmf9PG6MGrgSmHx0j/ysoh9Q0gwZRgtl4h GJs4q4cYSonFHMyb7UaQkDiwFb74F9rOxNMwjS5wRiyGfFW72lQSSnE4UchLpEpxNioY mGWAh47uVPAfXNbocYL9xsI56UmAz8+hJEI+Xf7Re+E8IFN6ttcnaTTEjVzCKlzv67I2 XTDw== X-Gm-Message-State: AFqh2ko3PycL0J4+0EdtHtdGpKqgoAu3Jl9RgLT8K83SzRDG/Xe8zY1h 8g/xbttAWSgadI5pUOseGtio8jcwBS8/EOTG X-Google-Smtp-Source: AMrXdXu7ZnQ1izoY9vZFxMTWxhhJrN4/5kvRHO90pRcaDLQtc5toFgyYHDpEI1KJOvuuPnLSvjqg8Q== X-Received: by 2002:a05:6000:1144:b0:2bd:da99:bb8b with SMTP id d4-20020a056000114400b002bdda99bb8bmr3693383wrx.5.1673630166807; Fri, 13 Jan 2023 09:16:06 -0800 (PST) Received: from legouguec-Precision-7550 ([2a01:e0a:253:fe0:df05:e325:4d96:7f79]) by smtp.gmail.com with ESMTPSA id i2-20020adfdec2000000b002b9b9445149sm23056630wrn.54.2023.01.13.09.16.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 09:16:05 -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: <5d36b0ba-bb72-3dac-d5f7-545c78490ab7@simark.ca> (Simon Marchi's message of "Fri, 13 Jan 2023 11:29:17 -0500") References: <20230112171043.1159787-1-legouguec@adacore.com> <1a05a197-c38c-891b-108a-c8c7174562a9@simark.ca> <877cxqipii.fsf@adacore.com> <5d36b0ba-bb72-3dac-d5f7-545c78490ab7@simark.ca> Date: Fri, 13 Jan 2023 18:16:04 +0100 Message-ID: <87tu0ugxt7.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.9 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: Simon Marchi 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) =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). > > 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=E2=80=A6 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, =E2=80=A6 on the hunch that exposing ada_tasks_pspace_data will encourage g= iving 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. */ =20 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 - =3D get_ada_tasks_pspace_data (current_program_space); + =3D get_ada_tasks_pspace_data (pspace); =20 gdb_assert (pspace_data->initialized_p); =20 Thanks for your patience.