From patchwork Thu Dec 14 20:22:35 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 82171 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9E494385DC2B for ; Thu, 14 Dec 2023 20:23:30 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by sourceware.org (Postfix) with ESMTPS id EC831386180F for ; Thu, 14 Dec 2023 20:22:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EC831386180F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EC831386180F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.49 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702585379; cv=none; b=SQkGE9+U+k39IuC5GmMubWLwJh0OYlxw8bBJSqpy+/FRxaFB98UqaR/JY6rBVdu2MK9FKMmm7yvm2AvLugWGeYz0RGp6gMwuKuw3Z8exo3F8UgGgPs2pYtRDItZluoo0PdqjT8jRApctO9h1dslNwkDzfRMeypall3XORyFbou4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702585379; c=relaxed/simple; bh=XZdEJe6u3iOT2+x14U7jDwCsHO5hJ3NqtfA2yuGnQMc=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=oA0T4WONP2pZzyLyRO2htzOyHAxq4ljYYrOMjoCaFdZYBHQIAsGbQitaoHqLHh3VJV8xBixFzoIdzCYgVl6LSNjWZm4gZRKyRaI3DBiezI5rdLqFB2gSbLSaAOqHVLYfvPBKQ2kRYkkAtQoxCVfiIllppEDsfU4H+FecKMWSBTY= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-40c19f5f822so6483855e9.1 for ; Thu, 14 Dec 2023 12:22:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702585375; x=1703190175; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZVxys935Ibo8wbBWOO6bd+d2WHWLEtbLyOhr4/ddy4k=; b=UTP1GaOhLMKOEOt2Z70ojcoNAOMPGkwUk4g+R5BYAcRj+0BBr1S/zpYXtf/DIhQoF8 UWKDSwjqs25OaCUb2zR1jO01H0+DJ+6IMWZ7VuU3Q51hYKiQFX6iayyhzXlOeqnkjxUk ow7btYir1DlMMq4Y5Jie8giLUkXGQxG7Szi5hzwjIu3ZeoLZ99pl3uzOEiUE3QbBfwtb j/CGnxVAGk5rzSDBniwV21wr2MySocO552UUKbAmECyEwcO5dbjSxf4YmHyU25KWMX+v 6SvquY8Jwyps03Rqnxsut2TB4+Y9cCBRitrdvu7FNWHbfZfY0C51xYTstcwR+Ahsmloz X/mQ== X-Gm-Message-State: AOJu0YxONMO8yDTAroLxFdTynGbIkt2uCta589QrKLm6o3w8xwvAYCL5 83eTRG9Ch3jwuUvqzj+b9PS2tmmC3q3N1g== X-Google-Smtp-Source: AGHT+IHPiQCsQmV15yP5Li5ZBUU+m1wpMfKlAz+rUO6EunCv6HCv5adOpn3Znn+i46vPikbYtFnl5A== X-Received: by 2002:a05:600c:4d0d:b0:40c:2c48:bdc0 with SMTP id u13-20020a05600c4d0d00b0040c2c48bdc0mr5420043wmp.136.1702585375128; Thu, 14 Dec 2023 12:22:55 -0800 (PST) Received: from localhost ([2001:8a0:f923:4f00:2646:535c:5a04:e380]) by smtp.gmail.com with UTF8SMTPSA id fm21-20020a05600c0c1500b0040c03c3289bsm26192616wmb.37.2023.12.14.12.22.54 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Dec 2023 12:22:54 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 5/8] Fix thread target ID of exited waves Date: Thu, 14 Dec 2023 20:22:35 +0000 Message-ID: <20231214202238.1065676-6-pedro@palves.net> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231214202238.1065676-1-pedro@palves.net> References: <20231214202238.1065676-1-pedro@palves.net> MIME-Version: 1.0 X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Currently, if you step over kernel exit, you see: stepi [AMDGPU Wave ?:?:?:1 (?,?,?)/? exited] Command aborted, thread exited. (gdb) Those '?' are because the thread/wave is already gone by the time GDB prints the "exited" notification, we can't ask dbgapi for any info about the wave anymore. This commit fixes it by caching the wave's coordinates as soon as GDB sees the wave for the first time, and making amd_dbgapi_target::pid_to_str use the cached info. At first I thought of clearing the wave_info object from a thread_exited observer. However, that is too soon, resulting in this: (gdb) si [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited] Command aborted, thread exited. (gdb) thread [Current thread is 6 (AMDGPU Wave ?:?:?:0 (?,?,?)/?) (exited)] We need instead to clear the wave info when the thread is ultimately deleted, so we get: (gdb) si [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited] Command aborted, thread exited. (gdb) thread [Current thread is 6 (AMDGPU Wave 1:4:1:1 (0,0,0)/0) (exited)] And for that, we need a new thread_deleted observable. Change-Id: I6c3e22541f051e1205f75eb657b04dc15e547580 --- gdb/amd-dbgapi-target.c | 168 +++++++++++++++++++++++++++++++--------- gdb/observable.c | 1 + gdb/observable.h | 5 ++ gdb/thread.c | 2 + 4 files changed, 138 insertions(+), 38 deletions(-) diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c index 18c0543c40e..86102b7fb03 100644 --- a/gdb/amd-dbgapi-target.c +++ b/gdb/amd-dbgapi-target.c @@ -109,6 +109,28 @@ get_amd_dbgapi_target_inferior_created_observer_token () return amd_dbgapi_target_inferior_created_observer_token; } +/* A type holding coordinate, etc. info for a given wave. We cache + this because we need this information after a wave exits. */ + +struct wave_info +{ + /* The wave. Set by the ctor. */ + amd_dbgapi_wave_id_t wave_id; + + /* All these fields are initialized here to a value that is printed + as "?". */ + amd_dbgapi_dispatch_id_t dispatch_id {}; + amd_dbgapi_queue_id_t queue_id {}; + amd_dbgapi_agent_id_t agent_id {}; + uint32_t group_ids[3] {UINT32_MAX, UINT32_MAX, UINT32_MAX}; + uint32_t wave_in_group = UINT32_MAX; + + explicit wave_info (amd_dbgapi_wave_id_t wave_id); + + /* Return the target ID string for the wave this wave_info is + for. */ + std::string to_string () const; +}; /* Big enough to hold the size of the largest register in bytes. */ #define AMDGPU_MAX_REGISTER_SIZE 256 @@ -160,6 +182,16 @@ struct amd_dbgapi_inferior_info /* List of pending events the amd-dbgapi target retrieved from the dbgapi. */ std::list> wave_events; + + /* Map of wave ID to wave_info. We cache wave_info objects because + we need to access the info after the wave is gone, in the thread + exit nofication. E.g.: + [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited] + + wave_info objects are added when we first see the wave, and + removed from a thread_deleted observer. */ + std::unordered_map + wave_info_map; }; static amd_dbgapi_event_id_t process_event_queue @@ -256,56 +288,70 @@ static const registry::key static async_event_handler *amd_dbgapi_async_event_handler = nullptr; -/* Return the target id string for a given wave. */ - -static std::string -wave_target_id_string (amd_dbgapi_wave_id_t wave_id) +std::string +wave_info::to_string () const { - amd_dbgapi_dispatch_id_t dispatch_id; - amd_dbgapi_queue_id_t queue_id; - amd_dbgapi_agent_id_t agent_id; - uint32_t group_ids[3], wave_in_group; std::string str = "AMDGPU Wave"; - amd_dbgapi_status_t status - = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_AGENT, - sizeof (agent_id), &agent_id); - str += (status == AMD_DBGAPI_STATUS_SUCCESS + str += (agent_id.handle != 0 ? string_printf (" %ld", agent_id.handle) : " ?"); - status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_QUEUE, - sizeof (queue_id), &queue_id); - str += (status == AMD_DBGAPI_STATUS_SUCCESS + str += (queue_id.handle != 0 ? string_printf (":%ld", queue_id.handle) : ":?"); - status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_DISPATCH, - sizeof (dispatch_id), &dispatch_id); - str += (status == AMD_DBGAPI_STATUS_SUCCESS + str += (dispatch_id.handle != 0 ? string_printf (":%ld", dispatch_id.handle) : ":?"); str += string_printf (":%ld", wave_id.handle); - status = amd_dbgapi_wave_get_info (wave_id, - AMD_DBGAPI_WAVE_INFO_WORKGROUP_COORD, - sizeof (group_ids), &group_ids); - str += (status == AMD_DBGAPI_STATUS_SUCCESS + str += (group_ids[0] != UINT32_MAX ? string_printf (" (%d,%d,%d)", group_ids[0], group_ids[1], group_ids[2]) : " (?,?,?)"); - status = amd_dbgapi_wave_get_info - (wave_id, AMD_DBGAPI_WAVE_INFO_WAVE_NUMBER_IN_WORKGROUP, - sizeof (wave_in_group), &wave_in_group); - str += (status == AMD_DBGAPI_STATUS_SUCCESS + str += (wave_in_group != UINT32_MAX ? string_printf ("/%d", wave_in_group) : "/?"); return str; } +wave_info::wave_info (amd_dbgapi_wave_id_t wave_id) + : wave_id (wave_id) +{ +} + +/* Read in wave_info for WAVE_ID. */ + +static wave_info +get_wave_info (amd_dbgapi_wave_id_t wave_id) +{ + wave_info res (wave_id); + + /* Any field that fails to be read is left with its in-class + initialized value, which is printed as "?". */ + + amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_AGENT, + sizeof (res.agent_id), &res.agent_id); + amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_QUEUE, + sizeof (res.queue_id), &res.queue_id); + amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_DISPATCH, + sizeof (res.dispatch_id), &res.dispatch_id); + + amd_dbgapi_wave_get_info (wave_id, + AMD_DBGAPI_WAVE_INFO_WORKGROUP_COORD, + sizeof (res.group_ids), &res.group_ids); + + amd_dbgapi_wave_get_info (wave_id, + AMD_DBGAPI_WAVE_INFO_WAVE_NUMBER_IN_WORKGROUP, + sizeof (res.wave_in_group), &res.wave_in_group); + + return res; +} + /* Clear our async event handler. */ static void @@ -510,7 +556,21 @@ amd_dbgapi_target::pid_to_str (ptid_t ptid) if (!ptid_is_gpu (ptid)) return beneath ()->pid_to_str (ptid); - return wave_target_id_string (get_amd_dbgapi_wave_id (ptid)); + process_stratum_target *proc_target = current_inferior ()->process_target (); + inferior *inf = find_inferior_pid (proc_target, ptid.pid ()); + gdb_assert (inf != nullptr); + amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (inf); + + auto wave_id = get_amd_dbgapi_wave_id (ptid); + + auto it = info->wave_info_map.find (wave_id.handle); + if (it != info->wave_info_map.end ()) + return it->second.to_string (); + + /* A wave we don't know about. Shouldn't usually happen, but + asserting and bringing down the session is a bit too harsh. Just + print all unknown info as "?"s. */ + return wave_info (wave_id).to_string (); } const char * @@ -929,6 +989,46 @@ make_gpu_ptid (ptid_t::pid_type pid, amd_dbgapi_wave_id_t wave_id) return ptid_t (pid, 1, wave_id.handle); } +/* When a thread is deleted, remove its wave_info from the inferior's + wave_info map. */ + +static void +amd_dbgapi_thread_deleted (thread_info *tp) +{ + if (tp->inf->target_at (arch_stratum) == &the_amd_dbgapi_target + && ptid_is_gpu (tp->ptid)) + { + amd_dbgapi_inferior_info *info = amd_dbgapi_inferior_data.get (tp->inf); + auto wave_id = get_amd_dbgapi_wave_id (tp->ptid); + auto it = info->wave_info_map.find (wave_id.handle); + gdb_assert (it != info->wave_info_map.end ()); + info->wave_info_map.erase (it); + } +} + +/* Register WAVE_PTID as a new thread in INF's thread list, and record + its wave_info in the inferior's wave_info map. */ + +static thread_info * +add_gpu_thread (inferior *inf, ptid_t wave_ptid) +{ + process_stratum_target *proc_target = inf->process_target (); + amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (inf); + + auto wave_id = get_amd_dbgapi_wave_id (wave_ptid); + + if (!info->wave_info_map.try_emplace (wave_id.handle, + get_wave_info (wave_id)).second) + internal_error ("wave ID %ld already in map", wave_id.handle); + + /* Create new GPU threads silently to avoid spamming the terminal + with thousands of "[New Thread ...]" messages. */ + thread_info *thread = add_thread_silent (proc_target, wave_ptid); + set_running (proc_target, wave_ptid, true); + set_executing (proc_target, wave_ptid, true); + return thread; +} + /* Process an event that was just pulled out of the amd-dbgapi library. */ static void @@ -1015,13 +1115,7 @@ process_one_event (amd_dbgapi_event_id_t event_id, thread_info *thread = proc_target->find_thread (event_ptid); if (thread == nullptr) - { - /* Silently create new GPU threads to avoid spamming the - terminal with thousands of "[New Thread ...]" messages. */ - thread = add_thread_silent (proc_target, event_ptid); - set_running (proc_target, event_ptid, true); - set_executing (proc_target, event_ptid, true); - } + thread = add_gpu_thread (inf, event_ptid); /* If the wave is stopped because of a software breakpoint, the program counter needs to be adjusted so that it points to the @@ -1686,10 +1780,7 @@ amd_dbgapi_target::update_thread_list () { ptid_t wave_ptid = make_gpu_ptid (inf->pid, amd_dbgapi_wave_id_t {tid}); - - add_thread_silent (inf->process_target (), wave_ptid); - set_running (inf->process_target (), wave_ptid, true); - set_executing (inf->process_target (), wave_ptid, true); + add_gpu_thread (inf, wave_ptid); } } @@ -2115,6 +2206,7 @@ _initialize_amd_dbgapi_target () gdb::observers::inferior_forked.attach (amd_dbgapi_inferior_forked, "amd-dbgapi"); gdb::observers::inferior_exit.attach (amd_dbgapi_inferior_exited, "amd-dbgapi"); gdb::observers::inferior_pre_detach.attach (amd_dbgapi_inferior_pre_detach, "amd-dbgapi"); + gdb::observers::thread_deleted.attach (amd_dbgapi_thread_deleted, "amd-dbgapi"); add_basic_prefix_cmd ("amdgpu", no_class, _("Generic command for setting amdgpu flags."), diff --git a/gdb/observable.c b/gdb/observable.c index f2e65b11604..29675f3abf3 100644 --- a/gdb/observable.c +++ b/gdb/observable.c @@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (all_objfiles_removed); DEFINE_OBSERVABLE (free_objfile); DEFINE_OBSERVABLE (new_thread); DEFINE_OBSERVABLE (thread_exit); +DEFINE_OBSERVABLE (thread_deleted); DEFINE_OBSERVABLE (thread_stop_requested); DEFINE_OBSERVABLE (target_resumed); DEFINE_OBSERVABLE (about_to_proceed); diff --git a/gdb/observable.h b/gdb/observable.h index 32ef65435cc..91a2c871524 100644 --- a/gdb/observable.h +++ b/gdb/observable.h @@ -126,6 +126,11 @@ extern observable /* exit_code */, bool /* silent */> thread_exit; +/* The thread specified by T has been deleted, with delete_thread. + This is called just before the thread_info object is destroyed with + operator delete. */ +extern observable thread_deleted; + /* An explicit stop request was issued to PTID. If PTID equals minus_one_ptid, the request applied to all threads. If ptid_is_pid(PTID) returns true, the request applied to all diff --git a/gdb/thread.c b/gdb/thread.c index 85bdbaa6cd8..bd3fe85f3b9 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -527,6 +527,8 @@ delete_thread_1 (thread_info *thr, std::optional exit_code, auto it = thr->inf->thread_list.iterator_to (*thr); thr->inf->thread_list.erase (it); + gdb::observers::thread_deleted.notify (thr); + delete thr; }