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 F153B3858D39 for ; Wed, 20 Sep 2023 15:27:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F153B3858D39 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=1695223622; 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=025ZB1+j15u9qX17TZmEGnoBccf8CjqJhfbcqGhy6Ts=; b=C0fdnh8P/p7vxy89NuOPymbE9DoZgrABfVXNDsWaIaISriV8FL9gyptxDia7KrfMFOG6UJ LIDG5jq4qrJa0J+FFE8/T1Y0Xuxj0pky4ozBFlGEBF9Y65/UJc5J+gdJGczfPQ9C+bZuhp CEGU1uO4CFNAAlOKdnzb/BzlGWJIy/w= Received: from mail-lf1-f71.google.com (mail-lf1-f71.google.com [209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-211-9hilsMEzMjiztjkmcrl_gQ-1; Wed, 20 Sep 2023 11:27:01 -0400 X-MC-Unique: 9hilsMEzMjiztjkmcrl_gQ-1 Received: by mail-lf1-f71.google.com with SMTP id 2adb3069b0e04-503c774fd61so1236261e87.3 for ; Wed, 20 Sep 2023 08:27:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695223620; x=1695828420; 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=025ZB1+j15u9qX17TZmEGnoBccf8CjqJhfbcqGhy6Ts=; b=mE4CONd+b5oYQRQPtizLpHS5mO7SYFhAxTqMfVFNrzjdXdivEL4BmeNV+0bEaoF8ZF P1ylVLtikSrai8b1wDv5NImKWY1yy8MPt/56DY3fpRB/mwOVQKUjr1e53cAi1Aqai56V Mmz9gqs4apJ+fAwhlTCVC8++Sqogs8FoNIb4/WhUrM55lkLXu9gOHFMLKzHiOTBf+G1n gM1FTqI3zQkmy83nh1wd35zs6LomG08Q1aaloYt2teLiCSc5K+ZjIBd6MB5n0MuAZQAo rKfkmQHmQY/aUMWXNv3vmsjDCpkhDp97u0gyDAnwJy8TSJiZxDnlQzOtxDYSHTzyClV9 eI8w== X-Gm-Message-State: AOJu0YxMxqPUvr632/G/Q2aB0NMt1dPXDktByQFxoHtS4t/CuCmuaTEI lWdD51Qh3VUdFxFa+n0WFwclnh15Ti8lewsDqKZYA6FEsrhHXzcko1eN3Dq8Yoh+mHeWb7mDCb0 k/mztaPBm/2/zXGKzaYDO6Q== X-Received: by 2002:a19:504b:0:b0:500:c348:7efd with SMTP id z11-20020a19504b000000b00500c3487efdmr2380741lfj.59.1695223619755; Wed, 20 Sep 2023 08:26:59 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF/ruX2fFSngtR59yOAzBfqTfhGfQ9SRQ402K4TbA71mMOg7+aYdyJeY40ejZCV4FCufxqDfg== X-Received: by 2002:a19:504b:0:b0:500:c348:7efd with SMTP id z11-20020a19504b000000b00500c3487efdmr2380727lfj.59.1695223619278; Wed, 20 Sep 2023 08:26:59 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id cf20-20020a170906b2d400b0099bd7b26639sm9574384ejb.6.2023.09.20.08.26.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 08:26: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: <87pm2cyldy.fsf@redhat.com> References: <20230918212651.660141-1-luis.machado@arm.com> <20230918212651.660141-16-luis.machado@arm.com> <87pm2cyldy.fsf@redhat.com> Date: Wed, 20 Sep 2023 16:26:57 +0100 Message-ID: <87led0yif2.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=-11.8 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: 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. 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, 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 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? 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. Thanks, Andrew --- diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index 2885afd60c7..f1f7675eb37 100644 --- a/gdb/linux-tdep.c +++ b/gdb/linux-tdep.c @@ -2077,6 +2077,8 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size) else stop_signal = GDB_SIGNAL_0; + bool multiple_arches = false; + struct gdbarch *some_arch = nullptr; if (signalled_thr != nullptr) { /* On some architectures, like AArch64, each thread can have a distinct @@ -2085,8 +2087,8 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size) Fetch each thread's gdbarch and pass it down to the lower layers so we can dump the right set of registers. */ - linux_corefile_thread (signalled_thr, - target_thread_architecture (signalled_thr->ptid), + some_arch = target_thread_architecture (signalled_thr->ptid); + linux_corefile_thread (signalled_thr, some_arch, obfd, note_data, note_size, stop_signal); } for (thread_info *thr : current_inferior ()->non_exited_threads ()) @@ -2100,7 +2102,10 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size) Fetch each thread's gdbarch and pass it down to the lower layers so we can dump the right set of registers. */ - linux_corefile_thread (thr, target_thread_architecture (thr->ptid), + struct gdbarch *tmp = target_thread_architecture (thr->ptid); + if (tmp != some_arch) + multiple_arches = true; + linux_corefile_thread (thr, tmp, obfd, note_data, note_size, stop_signal); } @@ -2125,7 +2130,8 @@ 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 (!multiple_arches) + gcore_elf_make_tdesc_note (obfd, ¬e_data, note_size); return note_data; }