From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id E29533845193 for ; Sat, 26 Nov 2022 01:47:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E29533845193 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x329.google.com with SMTP id cn2-20020a056830658200b0066c74617e3dso3674094otb.2 for ; Fri, 25 Nov 2022 17:47:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=KlBUTlzfYS21lByjD2Ao5n6WbRxfjMgTxoYDMcwbYMI=; b=lpd280LtSpS17FlA3vKDYBqhigTMgMkIANIGEqCYO0o+PPDTgcbW2y5WK1Yf93Bofh 5+fNGiZGg+Gp0237zpqwaKhQwPZUD+yxuYshW5MNJvCiVpXc5HctwhJTzTkDlLignc4g RUYA9v2F1KhvQMFpEVb+aFdZrZ3axl9c4Ri7PznLJ0p7bjhIwP22j84KnRxUGbJoq/Ik jY93s8s15W7P7MMQf11fH1HKl5YSfvTjroIpNcKBkQmv3vn1eRiEqDeEE8V9VcWJiqoB qpsaGrG+g6DuZBywSIDJCNzRHVJSsK6eNOHBk20Vc3oQjmlgRYv+GEpU9uf511oCZI38 b91w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=KlBUTlzfYS21lByjD2Ao5n6WbRxfjMgTxoYDMcwbYMI=; b=OQoxOKEfdrojT2rfz3ye1Z+LFLwQ8n0vwdyPN9xPJT1YCSgFEJw/ooEsak0f8wwkuw tfYqEGoNKFlyERKZqTDEo1b0i8fLf7DG4TJDihsbk13Sxg2ad12LaYbAxzwluow89TZ2 MH7vrdzWZtl7LaSpexScZI1JzKLIEkAVvDf3LgBeBWT4x6VFFxADIpxJp9ekQWYnlQEz tql3/aqt6SIDyOhtWxzl/5Mpk+QjP8uND912qvkil42W3CxGSdyZLfVAaw9Prz2W3xZN 9cXWdkrjhn9vbmJRAjN2Lr7ldBoR/p5kwbrW/SwsBoFFy6lxuOD+u+5Oty2HXHoJXhl1 kfvw== X-Gm-Message-State: ANoB5pnXn7/PwO+wKVMmo7ezRVFok/9xCo1OeNiMhr+s7XaZ2KevSoDd pReKDaDcffk0+E2ZuhtThC0ibX+K27Vrsw== X-Google-Smtp-Source: AA0mqf7RHyRtpsQGh8RkooyP6MSnGypBvP9Aj7OUkioEK3pwXQ6IWeeynp0t2ywremxb7HdfNfCecw== X-Received: by 2002:a05:6830:3498:b0:66d:8a3d:e2e9 with SMTP id c24-20020a056830349800b0066d8a3de2e9mr13990809otu.94.1669427265225; Fri, 25 Nov 2022 17:47:45 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:41ee:c7fc:c991:eee6]) by smtp.gmail.com with ESMTPSA id w14-20020a9d638e000000b006690f65a830sm2284217otk.14.2022.11.25.17.47.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Nov 2022 17:47:44 -0800 (PST) References: <20220908064151.3959930-1-thiago.bauermann@linaro.org> <20220908064151.3959930-6-thiago.bauermann@linaro.org> <1df73e50-a195-0db3-bf27-73e3550903c2@arm.com> User-agent: mu4e 1.8.11; emacs 28.2 From: Thiago Jung Bauermann To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread In-reply-to: <1df73e50-a195-0db3-bf27-73e3550903c2@arm.com> Date: Sat, 26 Nov 2022 01:47:42 +0000 Message-ID: <87pmda8p1d.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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: Luis Machado writes: > On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote: >> --- a/gdbserver/gdbthread.h >> +++ b/gdbserver/gdbthread.h >> @@ -80,6 +80,8 @@ struct thread_info >> /* Branch trace target information for this thread. */ >> struct btrace_target_info *btrace = nullptr; >> + >> + const struct target_desc *tdesc = nullptr; > > Should we keep a process-wide target description in case we fail to > detect the target description for a given thread? That is a great idea. I implemented it in v2, with the difference that we never fail to detect the target description for a thread. What can happen is that we fail to detect SVE, and in that case it is assumed to be unsupported. The thread-specific target description is used only for inferiors where we detect SVE, and the process-wide one used for all the others. > Also, it may simplify some of the changes in this patch. Indeed, it simplified many of the changes in this patch and the others. Thank you for the suggestion! >> --- a/gdbserver/linux-aarch64-low.cc >> +++ b/gdbserver/linux-aarch64-low.cc >> @@ -191,9 +191,26 @@ struct arch_process_info >> static int >> is_64bit_tdesc (void) >> { >> + const target_desc *tdesc; >> + >> + if (current_thread && current_thread->tdesc) >> + tdesc = current_thread->tdesc; >> + else >> + { >> + /* Find some thread that has a target description. */ >> + const struct thread_info *thread >> + = find_thread (current_process ()->pid, [] (thread_info *thr) { >> + return thr->tdesc != nullptr; >> + }); >> + >> + gdb_assert (thread != nullptr); >> + >> + tdesc = thread->tdesc; >> + } >> + >> /* We may not have a current thread at this point, so go straight to >> the process's target description. */ >> - return register_size (current_process ()->tdesc, 0) == 8; >> + return register_size (tdesc, 0) == 8; >> } > > Keeping the process-wide target description would simplify this > function in my opinion. We won't have 64-bit processes spawning 32-bit > threads. You are right. This patch series doesn't change this function anymore. >> @@ -678,6 +695,31 @@ void >> aarch64_target::low_new_thread (lwp_info *lwp) >> { >> aarch64_linux_new_thread (lwp); >> + >> + ptid_t ptid = ptid_of_lwp (lwp); >> + process_info *proc = find_process_pid (ptid.pid ()); >> + >> + /* If the inferior is still going through the shell or the thread isn't >> + stopped we can't read its registers in order to read the target >> + description. */ >> + if (proc->starting_up) >> + return; >> + >> + thread_info *thread = get_lwp_thread (lwp); >> + unsigned int machine; >> + gdb_assert (ptid.lwp_p ()); >> + bool is_elf64 = linux_pid_exe_is_elf_64_file (ptid.lwp (), &machine); >> + >> + if (is_elf64) >> + { >> + gdb::optional features >> + = aarch64_get_arch_features (thread); >> + >> + if (features.has_value()) >> + thread->tdesc = aarch64_linux_read_description (*features); >> + } >> + else >> + thread->tdesc = aarch32_linux_read_description (); >> } >> void >> @@ -848,14 +890,14 @@ aarch64_target::low_arch_setup () >> succeeds so all we can do here is assert that features is valid. */ >> gdb_assert (features.has_value ()); >> - current_process ()->tdesc = aarch64_linux_read_description (*features); >> + current_thread->tdesc = aarch64_linux_read_description (*features); >> /* Adjust the register sets we should use for this particular set of >> features. */ >> aarch64_adjust_register_sets (*features); >> } >> else >> - current_process ()->tdesc = aarch32_linux_read_description (); >> + current_thread->tdesc = aarch32_linux_read_description (); > > Given 32-bit Arm processes will have the same target description > throughout their lives, would it make sense to have a single target > description stored process-wide, and whenever we need the description > we can find it there? Yes, you are right. In v2 low_arch_setup keeps 32-bit Arm the same as before, with only process-wide target description. AArch64 inferiors that don't support SVE also get only a process-wide target description. > Maybe code it as: > > return current_thread->tdesc == nullptr? current_process ()->tdesc : > current_thread->tdesc; I did that. It turns out that this logic is only needed in two places: regcache.cc::get_thread_regcache and tdesc.cc::current_target_desc. > This would make the changes to all the other targets unnecessary, as > those would be able to lookup their target descriptions from the > process instead of the threads. Indeed! -- Thiago