[RFC PATCH v4 07/15] GDB, gdbserver: Create concept of load-early registers
Aktemur, Tankut Baris
tankut.baris.aktemur@intel.com
Mon Dec 30 14:33:42 GMT 2024
On Saturday, November 2, 2024 3:56 AM, Thiago Jung Bauermann wrote:
> These are registers that need to be saved first into the regcache.
> Consequently, also gdbserver needs to send these registers as expedited
> registers so that GDB can use them as early as possible.
>
> When debugging a remote target, load early registers using expedited
> registers and the p packet.
>
> For now the load-early registers concept is unused, as nothing in this
> patch adds registers to the load-early set. It is being sent separately to
> facilitate review.
>
> In a subsequent patch, registers which are used in location expressions
> that determine the size of other registers will need to be loaded early
> into the regcache so that they can be used to resolve the dynamic types of
> the variable-sized registers.
>
> This will be done by calling tdesc_create_reg () with the new load_early
> argument set to true.
>
> If I can move the VG register before the Z registers as Pedro suggested,
> this patch may be unnecessary.
Ignoring the possible solution by reordering the registers...
Is this patch necessary for the native target to work properly, too?
I thought the remote server case was the issue. If that understanding of
mine is right, instead of introducing a new concept of load-early registers,
can we use the expedite registers? So, the server can report the expedite
registers as part of the tdesc, and GDB would then fetch expedite registers
in remote_target before fetching all with 'g'?
> ---
> gdb/process-stratum-target.h | 3 ++
> gdb/record-full.c | 18 +++++--
> gdb/regcache.c | 99 +++++++++++++++++++++++++++++++++++-
> gdb/regcache.h | 17 +++++++
> gdb/regformats/regdef.h | 16 ++++--
> gdb/remote.c | 88 ++++++++++++++++++++++++++++----
> gdb/target-descriptions.c | 24 ++++++++-
> gdb/target-descriptions.h | 5 ++
> gdbserver/regcache.cc | 8 +++
> gdbserver/regcache.h | 2 +
> gdbserver/tdesc.cc | 12 ++++-
> gdbsupport/common-regcache.h | 3 ++
> gdbsupport/tdesc.cc | 9 ++--
> gdbsupport/tdesc.h | 11 +++-
> 14 files changed, 285 insertions(+), 30 deletions(-)
>
> diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
> index 9aa9d874ecbb..4d4ad3431d16 100644
> --- a/gdb/process-stratum-target.h
> +++ b/gdb/process-stratum-target.h
> @@ -55,6 +55,9 @@ class process_stratum_target : public target_ops
> gdbarch. */
> struct gdbarch *thread_architecture (ptid_t ptid) override;
>
> + /* Supply the load-early registers to REGCACHE when it's first created. */
> + virtual void supply_initial_registers (regcache *regcache) {};
Here we use the word "initial" but in other places mostly "early" or "load_early".
Can we use "early" here, too?
> +
> /* Default implementations for process_stratum targets. Return true
> if there's a selected inferior, false otherwise. */
> bool has_all_memory () override;
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 22a513ac79ab..8e4915abd083 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -926,15 +926,20 @@ record_full_core_open_1 ()
> {
> regcache *regcache = get_thread_regcache (inferior_thread ());
> int regnum = gdbarch_num_regs (regcache->arch ());
> - int i;
>
> /* Get record_full_core_regbuf. */
> target_fetch_registers (regcache, -1);
> record_full_core_regbuf = new detached_regcache (regcache->arch (), false);
>
> - for (i = 0; i < regnum; i ++)
> + /* Supply load-early registers first. */
> + for (int i : regcache->load_early_registers ())
> record_full_core_regbuf->raw_supply (i, *regcache);
>
> + for (int i = 0; i < regnum; i ++)
Extra space here------------------^
> + /* Load-early registers were already supplied. */
> + if (!regcache->is_load_early_register (i))
> + record_full_core_regbuf->raw_supply (i, *regcache);
I think this loop should have braces.
> +
> record_full_core_sections
> = build_section_table (current_program_space->core_bfd ());
>
> @@ -2107,10 +2112,15 @@ record_full_core_target::fetch_registers (struct regcache *regcache,
> if (regno < 0)
> {
> int num = gdbarch_num_regs (regcache->arch ());
> - int i;
>
> - for (i = 0; i < num; i ++)
> + /* Supply load-early registers first. */
> + for (int i : regcache->load_early_registers ())
> regcache->raw_supply (i, *record_full_core_regbuf);
> +
> + for (int i = 0; i < num; i ++)
> + /* Load-early registers were already supplied. */
> + if (!regcache->is_load_early_register (i))
> + regcache->raw_supply (i, *record_full_core_regbuf);
> }
> else
> regcache->raw_supply (regno, *record_full_core_regbuf);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 6e0c730d0d59..61289d46f4b1 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -30,6 +30,7 @@
> #include "regset.h"
> #include <unordered_map>
> #include "cli/cli-cmds.h"
> +#include "target-descriptions.h"
>
> /*
> * DATA STRUCTURE
> @@ -68,6 +69,9 @@ struct regcache_descr
> long *register_offset = nullptr;
> long *sizeof_register = nullptr;
>
> + /* Registers that need to be loaded early in the register cache. */
> + std::set<int> load_early_regs;
> +
> /* Cached table containing the type of each register. */
> struct type **register_type = nullptr;
> };
> @@ -120,6 +124,9 @@ init_regcache_descr (struct gdbarch *gdbarch)
> descr->sizeof_register[i] = descr->register_type[i]->length ();
> descr->register_offset[i] = offset;
> offset += descr->sizeof_register[i];
> +
> + if (tdesc_register_is_early_load (gdbarch, i))
> + descr->load_early_regs.insert (i);
> }
> /* Set the real size of the raw register cache buffer. */
> descr->sizeof_raw_registers = offset;
> @@ -230,6 +237,50 @@ reg_buffer::arch () const
> return m_descr->gdbarch;
> }
>
> +/* Utility functions returning useful register attributes stored in
> + the regcache descr. */
> +
> +/* See regcache.h. */
> +
> +bool
> +reg_buffer::fetch_load_early_registers ()
> +{
> + for (int regnum : this->load_early_registers ())
> + if (this->get_register_status (regnum) != REG_VALID)
> + /* A reg_buffer can't fetch registers, so we can only report failure. */
> + return false;
> +
> + return true;
> +}
> +
> +/* See regcache.h. */
> +
> +bool
> +regcache::fetch_load_early_registers ()
> +{
> + for (int regnum : this->load_early_registers ())
> + if (this->get_register_status (regnum) != REG_VALID)
> + target_fetch_registers (this, regnum);
> +
> + return true;
> +}
> +
> +/* See regcache.h. */
> +
> +bool
> +reg_buffer::has_load_early_registers ()
> +{
> + return !m_descr->load_early_regs.empty ();
> +}
> +
> +/* See regcache.h. */
> +
> +const std::set<int> &
> +reg_buffer::load_early_registers ()
> +{
> + return m_descr->load_early_regs;
> +}
> +
> /* Helper for reg_buffer::register_buffer. */
>
> template<typename ElemType>
> @@ -268,12 +319,31 @@ reg_buffer::save (register_read_ftype cooked_read)
> /* Clear the dest. */
> memset (m_registers.get (), 0, m_descr->sizeof_cooked_registers);
> memset (m_register_status.get (), REG_UNKNOWN, m_descr->nr_cooked_registers);
> +
> + /* Save load early registers first. */
> + for (int regnum : m_descr->load_early_regs)
> + {
> + gdb::array_view<gdb_byte> dst_buf = register_buffer (regnum);
> + register_status status = cooked_read (regnum, dst_buf);
> +
> + gdb_assert (status != REG_UNKNOWN);
> +
> + if (status != REG_VALID)
> + memset (dst_buf.data (), 0, dst_buf.size ());
> +
> + m_register_status[regnum] = status;
> + }
The body of the loop is the same below. It may make sense to extract it in
a helper method and reuse.
> +
> /* Copy over any registers (identified by their membership in the
> save_reggroup) and mark them as valid. The full [0 .. gdbarch_num_regs +
> gdbarch_num_pseudo_regs) range is checked since some architectures need
> to save/restore `cooked' registers that live in memory. */
> for (int regnum = 0; regnum < m_descr->nr_cooked_registers; regnum++)
> {
> + /* Load-early registers were already saved. */
> + if (this->is_load_early_register (regnum))
> + continue;
> +
> if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
> {
> gdb::array_view<gdb_byte> dst_buf = register_buffer (regnum);
> @@ -293,19 +363,34 @@ void
> regcache::restore (readonly_detached_regcache *src)
> {
> struct gdbarch *gdbarch = m_descr->gdbarch;
> - int regnum;
>
> gdb_assert (src != NULL);
> gdb_assert (src->m_has_pseudo);
>
> gdb_assert (gdbarch == src->arch ());
>
> + /* Restore load early registers first. */
If the length of a register depends on the value of another register,
I understand why the other must be loaded before. But when writing the
registers to the target, why does the order matter?
> + for (int regnum : m_descr->load_early_regs)
> + {
> + if (!gdbarch_register_reggroup_p (gdbarch, regnum, restore_reggroup))
> + continue;
In the 'save' method above, I would expect a symmetric check for
gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup)
but we didn't have it.
> +
> + if (src->m_register_status[regnum] != REG_VALID)
> + continue;
> +
> + cooked_write (regnum, src->register_buffer (regnum));
> + }
> +
> /* Copy over any registers, being careful to only restore those that
> were both saved and need to be restored. The full [0 .. gdbarch_num_regs
> + gdbarch_num_pseudo_regs) range is checked since some architectures need
> to save/restore `cooked' registers that live in memory. */
> - for (regnum = 0; regnum < m_descr->nr_cooked_registers; regnum++)
> + for (int regnum = 0; regnum < m_descr->nr_cooked_registers; regnum++)
> {
> + /* Load-early registers were already restored. */
> + if (this->is_load_early_register (regnum))
> + continue;
> +
> if (gdbarch_register_reggroup_p (gdbarch, regnum, restore_reggroup))
> {
> if (src->m_register_status[regnum] == REG_VALID)
> @@ -324,6 +409,14 @@ reg_buffer::get_register_status (int regnum) const
> return m_register_status[regnum];
> }
>
> +/* See gdbsupport/common-regcache.h. */
> +
> +bool
> +reg_buffer::is_load_early_register (int regnum) const
> +{
> + return m_descr->load_early_regs.count (regnum) > 0;
> +}
> +
> void
> reg_buffer::invalidate (int regnum)
> {
> @@ -394,6 +487,8 @@ get_thread_arch_regcache (inferior *inf_for_target_calls, ptid_t ptid,
> constructor explicitly instead of implicitly. */
> ptid_regc_map.insert (std::make_pair (ptid, regcache_up (new_regcache)));
>
> + proc_target->supply_initial_registers (new_regcache);
> +
> return new_regcache;
> }
>
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 739172a249b8..5c28fec4af11 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -20,6 +20,7 @@
> #ifndef REGCACHE_H
> #define REGCACHE_H
>
> +#include <set>
> #include "gdbsupport/array-view.h"
> #include "gdbsupport/common-regcache.h"
> #include "gdbsupport/function-view.h"
> @@ -198,6 +199,9 @@ class reg_buffer : public reg_buffer_common
> /* See gdbsupport/common-regcache.h. */
> enum register_status get_register_status (int regnum) const override;
>
> +/* See gdbsupport/common-regcache.h. */
> + bool is_load_early_register (int regnum) const override;
> +
> /* See gdbsupport/common-regcache.h. */
> void raw_collect (int regnum, gdb::array_view<gdb_byte> dst) const override;
>
> @@ -256,6 +260,12 @@ class reg_buffer : public reg_buffer_common
> /* See gdbsupport/common-regcache.h. */
> bool raw_compare (int regnum, const void *buf, int offset) const override;
>
> + /* Whether any register needs to be loaded before other registers. */
> + bool has_load_early_registers ();
> +
> + /* Return set of regnums which need to be loaded before other registers. */
> + const std::set<int> &load_early_registers ();
> +
> /* See gdbsupport/common-regcache.h. */
> int register_size (int regnum) const override;
>
> @@ -265,6 +275,10 @@ class reg_buffer : public reg_buffer_common
>
> int num_raw_registers () const;
>
> + /* Ensure all load early registers are fetched in this cache.
> + Return false if they aren't. */
> + virtual bool fetch_load_early_registers ();
> +
> /* Return a view on register REGNUM's buffer cache. */
> template <typename ElemType>
> gdb::array_view<ElemType> register_buffer (int regnum) const;
> @@ -485,6 +499,9 @@ class regcache : public detached_regcache
> gdb::array_view<const gdb_byte> src,
> bool is_raw);
>
> + /* See class reg_buffer. */
> + bool fetch_load_early_registers () override;
> +
> /* The inferior to switch to, to make target calls.
>
> This may not be the inferior of thread M_PTID. For instance, this
> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
> index 0ba7a08bb0c5..09339d495209 100644
> --- a/gdb/regformats/regdef.h
> +++ b/gdb/regformats/regdef.h
> @@ -26,13 +26,15 @@ struct reg
> reg (int _offset)
> : name (""),
> offset (_offset),
> - size (0)
> + size (0),
> + load_early (false)
> {}
>
> - reg (const char *_name, int _offset, int _size)
> + reg (const char *_name, int _offset, int _size, bool _load_early)
> : name (_name),
> offset (_offset),
> - size (_size)
> + size (_size),
> + load_early (_load_early)
> {}
>
> /* The name of this register - NULL for pad entries. */
> @@ -49,11 +51,17 @@ struct reg
> /* The size (in bits) of the value of this register, as transmitted. */
> int size;
>
> + /* Whether this register needs to be loaded early in the register cache,
> + because variable-size registers depend on it to calculate their
> + size. */
> + bool load_early;
> +
> bool operator== (const reg &other) const
> {
> return (strcmp (name, other.name) == 0
> && offset == other.offset
> - && size == other.size);
> + && size == other.size
> + && load_early == other.load_early);
> }
>
> bool operator!= (const reg &other) const
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2da2c5a4789a..0217c05bce52 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -683,6 +683,9 @@ class remote_state
> immediately, so queue is not needed for them. */
> std::vector<stop_reply_up> stop_reply_queue;
>
> + /* Contains the stop reply packet when first starting the inferior. */
> + gdb::char_vector first_stop_reply;
> +
> /* FIXME: cagney/1999-09-23: Even though getpkt was called with
> ``forever'' still use the normal timeout mechanism. This is
> currently used by the ASYNC code to guarantee that target reads
> @@ -1218,6 +1221,8 @@ class remote_target : public process_stratum_target
> ptid_t select_thread_for_ambiguous_stop_reply
> (const struct target_waitstatus &status);
>
> + void supply_initial_registers (regcache *regcache) override;
> +
> void remote_notice_new_inferior (ptid_t currthread, bool executing);
>
> void print_one_stopped_thread (thread_info *thread);
> @@ -1317,6 +1322,8 @@ class remote_target : public process_stratum_target
>
> int fetch_register_using_p (struct regcache *regcache,
> packet_reg *reg);
> +
> + void fetch_early_registers (struct regcache *regcache);
> int send_g_packet ();
> void process_g_packet (struct regcache *regcache);
> void fetch_registers_using_g (struct regcache *regcache);
> @@ -1417,6 +1424,9 @@ class remote_target : public process_stratum_target
>
> bool start_remote_1 (int from_tty, int extended_p);
>
> + void supply_expedited_regs (struct regcache *regcache,
> + std::vector<cached_reg_t> &expedited_regs);
> +
> /* The remote state. Don't reference this directly. Use the
> get_remote_state method instead. */
> remote_state m_remote_state;
> @@ -8530,6 +8540,21 @@ remote_target::select_thread_for_ambiguous_stop_reply
> return first_resumed_thread->ptid;
> }
>
> +/* Supply the contents of EXPEDITED_REGS to REGCACHE. */
> +
> +void
> +remote_target::supply_expedited_regs (struct regcache *regcache,
> + std::vector<cached_reg_t> &expedited_regs)
> +{
> + remote_state *rs = get_remote_state ();
> +
> + for (cached_reg_t ® : expedited_regs)
> + {
> + regcache->raw_supply (reg.num, reg.data);
> + rs->last_seen_expedited_registers.insert (reg.num);
> + }
> +}
> +
> /* Called when it is decided that STOP_REPLY holds the info of the
> event that is to be returned to the core. This function always
> destroys STOP_REPLY. */
> @@ -8568,12 +8593,7 @@ remote_target::process_stop_reply (stop_reply_up stop_reply,
> regcache *regcache
> = get_thread_arch_regcache (find_inferior_ptid (this, ptid), ptid,
> stop_reply->arch);
> -
> - for (cached_reg_t ® : stop_reply->regcache)
> - {
> - regcache->raw_supply (reg.num, reg.data);
> - rs->last_seen_expedited_registers.insert (reg.num);
> - }
> + supply_expedited_regs (regcache, stop_reply->regcache);
> }
>
> remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
> @@ -8599,6 +8619,28 @@ remote_target::process_stop_reply (stop_reply_up stop_reply,
> return ptid;
> }
>
> +/* See gdb/process-stratum-target.h. */
> +
> +void
> +remote_target::supply_initial_registers (regcache *regcache)
> +{
> + remote_state *rs = get_remote_state ();
> +
> + if (rs->first_stop_reply.empty ())
> + return;
> +
> + notif_event_up reply
> + = remote_notif_parse (this, ¬if_client_stop,
> + rs->first_stop_reply.data ());
> + std::vector<cached_reg_t> &expedited_regs
> + = ((struct stop_reply *) reply.get ())->regcache;
> +
> + if (!expedited_regs.empty ())
> + supply_expedited_regs (regcache, expedited_regs);
> +
> + rs->first_stop_reply.clear ();
> +}
> +
> /* The non-stop mode version of target_wait. */
>
> ptid_t
> @@ -8918,6 +8960,27 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
> return 1;
> }
>
> +/* Fetch load-early registers individually with the 'p' packet. */
> +
> +void
> +remote_target::fetch_early_registers (struct regcache *regcache)
> +{
> + struct remote_state *rs = get_remote_state ();
> + remote_arch_state *rsa = rs->get_remote_arch_state (regcache->arch ());
> +
> + for (int regnum : regcache->load_early_registers ())
> + {
> + /* We may already have it from a stop reply packet. */
> + if (regcache->get_register_status (regnum) == REG_VALID)
> + continue;
> +
> + packet_reg *reg = packet_reg_from_regnum (regcache->arch (), rsa, regnum);
> + int res = fetch_register_using_p (regcache, reg);
> + if (res == 0)
> + error (_("Could not load early register %d using p packet."), regnum);
> + }
> +}
> +
> /* Fetch the registers included in the target's 'g' packet. */
>
> int
> @@ -9062,6 +9125,9 @@ remote_target::process_g_packet (struct regcache *regcache)
> void
> remote_target::fetch_registers_using_g (struct regcache *regcache)
> {
> + if (regcache->has_load_early_registers ())
> + fetch_early_registers (regcache);
> +
> send_g_packet ();
> process_g_packet (regcache);
> }
> @@ -10920,7 +10986,6 @@ extended_remote_target::create_inferior (const char *exec_file,
> char **env, int from_tty)
> {
> int run_worked;
> - char *stop_reply;
> struct remote_state *rs = get_remote_state ();
> const char *remote_exec_file = get_remote_exec_file ();
>
> @@ -10953,7 +11018,10 @@ Remote replied unexpectedly while setting startup-with-shell: %s"),
>
> /* Now restart the remote server. */
> run_worked = extended_remote_run (args) != -1;
> - if (!run_worked)
> + if (run_worked)
> + /* vRun's success return is a stop reply. */
> + rs->first_stop_reply = rs->buf;
> + else
> {
> /* vRun was not supported. Fail if we need it to do what the
> user requested. */
> @@ -10966,9 +11034,7 @@ Remote replied unexpectedly while setting startup-with-shell: %s"),
> extended_remote_restart ();
> }
>
> - /* vRun's success return is a stop reply. */
> - stop_reply = run_worked ? rs->buf.data () : NULL;
> - add_current_inferior_and_thread (stop_reply);
> + add_current_inferior_and_thread (run_worked ? rs->buf.data () : nullptr);
>
> /* Get updated offsets, if the stub uses qOffsets. */
> get_offsets ();
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 1bd22c273a29..82e4d96276e3 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -850,6 +850,16 @@ tdesc_register_name (struct gdbarch *gdbarch, int regno)
> return "";
> }
>
> +/* See target-descriptions.h. */
> +
> +bool
> +tdesc_register_is_early_load (struct gdbarch *gdbarch, int regno)
> +{
> + struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno);
> +
> + return reg == nullptr ? false : reg->load_early;
I wonder if it wouldn't be better to assert that reg != nullptr.
Is there a case where tdesc_find_register returns null?
> +}
> +
> struct type *
> tdesc_register_type (struct gdbarch *gdbarch, int regno)
> {
> @@ -1484,7 +1494,12 @@ class print_c_tdesc : public tdesc_element_visitor
> gdb_printf ("\"%s\", ", reg->group.c_str ());
> else
> gdb_printf ("NULL, ");
> - gdb_printf ("%d, \"%s\");\n", reg->bitsize, reg->type.c_str ());
> + gdb_printf ("%d, \"%s\"", reg->bitsize, reg->type.c_str ());
> +
> + if (reg->load_early)
> + gdb_printf (", true");
> +
> + gdb_printf (");\n");
> }
>
> protected:
> @@ -1627,7 +1642,12 @@ class print_c_feature : public print_c_tdesc
> gdb_printf ("\"%s\", ", reg->group.c_str ());
> else
> gdb_printf ("NULL, ");
> - gdb_printf ("%d, \"%s\");\n", reg->bitsize, reg->type.c_str ());
> + gdb_printf ("%d, \"%s\"", reg->bitsize, reg->type.c_str ());
> +
> + if (reg->load_early)
> + gdb_printf (", true");
> +
> + gdb_printf (");\n");
>
> m_next_regnum++;
> }
> diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
> index dc83db0acf28..e40c7db5f79f 100644
> --- a/gdb/target-descriptions.h
> +++ b/gdb/target-descriptions.h
> @@ -199,6 +199,11 @@ const char *tdesc_feature_name (const struct tdesc_feature *feature);
>
> const char *tdesc_register_name (struct gdbarch *gdbarch, int regno);
>
> +/* Return whether register REGNO needs to be loaded early in the
> + register cache. */
> +
> +bool tdesc_register_is_early_load (struct gdbarch *gdbarch, int regno);
> +
> /* Return the type of register REGNO, from the target description or
> from an architecture-provided pseudo_register_type method. */
>
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index ec19864bd690..cd1ee2e5f145 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -568,6 +568,14 @@ regcache::get_register_status (int regnum) const
>
> /* See gdbsupport/common-regcache.h. */
>
> +bool
> +regcache::is_load_early_register (int regnum) const
> +{
> + return find_register_by_number (this->tdesc, regnum).load_early;
> +}
> +
> +/* See gdbsupport/common-regcache.h. */
> +
> bool
> regcache::raw_compare (int regnum, const void *buf, int offset) const
> {
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 5de8fd3d127e..07e48a7432b7 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -49,6 +49,8 @@ struct regcache : public reg_buffer_common
> /* See gdbsupport/common-regcache.h. */
> enum register_status get_register_status (int regnum) const override;
>
> + bool is_load_early_register (int regnum) const override;
> +
> /* See gdbsupport/common-regcache.h. */
> int register_size (int regnum) const override;
>
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index d052f43c76e6..6f7ebb7c5c76 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -56,6 +56,8 @@ init_target_desc (struct target_desc *tdesc,
> const char **expedite_regs)
> {
> int offset = 0;
> + /* Additional registers to expedite from the features. */
> + std::vector<const char *> expedite_from_features;
>
> /* Go through all the features and populate reg_defs. */
> for (const tdesc_feature_up &feature : tdesc->features)
> @@ -70,8 +72,11 @@ init_target_desc (struct target_desc *tdesc,
> tdesc->reg_defs.resize (regnum, gdb::reg (offset));
>
> tdesc->reg_defs.emplace_back (treg->name.c_str (), offset,
> - treg->bitsize);
> + treg->bitsize, treg->load_early);
> offset += treg->bitsize;
> +
> + if (treg->load_early)
> + expedite_from_features.push_back (treg->name.c_str ());
> }
>
> tdesc->registers_size = offset / 8;
> @@ -88,6 +93,11 @@ init_target_desc (struct target_desc *tdesc,
> int expedite_count = 0;
> while (expedite_regs[expedite_count] != nullptr)
> tdesc->expedite_regs.push_back (expedite_regs[expedite_count++]);
> +
> + if (!expedite_from_features.empty ())
We can simply remove the check and we would be trivially inserting
the (non-existing) elements of an empty set.
> + tdesc->expedite_regs.insert (tdesc->expedite_regs.end (),
> + expedite_from_features.cbegin (),
> + expedite_from_features.cend ());
Do we check anywhere that we are not adding a register twice,
once from the expedite regs as specified by the target, and once
again from this expedite_from_features?
> #endif
> }
>
> diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
> index 4594999346fd..93b1b5d522f2 100644
> --- a/gdbsupport/common-regcache.h
> +++ b/gdbsupport/common-regcache.h
> @@ -73,6 +73,9 @@ struct reg_buffer_common
> buffer. */
> virtual register_status get_register_status (int regnum) const = 0;
>
> + /* Does this register need to be loaded before others? */
> + virtual bool is_load_early_register (int regnum) const = 0;
> +
> /* Return the size of register numbered REGNUM in this buffer. */
> virtual int register_size (int regnum) const = 0;
>
> diff --git a/gdbsupport/tdesc.cc b/gdbsupport/tdesc.cc
> index 080d39c485dc..a99119274f44 100644
> --- a/gdbsupport/tdesc.cc
> +++ b/gdbsupport/tdesc.cc
> @@ -21,12 +21,13 @@
>
> tdesc_reg::tdesc_reg (struct tdesc_feature *feature, const std::string &name_,
> int regnum, int save_restore_, const char *group_,
> - int bitsize_, const char *type_)
> + int bitsize_, const char *type_, bool load_early_)
> : name (name_), target_regnum (regnum),
> save_restore (save_restore_),
> group (group_ != NULL ? group_ : ""),
> bitsize (bitsize_),
> - type (type_ != NULL ? type_ : "<unknown>")
> + type (type_ != NULL ? type_ : "<unknown>"),
> + load_early (load_early_)
> {
> /* If the register's type is target-defined, look it up now. We may not
> have easy access to the containing feature when we want it later. */
> @@ -137,10 +138,10 @@ tdesc_named_type (const struct tdesc_feature *feature, const char *id)
> void
> tdesc_create_reg (struct tdesc_feature *feature, const char *name,
> int regnum, int save_restore, const char *group,
> - int bitsize, const char *type)
> + int bitsize, const char *type, bool load_early)
> {
> tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,
> - group, bitsize, type);
> + group, bitsize, type, load_early);
>
> feature->registers.emplace_back (reg);
> }
> diff --git a/gdbsupport/tdesc.h b/gdbsupport/tdesc.h
> index c9e7603369cb..7e483486139b 100644
> --- a/gdbsupport/tdesc.h
> +++ b/gdbsupport/tdesc.h
> @@ -70,7 +70,7 @@ struct tdesc_reg : tdesc_element
> {
> tdesc_reg (struct tdesc_feature *feature, const std::string &name_,
> int regnum, int save_restore_, const char *group_,
> - int bitsize_, const char *type_);
> + int bitsize_, const char *type_, bool load_early_ = false);
>
> virtual ~tdesc_reg () = default;
>
> @@ -110,6 +110,12 @@ struct tdesc_reg : tdesc_element
> /* The target-described type corresponding to TYPE, if found. */
> struct tdesc_type *tdesc_type;
>
> + /* Whether this register needs to be loaded early in GDB's regcache.
> +
> + In addition, if true gdbserver will send it as an expedited register
> + in stop replies. */
> + bool load_early;
> +
> void accept (tdesc_element_visitor &v) const override
> {
> v.visit (this);
> @@ -415,7 +421,8 @@ void tdesc_add_enum_value (tdesc_type_with_fields *type, int value,
> /* Create a register in feature FEATURE. */
> void tdesc_create_reg (struct tdesc_feature *feature, const char *name,
> int regnum, int save_restore, const char *group,
> - int bitsize, const char *type);
> + int bitsize, const char *type,
> + bool load_early = false);
>
> /* Return the tdesc in string XML format. */
>
Thanks
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
More information about the Gdb-patches
mailing list