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.133.124]) by sourceware.org (Postfix) with ESMTPS id EB7993858C27 for ; Wed, 20 Sep 2023 14:22:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EB7993858C27 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=1695219774; 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=1ko5B62BVHK/rykuAxzWjNoi8hQrnV2+rZJb4+Pr4F0=; b=dkpFNcBEykmUvVYSx2f0FLlPG/xpHRvW0acsmfqBdeZq84o77U5btydOaeaM7JrqAHIMyA 2hObc/4Qj/WJlHOAKestJ6OyQPILiwT+pAerX87lZeBvp6Gkcah1pwBL/3JigmYpc1o7go XGd30s1va9kxhEs4FBsy/SbkWOB6LSg= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-364-fA7jOfVZOW-pZvFB1j1e_w-1; Wed, 20 Sep 2023 10:22:53 -0400 X-MC-Unique: fA7jOfVZOW-pZvFB1j1e_w-1 Received: by mail-ed1-f69.google.com with SMTP id 4fb4d7f45d1cf-52f274df255so1674699a12.1 for ; Wed, 20 Sep 2023 07:22:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695219772; x=1695824572; 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=1ko5B62BVHK/rykuAxzWjNoi8hQrnV2+rZJb4+Pr4F0=; b=wSpsAIan97Ig3rl/4ANXp0ejpG5KhEiss/+uzxnbMpH/6LXLAFYkGKv0Ru+rlXdbqv 2k5mEgsLIZ6JzbwaEPzPp1aQ+g8nOuDT4UEZWmQgq6IeBKzE3u+3oiONYbb9VxPtMpmD 2b/xgxFKnWSC2HL6O/G/JIKO4UwKBViAPyaCRAQSMkwfTpUUQLIaETKjGZuFij1MNUYM KZW5F5QALoM63Xh1wWj6zTMrHzlspTf/3uaJzFrV7dPFUV54EVEFpK2gPpmQpf8Mzrg5 j9Frb7SlgkPzdJLLQOYMg+p7R0e61fmSNFOTlxW49EUjP2urZENpFwVWVH29S8K1z4uh BZEQ== X-Gm-Message-State: AOJu0YyiPBt50XfXyyXkcfEX4tIVW2NBTTtPKZM4LGD5pvaXSOjile33 k4V/aBW3WWYY14iu1lrqRLncdv31Rtahr4MCtGJHOBqiiXcwl79cF+jDtVtUKUTQcV0G/0EU70u os86jug0mDF8AM7Kyxu3aEw== X-Received: by 2002:a17:906:518a:b0:9a2:86a:f9c0 with SMTP id y10-20020a170906518a00b009a2086af9c0mr3740644ejk.1.1695219771937; Wed, 20 Sep 2023 07:22:51 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFw2Tp5cXpMSk6teODeUYOHE0lu28n/DaktM64FOoQ3ftUzv1JkWDYHvCjl3mc0LV8dWDBaVA== X-Received: by 2002:a17:906:518a:b0:9a2:86a:f9c0 with SMTP id y10-20020a170906518a00b009a2086af9c0mr3740625ejk.1.1695219771520; Wed, 20 Sep 2023 07:22:51 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id jw24-20020a17090776b800b009a168ab6ee2sm9393552ejc.164.2023.09.20.07.22.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 07:22:51 -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: <20230918212651.660141-16-luis.machado@arm.com> References: <20230918212651.660141-1-luis.machado@arm.com> <20230918212651.660141-16-luis.machado@arm.com> Date: Wed, 20 Sep 2023 15:22:49 +0100 Message-ID: <87pm2cyldy.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.7 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_H3,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 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. Thanks, Andrew > > Regression-tested on aarch64-linux Ubuntu 22.04/20.04. > --- > gdb/aarch64-linux-tdep.c | 33 ++++++++++++++++++++++++++ > gdb/arch-utils.c | 10 ++++++++ > gdb/arch-utils.h | 6 +++++ > gdb/corelow.c | 50 ++++++++++++++++++++++++--------------- > gdb/gdbarch-gen.h | 14 +++++++++++ > gdb/gdbarch.c | 22 +++++++++++++++++ > gdb/gdbarch_components.py | 19 +++++++++++++++ > 7 files changed, 135 insertions(+), 19 deletions(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 47e5e1db641..21ac7ebdc56 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -2199,6 +2199,33 @@ aarch64_linux_decode_memtag_section (struct gdbarch *gdbarch, > return tags; > } > > +/* AArch64 Linux implementation of the > + gdbarch_use_target_description_from_corefile_notes hook. */ > + > +static bool > +aarch64_use_target_description_from_corefile_notes (gdbarch *gdbarch, > + bfd *obfd) > +{ > + /* Sanity check. */ > + gdb_assert (obfd != nullptr); > + > + /* If the corefile contains any SVE or SME register data, we don't want to > + use the target description note, as it may be incorrect. > + > + Currently the target description note contains a potentially incorrect > + target description if the originating program changed the SVE or SME > + vector lengths mid-execution. > + > + Once we support per-thread target description notes in the corefiles, we > + can always trust those notes whenever they are available. */ > + if (bfd_get_section_by_name (obfd, ".reg-aarch-sve") != nullptr > + || bfd_get_section_by_name (obfd, ".reg-aarch-za") != nullptr > + || bfd_get_section_by_name (obfd, ".reg-aarch-zt") != nullptr) > + return false; > + > + return true; > +} > + > static void > aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > { > @@ -2469,6 +2496,12 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > aarch64_displaced_step_hw_singlestep); > > set_gdbarch_gcc_target_options (gdbarch, aarch64_linux_gcc_target_options); > + > + /* Hook to decide if the target description should be obtained from > + corefile target description note(s) or inferred from the corefile > + sections. */ > + set_gdbarch_use_target_description_from_corefile_notes (gdbarch, > + aarch64_use_target_description_from_corefile_notes); > } > > #if GDB_SELF_TEST > diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c > index ee34fc07d33..c1d2c939eb9 100644 > --- a/gdb/arch-utils.c > +++ b/gdb/arch-utils.c > @@ -1092,6 +1092,16 @@ default_read_core_file_mappings > { > } > > +/* See arch-utils.h. */ > +bool > +default_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, > + struct bfd *obfd) > +{ > + /* Always trust the corefile target description contained in the target > + description note. */ > + return true; > +} > + > CORE_ADDR > default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame) > { > diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h > index 2bdc3251c9c..5df3de7b5d9 100644 > --- a/gdb/arch-utils.h > +++ b/gdb/arch-utils.h > @@ -305,6 +305,12 @@ extern void default_read_core_file_mappings > read_core_file_mappings_pre_loop_ftype pre_loop_cb, > read_core_file_mappings_loop_ftype loop_cb); > > +/* Default implementation of gdbarch > + use_target_description_from_corefile_notes. */ > +extern bool default_use_target_description_from_corefile_notes > + (struct gdbarch *gdbarch, > + struct bfd *obfd); > + > /* Default implementation of gdbarch default_get_return_buf_addr method. */ > extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch, > frame_info_ptr cur_frame); > diff --git a/gdb/corelow.c b/gdb/corelow.c > index 439270f5559..114ce3054d5 100644 > --- a/gdb/corelow.c > +++ b/gdb/corelow.c > @@ -1229,35 +1229,47 @@ core_target::thread_alive (ptid_t ptid) > const struct target_desc * > core_target::read_description () > { > - /* If the core file contains a target description note then we will use > - that in preference to anything else. */ > - bfd_size_type tdesc_note_size = 0; > - struct bfd_section *tdesc_note_section > - = bfd_get_section_by_name (core_bfd, ".gdb-tdesc"); > - if (tdesc_note_section != nullptr) > - tdesc_note_size = bfd_section_size (tdesc_note_section); > - if (tdesc_note_size > 0) > + /* First check whether the target wants us to use the corefile target > + description notes. */ > + if (gdbarch_use_target_description_from_corefile_notes (m_core_gdbarch, > + core_bfd)) > { > - gdb::char_vector contents (tdesc_note_size + 1); > - if (bfd_get_section_contents (core_bfd, tdesc_note_section, > - contents.data (), (file_ptr) 0, > - tdesc_note_size)) > + /* If the core file contains a target description note then go ahead and > + use that. */ > + bfd_size_type tdesc_note_size = 0; > + struct bfd_section *tdesc_note_section > + = bfd_get_section_by_name (core_bfd, ".gdb-tdesc"); > + if (tdesc_note_section != nullptr) > + tdesc_note_size = bfd_section_size (tdesc_note_section); > + if (tdesc_note_size > 0) > { > - /* Ensure we have a null terminator. */ > - contents[tdesc_note_size] = '\0'; > - const struct target_desc *result > - = string_read_description_xml (contents.data ()); > - if (result != nullptr) > - return result; > + gdb::char_vector contents (tdesc_note_size + 1); > + if (bfd_get_section_contents (core_bfd, tdesc_note_section, > + contents.data (), (file_ptr) 0, > + tdesc_note_size)) > + { > + /* Ensure we have a null terminator. */ > + contents[tdesc_note_size] = '\0'; > + const struct target_desc *result > + = string_read_description_xml (contents.data ()); > + if (result != nullptr) > + return result; > + } > } > } > > + /* If the architecture provides a corefile target description hook, use > + it now. Even if the core file contains a target description in a note > + section, it is not useful for targets that can potentially have distinct > + descriptions for each thread. One example is AArch64's SVE/SME > + extensions that allow per-thread vector length changes, resulting in > + registers with different sizes. */ > if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch)) > { > const struct target_desc *result; > > result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd); > - if (result != NULL) > + if (result != nullptr) > return result; > } > > diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h > index d62eefa1c5b..33276aa1c43 100644 > --- a/gdb/gdbarch-gen.h > +++ b/gdb/gdbarch-gen.h > @@ -1717,3 +1717,17 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g > typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb); > extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb); > extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings); > + > +/* Return true if the target description for all threads should be read from the > + target description core file note(s). Return false if the target description > + for all threads should be inferred from the core file contents/sections. > + > + The corefile's bfd is passed through OBFD. > + > + This hook should be used by targets that can have distinct target descriptions > + for each thread when the core file only holds a single target description > + note. */ > + > +typedef bool (gdbarch_use_target_description_from_corefile_notes_ftype) (struct gdbarch *gdbarch, struct bfd *obfd); > +extern bool gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, struct bfd *obfd); > +extern void set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes); > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c > index 1fc254d3d6e..ee868908598 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -256,6 +256,7 @@ struct gdbarch > gdbarch_type_align_ftype *type_align = default_type_align; > gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags; > gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings; > + gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes = default_use_target_description_from_corefile_notes; > }; > > /* Create a new ``struct gdbarch'' based on information provided by > @@ -523,6 +524,7 @@ verify_gdbarch (struct gdbarch *gdbarch) > /* Skip verify of type_align, invalid_p == 0 */ > /* Skip verify of get_pc_address_flags, invalid_p == 0 */ > /* Skip verify of read_core_file_mappings, invalid_p == 0 */ > + /* Skip verify of use_target_description_from_corefile_notes, invalid_p == 0 */ > if (!log.empty ()) > internal_error (_("verify_gdbarch: the following are invalid ...%s"), > log.c_str ()); > @@ -1373,6 +1375,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file) > gdb_printf (file, > "gdbarch_dump: read_core_file_mappings = <%s>\n", > host_address_to_string (gdbarch->read_core_file_mappings)); > + gdb_printf (file, > + "gdbarch_dump: use_target_description_from_corefile_notes = <%s>\n", > + host_address_to_string (gdbarch->use_target_description_from_corefile_notes)); > if (gdbarch->dump_tdep != NULL) > gdbarch->dump_tdep (gdbarch, file); > } > @@ -5409,3 +5414,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, > { > gdbarch->read_core_file_mappings = read_core_file_mappings; > } > + > +bool > +gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, struct bfd *obfd) > +{ > + gdb_assert (gdbarch != NULL); > + gdb_assert (gdbarch->use_target_description_from_corefile_notes != NULL); > + if (gdbarch_debug >= 2) > + gdb_printf (gdb_stdlog, "gdbarch_use_target_description_from_corefile_notes called\n"); > + return gdbarch->use_target_description_from_corefile_notes (gdbarch, obfd); > +} > + > +void > +set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, > + gdbarch_use_target_description_from_corefile_notes_ftype use_target_description_from_corefile_notes) > +{ > + gdbarch->use_target_description_from_corefile_notes = use_target_description_from_corefile_notes; > +} > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py > index 846467b8d83..bbb9b188286 100644 > --- a/gdb/gdbarch_components.py > +++ b/gdb/gdbarch_components.py > @@ -2732,3 +2732,22 @@ Read core file mappings > predefault="default_read_core_file_mappings", > invalid=False, > ) > + > +Method( > + comment=""" > +Return true if the target description for all threads should be read from the > +target description core file note(s). Return false if the target description > +for all threads should be inferred from the core file contents/sections. > + > +The corefile's bfd is passed through OBFD. > + > +This hook should be used by targets that can have distinct target descriptions > +for each thread when the core file only holds a single target description > +note. > +""", > + type="bool", > + name="use_target_description_from_corefile_notes", > + params=[("struct bfd *", "obfd")], > + predefault="default_use_target_description_from_corefile_notes", > + invalid=False, > +) > -- > 2.25.1