From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 6E5D33858D39 for ; Thu, 21 Sep 2023 10:03:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6E5D33858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695290584; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9bquXj1YCt/73Ra5/bNVqG2djzxFMg0WXoktHJrSgag=; b=aVw6uGp0TfSIlqKRPUb2YKsLUuhr/Wx4KGQCwOau5DcZsx6b+eiw4cc6htyRDzsdoq1LMg 8WAmiOQ5iJJzkn6e5IpYRxUQ91X294W0c8Os6ATgXGB6GcjqjgC28cksyrOY3tEYUaVwMG KhjqLOm3kSf2vrCszdTy31JWvvF367A= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-563-YQPQJf43PHOOfhewzlYJbg-1; Thu, 21 Sep 2023 06:03:00 -0400 X-MC-Unique: YQPQJf43PHOOfhewzlYJbg-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-402d1892cecso3224815e9.1 for ; Thu, 21 Sep 2023 03:03:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695290579; x=1695895379; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=9bquXj1YCt/73Ra5/bNVqG2djzxFMg0WXoktHJrSgag=; b=Iewsq/et0X/WIHdH7gksxBTBWINz2Iif8W+yQAWrYljc97SHMzDB9xf1iZS0AmVFjU +dlcMBDTvntCj7VYCdGzqcupgk2LPxLcA0x6UktQ3bONrkAYaRdHTlfgapJxNtq77eo/ s/iWw+01pepwJ1faBSeyxzDM+1SsHSdsQdqXtRvhcrr2feaEjH6QWKOW2OoLdHyjo8m1 BUJ6I7lhJ/AItETQWI8D3Wg93ER768HP19x9wZlDVuKte7cPVoXVH/FkXiapD9ez4eu6 1diJJTMeh354g5rbhxYRk8gxeKpIuVsaTPJrg+l3Y/WYZ3xby61LE6ZE8lder6K6Qsmg d1kA== X-Gm-Message-State: AOJu0YyfOXgzhcgFrrjYtWkuZV6BU6LOKmlaxlS0YL+o1yz4turr+SVO a4NPRM0oYAWqIltu1eldoa5eSD2/RNpsrJsAbAZWJ8vLhkAqMolCQAxQIEym/ocAD5h4Z9z5k7k 64IzYWGGzeOT2jObPoybZObQFsHkUJQ== X-Received: by 2002:a05:600c:1d8b:b0:3fe:d67d:5040 with SMTP id p11-20020a05600c1d8b00b003fed67d5040mr6582150wms.5.1695290579197; Thu, 21 Sep 2023 03:02:59 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE6KLVNd3XPLk5bE0i5x1M/xqdy9okQVVnHv+qVBeVQakShWcJJOvoIYyy9jAHzX4ARsP9xQg== X-Received: by 2002:a05:600c:1d8b:b0:3fe:d67d:5040 with SMTP id p11-20020a05600c1d8b00b003fed67d5040mr6582134wms.5.1695290578718; Thu, 21 Sep 2023 03:02:58 -0700 (PDT) Received: from localhost ([62.31.95.162]) by smtp.gmail.com with ESMTPSA id g18-20020a5d5552000000b003196b1bb528sm1311802wrw.64.2023.09.21.03.02.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Sep 2023 03:02:58 -0700 (PDT) From: Andrew Burgess To: Luis Machado , gdb-patches@sourceware.org Cc: thiago.bauermann@linaro.org Subject: Re: [PATCH v7 15/18] [gdb/generic] corefile/bug: Add hook to control the use of target description notes from corefiles In-Reply-To: <423fed06-ce80-52d0-013d-03384fecc853@arm.com> References: <20230918212651.660141-1-luis.machado@arm.com> <20230918212651.660141-16-luis.machado@arm.com> <87pm2cyldy.fsf@redhat.com> <87led0yif2.fsf@redhat.com> <423fed06-ce80-52d0-013d-03384fecc853@arm.com> Date: Thu, 21 Sep 2023 11:02:56 +0100 Message-ID: <87il83yhbj.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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/20/23 16:26, Andrew Burgess wrote: >> Andrew Burgess writes: >> >>> Luis Machado via Gdb-patches writes: >>> >>>> New entry in v7. >>>> >>>> --- >>>> >>>> Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread >>>> can potentially have distinct target descriptions/gdbarches. >>>> >>>> When loading a gcore-generated core file, at the moment GDB gives priority >>>> to the target description dumped to NT_GDB_TDESC. Though technically correct >>>> for most targets, it doesn't work correctly for AArch64 with SVE or SME >>>> support. >>>> >>>> The correct approach for AArch64/Linux is to either have per-thread target >>>> description notes in the corefiles or to rely on the >>>> gdbarch_core_read_description hook, so it can figure out the proper target >>>> description for a given thread based on the various available register notes. >>>> >>>> The former, although more correct, doesn't address the case of existing gdb's >>>> that only output a single target description note. >>> >>> Do those existing GDB's support per-thread target descriptions though? >>> I thought this series was the where the per-thread target description >>> feature was being added ... so shouldn't core files generated by >>> previous GDB's only support a single target description? Or am I >>> missing something. >>> >>>> >>>> This patch goes for the latter, and adds a new gdbarch hook to conditionalize >>>> the use of the corefile target description note. The hook is called >>>> use_target_description_from_corefile_notes. >>>> >>>> The hook defaults to returning true, meaning targets will use the corefile >>>> target description note. AArch64 Linux overrides the hook to return false >>>> when it detects any of the SVE or SME register notes in the corefile. >>>> >>>> Otherwise it should be fine for AArch64 Linux to use the corefile target >>>> description note. >>>> >>>> When we support per-thread target description notes, then we can augment >>>> the AArch64 Linux hook to rely on those notes. >>> >>> I guess I was a little surprised that I couldn't see anywhere in this >>> series that you _stop_ adding the NT_GDB_TDESC note to core files >>> generated from within GDB. >>> >>> I guess I was expecting to see some logic in gcore_elf_make_tdesc_note >>> that tried to figure out if we had per-thread tdesc, and if so, at a >>> minimum, didn't add the NT_GDB_TDESC. >>> >>> If we did that, then, I'm thinking: >>> >>> - Previous GDB's only supported a single tdesc, and so are correct, >>> >>> - New GDB's support per-thread tdesc, but don't emit NT_GDB_TDESC, so >>> we don't need to guard against loading them >>> >>> Maybe I'm missing something though. >> >> Luis, >> >> After our discussion on IRC I think I understand this a bit more, so >> thanks for that. > > You're welcome. Hopefully I managed to clarify most of it. > >> >> So, here's what I think is true: >> >> - AArch64 supports per-thread gdbarch since before this patch series >> (implements ::thread_architecture), >> >> - GDB by default writes out a single NT_GDB_TDESC which describes the >> "per-inferior" gdbarch, but this doesn't correctly represent the >> per-thread gdbarch/tdesc, >> >> - If a NT_GDB_TDESC is present then GDB is going to try and load it >> back in and use it, >> >> - This is a problem that has existed for a while, but you've only just >> become aware, and so you're fixing it here, >> >> - The new gdbarch hook allows an architecture to avoid loading a >> single NT_GDB_TDESC note, for AArch64 this is when we see registers >> that are (likely) going to require per-thread gdbarch/tdesc, >> > > All of the above are correct. > >> I'm hoping we both agree on the above. So from there I think I has two >> concerns: >> >> 1. GDB should avoid writing out the NT_GDB_TDESC is the situation where >> it know that a single such note is not correct, and > > I see your point now. Not outputting NT_GDB_TDESC when it is incorrect sounds > reasonable, even if the incorrect NT_GDB_TDESC note won't be used. > > It is also reasonable to do it until the point where we support per-thread > NT_GDB_TDESC's, which is, I think, the proper solution. > >> >> 2. I wonder if there's a better solution than the new hook. >> >> For point (1) the patch below is a rough draft of what I'm thinking >> about: by looking at each of the gdbarch pointers we can spot if an >> inferior uses multiple different gdbarches -- if it does then we know >> that a single NT_GDB_TDESC is going to be wrong, so lets not write it >> out. >> >> As was mentioned on IRC, longer term, it might be better if we wrote out >> one tdesc for each thread, but that feels like a bigger job, and I think >> stopping GDB doing something that's just *wrong* is pretty easy, so why >> wouldn't we do that? > > I think outputting per-thread NT_GDB_TDESC's is fairly easy. Probably a matter of > calling the function that outputs NT_GDB_TDESC while we're dumping register sets for > each thread. Indeed, I have this bit done locally. What isn't clear to me is how having multiple NT_GDB_TDESC will help -- does core_file::read_description get called multiple times when using AArch64 with sve/sme? > > I haven't investigated in-depth how to use that information when loading a core file, but > there might be some complexity because we read the target description before we load the threads. > > If the target description we read first is *always* the one for the signalled thread, then we might > be ok and it should make the implementation easier. Anyway, that's a > bit off-topic for this patch. I mean, maybe this is a better solution? What if gcore_elf_make_tdesc_note took a 'struct gdbarch *' argument, and it was the callers to chose which gdbarch to pass. We would then spec in the comments of gcore_elf_make_tdesc_note that the passed in gdbarch should be the gdbarch of the signalled thread. See the patch below. Could I ask: would you mind sending me a small application and corefile for AArch64 with sve/sme please. I'd love to have a play to better understand how GDB sets up the per-thread gdbarch in that case. I'd be very grateful. > >> >> For point (2) I agree with the premise that we need a mechanism to stop >> AArch64 loading the incorrect single NT_GDB_TDESC. This is a slight >> change in stance from what I originally wrote, but our IRC conversation >> showed me I was wrong originally. I don't have time this evening to >> look at this, but will follow up again tomorrow with more thoughts. I wonder, instead of adding the new hook, maybe we should just reorder the checks in core_target::read_description -- ask the gdbarch to grok a tdesc from the .regs (etc) sections, and if that fails, check for an encoded tdesc. When I originally added this code my problem case was RISC-V, where the CSRs (control regs) for embedded targets can be different for each target. The CSRs are just encoded as a blob, so unless you already know which regs are present you're stuck. For real h/w the controller (e.g. openocd) provides a tdesc (via target.xml), but for a core file we needed a way to reacquire the tdesc. My assumption at the time that the tdesc we wrote would always be correct, but clearly this isn't the case. Right now RISC-V doesn't event override gdbarch_core_read_description, so if we read the encoded tdesc after checking that gdbarch hook nothing would change. For other architectures, if gdbarch_core_read_description is implemented then we'll be back to using that, and that worked fine before. ... And if, in some future world we do implement gdbarch_core_read_description for RISC-V then the solution seems obvious, if there's a section containing CSRs, and there's also a section containing a tdesc, then the RISC-V gdbarch_core_read_description can just return nullptr, safe in the knowledge the generic GDB code will load the encoded tdesc. > > Except for some different names and tweaks, your proposed approach works correctly. > > I tested this and noticed the lack of NT_GDB_TDESC for a AArch64 Linux target > with sve/sme support and one thread with a differing gdbarch. I also saw the note for > a AArch64 Linux target without sve/sme support, or with sve/sme support but all threads > having the exact same gdbarch. So that looks good to me. > > Would you like this extra code to be included as part of this > particular patch? Yeah I think something like this should be added to this patch, we should stop GDB creating incorrect core files I think. Thanks, Andrew ---- diff --git a/gdb/elf-none-tdep.c b/gdb/elf-none-tdep.c index 460f02e7dbb..ad31d3c820a 100644 --- a/gdb/elf-none-tdep.c +++ b/gdb/elf-none-tdep.c @@ -111,7 +111,9 @@ elf_none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, /* Target description. */ - gcore_elf_make_tdesc_note (obfd, ¬e_data, note_size); + if (signalled_thr != nullptr) + gdbarch = target_thread_architecture (signalled_thr->ptid); + gcore_elf_make_tdesc_note (gdbarch, obfd, ¬e_data, note_size); return note_data; } diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c index dc5020d92d2..c44dc0871c6 100644 --- a/gdb/fbsd-tdep.c +++ b/gdb/fbsd-tdep.c @@ -774,7 +774,9 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size) } /* Include the target description when possible. */ - gcore_elf_make_tdesc_note (obfd, ¬e_data, note_size); + if (signalled_thr != nullptr) + gdbarch = target_thread_architecture (signalled_thr->ptid); + gcore_elf_make_tdesc_note (gdbarch, obfd, ¬e_data, note_size); return note_data; } diff --git a/gdb/gcore-elf.c b/gdb/gcore-elf.c index 643426d911b..0142db82670 100644 --- a/gdb/gcore-elf.c +++ b/gdb/gcore-elf.c @@ -139,12 +139,12 @@ gcore_elf_build_thread_register_notes /* See gcore-elf.h. */ void -gcore_elf_make_tdesc_note (bfd *obfd, +gcore_elf_make_tdesc_note (struct gdbarch *gdbarch, bfd *obfd, gdb::unique_xmalloc_ptr *note_data, int *note_size) { /* Append the target description to the core file. */ - const struct target_desc *tdesc = gdbarch_target_desc (target_gdbarch ()); + const struct target_desc *tdesc = gdbarch_target_desc (gdbarch); const char *tdesc_xml = tdesc == nullptr ? nullptr : tdesc_get_features_xml (tdesc); if (tdesc_xml != nullptr && *tdesc_xml != '\0') diff --git a/gdb/gcore-elf.h b/gdb/gcore-elf.h index 2cfeb3e8550..e23fe5e7162 100644 --- a/gdb/gcore-elf.h +++ b/gdb/gcore-elf.h @@ -42,6 +42,7 @@ extern void gcore_elf_build_thread_register_notes set to nullptr. */ extern void gcore_elf_make_tdesc_note - (bfd *obfd, gdb::unique_xmalloc_ptr *note_data, int *note_size); + (struct gdbarch *gdbarch, bfd *obfd, + gdb::unique_xmalloc_ptr *note_data, int *note_size); #endif /* GCORE_ELF_H */ diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index 2885afd60c7..24ae1d711d1 100644 --- a/gdb/linux-tdep.c +++ b/gdb/linux-tdep.c @@ -1836,6 +1836,10 @@ linux_corefile_thread (struct thread_info *info, such a core file is useless. */ if (note_data != nullptr) { + /* If we want per-thread NT_GDB_TDESC then at this point we should + do this: */ + // gcore_elf_make_tdesc_note (gdbarch, obfd, ¬e_data, note_size); + gdb::byte_vector siginfo_data = linux_get_siginfo_data (info, gdbarch); if (!siginfo_data.empty ()) @@ -2125,7 +2129,9 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size) linux_make_mappings_corefile_notes (gdbarch, obfd, note_data, note_size); /* Target description. */ - gcore_elf_make_tdesc_note (obfd, ¬e_data, note_size); + if (signalled_thr != nullptr) + gdbarch = target_thread_architecture (signalled_thr->ptid); + gcore_elf_make_tdesc_note (gdbarch, obfd, ¬e_data, note_size); return note_data; } ---