From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca 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 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1694789264; bh=iyiHzgwYd9jBXt16l4aTAgsiGlqHboQhxDLEIUh7E5k=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=i8c/gPtV0PRGjb8Ov7j8pB/dM4km3JXCM6no9Irf/Hd9XiFGffFveCLkxLbwTr3Kg vA6j/iBPexrICN86N3aFMYV/ebTx1jAK5o7Glkr9BCE7M1iE2BSgV7U/Jplimg8VSm kjBvusrjuvxQl+JC6qvpcqBhQuDzdVwDttzonQmE= 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> From: Simon Marchi 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 List-Id: 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