From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id llXAJeZuBGUOtBYAWB0awg (envelope-from ) for ; Fri, 15 Sep 2023 10:49:10 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=Oe/KU0k7; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 854E01E0C3; Fri, 15 Sep 2023 10:49:10 -0400 (EDT) Received: from server2.sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 7240B1E028 for ; Fri, 15 Sep 2023 10:49:08 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B7DB53857C71 for ; Fri, 15 Sep 2023 14:49:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B7DB53857C71 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1694789347; bh=iyiHzgwYd9jBXt16l4aTAgsiGlqHboQhxDLEIUh7E5k=; h=Date:Subject:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Oe/KU0k7wsj4xaH0VrPperOAGs2aeyFHgBp10pEg+tVikA/CkAZBgt6P9fJPH86nO qa6OnVxtxpEaqASc3Cwb1pGlj+q/TrdiBsHtsnkpMAdapEw9h8qboK1jlDuKxIx4TY Q6fJwe3mktIt5itt2Zx57he2JdO61w/ZhLDCtCOc= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 7FB3C3858032 for ; Fri, 15 Sep 2023 14:48:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7FB3C3858032 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 38FElcGs020811 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 15 Sep 2023 10:47:43 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 38FElcGs020811 Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 708221E028; Fri, 15 Sep 2023 10:47:38 -0400 (EDT) Message-ID: <661f6bd6-02aa-4e53-b82d-2cecdb46a434@polymtl.ca> Date: Fri, 15 Sep 2023 10:47:38 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 3/3] gdb/amdgpu: add precise-memory support Content-Language: fr To: Lancelot SIX , Simon Marchi Cc: gdb-patches@sourceware.org References: <20230914161433.432600-1-simon.marchi@efficios.com> <20230914161433.432600-3-simon.marchi@efficios.com> <20230915101353.h6z7fv4e5ekn3jn5@octopus> In-Reply-To: <20230915101353.h6z7fv4e5ekn3jn5@octopus> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 15 Sep 2023 14:47:38 +0000 X-Spam-Status: No, score=-3037.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 9/15/23 06:13, Lancelot SIX via Gdb-patches wrote: > Hi Simon, > >> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c >> index 22c269b7992c..d38b790f53f7 100644 >> --- a/gdb/amd-dbgapi-target.c >> +++ b/gdb/amd-dbgapi-target.c >> @@ -1326,6 +1338,29 @@ amd_dbgapi_target::stopped_by_hw_breakpoint () >> return false; >> } >> >> +/* Try to set the process's memory access reporting precision mode. >> + >> + Warn if the requested mode is not supported on at least one agent in the >> + process. >> + >> + Error out if setting the requested mode failed for some other reason. */ >> + >> +static void >> +try_set_process_memory_precision (amd_dbgapi_inferior_info &info) >> +{ >> + auto mode = (info.precise_memory.requested >> + ? AMD_DBGAPI_MEMORY_PRECISION_PRECISE >> + : AMD_DBGAPI_MEMORY_PRECISION_NONE); >> + amd_dbgapi_status_t status >> + = amd_dbgapi_set_memory_precision (info.process_id, mode); >> + >> + if (status == AMD_DBGAPI_STATUS_ERROR_NOT_SUPPORTED) >> + warning (_("AMDGPU precise memory access reporting could not be enabled.")); >> + else if (status != AMD_DBGAPI_STATUS_SUCCESS) >> + error (_("amd_dbgapi_set_memory_precision failed (%s)"), >> + get_status_string (status)); > > I think you forgot something like > > else > info.precise_memory.enabled = info.precise_memory.requested; Huh that's true, that got lost in my refactoring. How come the test didn't catch it? Oh, oh my machine I have multiple AMD GPUs, not all of which support this memory precision feature. So it would just stay disabled. I spoke to Lancelot offline, there is apparently no way right now to tell the whole stack to just consider a single card. > > here. > >> +} >> + >> /* Make the amd-dbgapi library attach to the process behind INF. >> >> Note that this is unrelated to the "attach" GDB concept / command. >> @@ -1399,6 +1434,8 @@ attach_amd_dbgapi (inferior *inf) >> amd_dbgapi_debug_printf ("process_id = %" PRIu64 ", notifier fd = %d", >> info->process_id.handle, info->notifier); >> >> + try_set_process_memory_precision (*info); >> + >> /* If GDB is attaching to a process that has the runtime loaded, there will >> already be a "runtime loaded" event available. Consume it and push the >> target. */ >> @@ -1443,8 +1480,10 @@ detach_amd_dbgapi (inferior *inf) >> for (auto &&value : info->breakpoint_map) >> delete_breakpoint (value.second); >> >> - /* Reset the amd_dbgapi_inferior_info. */ >> + /* Reset the amd_dbgapi_inferior_info, except for precise_memory_mode. */ >> + bool precise_memory_requested = info->precise_memory.requested; >> *info = amd_dbgapi_inferior_info (inf); > > Not really significant, but I am wonedring if adding a > request_precise_memory bool parameter to the amd_dbgapi_inferior_info > ctor (maybe with default value to false) would make sense. > > That being said, I'm happy either way. Yeah I can do that, it allows this to be a one-liner: *info = amd_dbgapi_inferior_info (inf, info->precise_memory.requested); > >> + info->precise_memory.requested = precise_memory_requested; >> >> maybe_reset_amd_dbgapi (); >> } > > I have tested this patch (with the fix suggested above) on gfx900 (where > precise memory is not supported) and gfx90a (where precise memory is > supported). > > With the correction, the patch looks good to me. Thanks, I will post a v3 just to be sure. Simon