From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id BAA8B3858D1E for ; Fri, 15 Sep 2023 10:13:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BAA8B3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from octopus (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 81A2480AE9; Fri, 15 Sep 2023 10:13:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1694772838; bh=4Z4USMhoMl2fZ6erdixSkb/nl2i5Kwg6m+Kp74Qco3g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rUZpPfy1GQKHuPZvhqQncWfjeJWtZXKp2so1yan818dIxCpM5cLcTXbJ77fwsUZTv fjTiPMuzwzZ7/Ih0H6qwVp1PEdLSFAQA4wFg3cqW27f/cCmi7SNU5ZWPAegpb+JfOw 8Dh7XfGxh1oRjqmzcjwFng+2/alVtO8vEnXHahHIWDrCxCi8PVWZJPWn8vgSCF3d+r zwre5TTQxE7ot2aOVmzCWopFACJe4zWPNB2vmokgu3psDxAFGKui1H178nDu3fKLya vm2TK9oFUIIvaFfn4HGhp9/T/O/1TBto9m5/3q9EphluHocONgoTgQt+bjRyGs0mZl f1f3NYXD3NcKg== Date: Fri, 15 Sep 2023 11:13:53 +0100 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 3/3] gdb/amdgpu: add precise-memory support Message-ID: <20230915101353.h6z7fv4e5ekn3jn5@octopus> References: <20230914161433.432600-1-simon.marchi@efficios.com> <20230914161433.432600-3-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230914161433.432600-3-simon.marchi@efficios.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Fri, 15 Sep 2023 10:13:58 +0000 (UTC) X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: 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; 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. > + 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. Best, Lancelot.