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 F2323388461C for ; Fri, 28 Jun 2024 14:49:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F2323388461C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org F2323388461C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719586186; cv=none; b=DG+EwTgblp1wmorjpo1ddYD5apU3LbMmdY/QeS+JJMwUEU7GuETsGoMunma7yOcppc6pow23n0ElDTrzTvC5XYZ/aLVc/9AOeIenP042+OMeaXpNhy14AUArS3/ITglTfjr+1bQ17gnYznE8X3odP2ASs860x2wRkph7v5KxO6A= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719586186; c=relaxed/simple; bh=HAUrAl4UwlLRQicClWUq4VGaRfFKJ5UxnxPO3JPj77I=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=uJ7NXAksEyVVz7LIBf8gC3DiexueufrCw0J7/cC/4jFbpElVSHuqt2/rg9bS207HJjCaoBORch4sp50cGl6PqfWWofOXAbSXaFEvXEo7JmEYxWW5YdoLrWW/SE6nXEYR/EQJ6euZ3B0/pPJajpnu3/we9eazjGduAiAjl5MEFg4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1719586182; 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=iN7dlOe6OsMgDm9goW+8on4R3CMdiM4KPfDfxfS7F7o=; b=Zc7RYGV8Gdp8YIArj4iwI2pGkQwCHr3CdcLFTaAk+p7jYlQSCzVGX8hZgB5eX6XKELK5vB /+RgYcll+bowrAEOwjearYnxOMf0U9rtsld7WVEYpFLsoRFTms1SosE3Z52Fki2U4mXJ7o F/42N2H1RWenk2u7DZRfEfVDuy2j12I= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-522-rJwJLbl0OtesHIfMUnXAGQ-1; Fri, 28 Jun 2024 10:49:41 -0400 X-MC-Unique: rJwJLbl0OtesHIfMUnXAGQ-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4256809ae27so5453505e9.1 for ; Fri, 28 Jun 2024 07:49:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719586180; x=1720190980; 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=iN7dlOe6OsMgDm9goW+8on4R3CMdiM4KPfDfxfS7F7o=; b=bl8cpaTAWubHGMNx5cEi7InCMbKC9Ni4wSOMu1ct8RaAX5JYcHlfRI1n4Jm0YF3DuF UJcAFR7ED+d/r210HrlrLUsXOtptrc9/kvtfGuTn6VptkUnle3U4tV16Ypfvv696DHFy JU0yzly9m1TWPleiLkgtdgS6t9xAiFovTOTumbe+O6arsmd3tiaU23kgGjnB+6YMeNHi SuKx8+DAAB6/Eh7805lBHh2wfe7VRChrTMWYYMSl4oGTJuqRGWt4aJUc+67n8v+b98zA TNFPCjInNoxaHFw9GHrqkV934dViNL5GSu19H51vOK6A2t725V2mc0DOKPJbh7Fdt3eL M1ag== X-Forwarded-Encrypted: i=1; AJvYcCVwrsaB4LFrIzNT1gFdqYe5ZAiLdWGslIclA6bCXAWbcaZZzmh9cwqFSYnw7+flkHebI3psWSSnTFhRm3hYHUcvMIsiz7IlzIE0JA== X-Gm-Message-State: AOJu0Yya0/FF3VkqxnjS0Jo2io+0x63baG4Ohx94obliUc7ebTw/AyK9 v0HiMKVduIbycwNynD/XTa5NvodC0kNYJI64CLviPmqXxFK5ZHDAZ2fc8Pv/b0P0JDUjMQI9HBH nyQB8cpoT1/5k2cbb3ZlTJAthNPihZOWE+XwW+sWEDAU3XSXqFRjKu6mcASk= X-Received: by 2002:a5d:660a:0:b0:364:e290:c60b with SMTP id ffacd0b85a97d-366e94cf384mr10985058f8f.38.1719586179750; Fri, 28 Jun 2024 07:49:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFeIw4W1rCg4k0TLiiBubAH+jeL3jDfscODFKWNVzqFomFJmN+pzZi+2btFD8gsKk72GIiBVg== X-Received: by 2002:a5d:660a:0:b0:364:e290:c60b with SMTP id ffacd0b85a97d-366e94cf384mr10985032f8f.38.1719586178973; Fri, 28 Jun 2024 07:49:38 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3675a0cd56dsm2525597f8f.19.2024.06.28.07.49.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Jun 2024 07:49:38 -0700 (PDT) From: Andrew Burgess To: Guinevere Larsen , gdb-patches@sourceware.org Cc: Guinevere Larsen Subject: Re: [PATCH] gdb: reintroduce reg_buffer::m_readonly in place of m_has_pseudo In-Reply-To: <20240621180730.859487-1-blarsen@redhat.com> References: <20240621180730.859487-1-blarsen@redhat.com> Date: Fri, 28 Jun 2024 15:49:37 +0100 Message-ID: <874j9d86m6.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: Guinevere Larsen writes: > A series of commits obfuscated the meaning of reg_buffer:m_has_pseudo, > most notably for the current code being commit > daf6667d1f94c7e74df4076daf021cd28a2, that changed the checks in > regcache::save and regcache::restore from checking if the cache was > readonly to checking if the cache had registers. > > There was no reason provided for that change, but following commits > started to use a parameters called "readonly" to set m_has_pseudo, until > a following commit only used the literal "true" and "false" values, but > still mapping perfectly on whether the register cache will be read only, > and not taking the gdbarch's pseudo registers or specific needs of a > cache's location into account. > > The current usages in regcache::save and ::restore were just changed to > use m_readonly, making the code more understandable. Two other uses, in > readable_regcache::cooked_read and ::cooked_read_value, have just been > removed, and the final 2 uses, in reg_buffer's constructor and in > reg_buffer::assert_regnum, were changed to always act as if the buffer > has pseudo registers. This last bit confuses me. A read/write reg cache can't really hold the pseudo registers I think. If we wrote to a raw-register and then read a pseudo that is made (in part) from the raw register value, we'd miss the updated value, wouldn't we? Or maybe I'm missing something... I'm not sure moving to a situation where we carry around some unused space, and just rely on GDB not filling it in is not the best situation. Can you explain a little more what the newly allocated space is expected to be used for, and why it's safe? Thanks, Andrew > > I chose to make reg_buffer always allocate space for pseudo-registers > because the cooker_read methods act as if it was possible to cache a > pseudo register's value, but as far as I could see, there wasn't any > code doing it. Since it would be done in some tdep file and I didn't > feel confident in saying no code at all does this caching, and thought > it would be safer to allocate space for it. > > Tested on x86_64 Fedora 40 > --- > gdb/regcache-dump.c | 18 +++++++----------- > gdb/regcache.c | 35 ++++++++++++----------------------- > gdb/regcache.h | 19 +++++++++++-------- > 3 files changed, 30 insertions(+), 42 deletions(-) > > diff --git a/gdb/regcache-dump.c b/gdb/regcache-dump.c > index bc665dc08a6..6a6633e49dd 100644 > --- a/gdb/regcache-dump.c > +++ b/gdb/regcache-dump.c > @@ -32,8 +32,7 @@ class register_dump_regcache : public register_dump > { > public: > register_dump_regcache (regcache *regcache, bool dump_pseudo) > - : register_dump (regcache->arch ()), m_regcache (regcache), > - m_dump_pseudo (dump_pseudo) > + : register_dump (regcache->arch (), dump_pseudo), m_regcache (regcache) > { > } > > @@ -80,9 +79,6 @@ class register_dump_regcache : public register_dump > > private: > regcache *m_regcache; > - > - /* Dump pseudo registers or not. */ > - const bool m_dump_pseudo; > }; > > /* Dump from reg_buffer, used when there is no thread or > @@ -92,7 +88,7 @@ class register_dump_reg_buffer : public register_dump, reg_buffer > { > public: > register_dump_reg_buffer (gdbarch *gdbarch, bool dump_pseudo) > - : register_dump (gdbarch), reg_buffer (gdbarch, dump_pseudo) > + : register_dump (gdbarch, dump_pseudo), reg_buffer (gdbarch, true) > { > } > > @@ -101,14 +97,14 @@ class register_dump_reg_buffer : public register_dump, reg_buffer > { > if (regnum < 0) > { > - if (m_has_pseudo) > + if (m_dump_pseudo) > gdb_printf (file, "Cooked value"); > else > gdb_printf (file, "Raw value"); > } > else > { > - if (regnum < gdbarch_num_regs (m_gdbarch) || m_has_pseudo) > + if (regnum < gdbarch_num_regs (m_gdbarch) || m_dump_pseudo) > { > auto size = register_size (m_gdbarch, regnum); > > @@ -140,7 +136,7 @@ class register_dump_none : public register_dump > { > public: > register_dump_none (gdbarch *arch) > - : register_dump (arch) > + : register_dump (arch, false) > {} > > protected: > @@ -154,7 +150,7 @@ class register_dump_remote : public register_dump > { > public: > register_dump_remote (gdbarch *arch) > - : register_dump (arch) > + : register_dump (arch, false) > {} > > protected: > @@ -181,7 +177,7 @@ class register_dump_groups : public register_dump > { > public: > register_dump_groups (gdbarch *arch) > - : register_dump (arch) > + : register_dump (arch, true) > {} > > protected: > diff --git a/gdb/regcache.c b/gdb/regcache.c > index f04354d822f..135238e5be3 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -185,8 +185,8 @@ regcache_register_size (const reg_buffer_common *regcache, int n) > (gdb::checked_static_cast (regcache)->arch (), n); > } > > -reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo) > - : m_has_pseudo (has_pseudo) > +reg_buffer::reg_buffer (gdbarch *gdbarch, bool readonly) > + : m_readonly (readonly) > { > gdb_assert (gdbarch != NULL); > m_descr = regcache_descr (gdbarch); > @@ -194,18 +194,9 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo) > /* We don't zero-initialize the M_REGISTERS array, as the bytes it contains > aren't meaningful as long as the corresponding register status is not > REG_VALID. */ > - if (has_pseudo) > - { > - m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]); > - m_register_status.reset > - (new register_status[m_descr->nr_cooked_registers] ()); > - } > - else > - { > - m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers]); > - m_register_status.reset > - (new register_status[gdbarch_num_regs (gdbarch)] ()); > - } > + m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]); > + m_register_status.reset > + (new register_status[m_descr->nr_cooked_registers] ()); > } > > regcache::regcache (inferior *inf_for_target_calls, gdbarch *gdbarch) > @@ -264,8 +255,9 @@ reg_buffer::save (register_read_ftype cooked_read) > { > struct gdbarch *gdbarch = m_descr->gdbarch; > > - /* It should have pseudo registers. */ > - gdb_assert (m_has_pseudo); > + /* This method should only be called when we're creating a > + readonly cache. */ > + gdb_assert (m_readonly); > /* 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); > @@ -297,7 +289,7 @@ regcache::restore (readonly_detached_regcache *src) > int regnum; > > gdb_assert (src != NULL); > - gdb_assert (src->m_has_pseudo); > + gdb_assert (src->m_readonly); > > gdb_assert (gdbarch == src->arch ()); > > @@ -336,10 +328,7 @@ void > reg_buffer::assert_regnum (int regnum) const > { > gdb_assert (regnum >= 0); > - if (m_has_pseudo) > - gdb_assert (regnum < m_descr->nr_cooked_registers); > - else > - gdb_assert (regnum < gdbarch_num_regs (arch ())); > + gdb_assert (regnum < m_descr->nr_cooked_registers); > } > > /* Type to map a ptid to a list of regcaches (one thread may have multiple > @@ -723,7 +712,7 @@ readable_regcache::cooked_read (int regnum, gdb::array_view dst) > > gdb_assert (dst.size () == m_descr->sizeof_register[regnum]); > > - if (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN) > + if (m_register_status[regnum] != REG_UNKNOWN) > { > if (m_register_status[regnum] == REG_VALID) > copy (register_buffer (regnum), dst); > @@ -774,7 +763,7 @@ readable_regcache::cooked_read_value (int regnum) > gdb_assert (regnum < m_descr->nr_cooked_registers); > > if (regnum < num_raw_registers () > - || (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN) > + || (m_register_status[regnum] != REG_UNKNOWN) > || !gdbarch_pseudo_register_read_value_p (m_descr->gdbarch)) > { > value *result = value::allocate_register > diff --git a/gdb/regcache.h b/gdb/regcache.h > index 2f4b7d94c69..3ecf07020be 100644 > --- a/gdb/regcache.h > +++ b/gdb/regcache.h > @@ -188,7 +188,7 @@ struct cached_reg_t > class reg_buffer : public reg_buffer_common > { > public: > - reg_buffer (gdbarch *gdbarch, bool has_pseudo); > + reg_buffer (gdbarch *gdbarch, bool readonly); > > DISABLE_COPY_AND_ASSIGN (reg_buffer); > > @@ -275,7 +275,7 @@ class reg_buffer : public reg_buffer_common > > struct regcache_descr *m_descr; > > - bool m_has_pseudo; > + bool m_readonly; > /* The register buffers. */ > std::unique_ptr m_registers; > /* Register cache status. */ > @@ -290,8 +290,8 @@ class reg_buffer : public reg_buffer_common > class readable_regcache : public reg_buffer > { > public: > - readable_regcache (gdbarch *gdbarch, bool has_pseudo) > - : reg_buffer (gdbarch, has_pseudo) > + readable_regcache (gdbarch *gdbarch, bool readonly) > + : reg_buffer (gdbarch, readonly) > {} > > /* Transfer a raw register [0..NUM_REGS) from core-gdb to this regcache, > @@ -349,8 +349,8 @@ class readable_regcache : public reg_buffer > class detached_regcache : public readable_regcache > { > public: > - detached_regcache (gdbarch *gdbarch, bool has_pseudo) > - : readable_regcache (gdbarch, has_pseudo) > + detached_regcache (gdbarch *gdbarch, bool readonly) > + : readable_regcache (gdbarch, readonly) > {} > > void raw_update (int regnum) override > @@ -537,8 +537,8 @@ class register_dump > virtual ~register_dump () = default; > > protected: > - register_dump (gdbarch *arch) > - : m_gdbarch (arch) > + register_dump (gdbarch *arch, bool dump_pseudo) > + : m_gdbarch (arch), m_dump_pseudo (dump_pseudo) > {} > > /* Dump the register REGNUM contents. If REGNUM is -1, print the > @@ -546,6 +546,9 @@ class register_dump > virtual void dump_reg (ui_file *file, int regnum) = 0; > > gdbarch *m_gdbarch; > + > + /* Dump pseudo registers or not. */ > + const bool m_dump_pseudo; > }; > > #endif /* REGCACHE_H */ > -- > 2.45.2