From: Lancelot SIX <lsix@lancelotsix.com>
To: gdb-patches@sourceware.org
Cc: Lancelot SIX <lsix@lancelotsix.com>, Pedro Alves <pedro@palves.net>
Subject: [PATCH v4 0/4] Refactor cmd_list_element.var
Date: Wed, 29 Sep 2021 22:50:07 +0100 [thread overview]
Message-ID: <20210929215011.1489639-1-lsix@lancelotsix.com> (raw)
Hi,
This is a V4 following
https://sourceware.org/pipermail/gdb-patches/2021-September/182006.html
This version addresses the comments from Pedro. It has also been rebased
on top of current trunk.
The entire series have been tested on x86_64-linux-gnu without regression
observed.
All feedbacks welcome.
Lancelot.
---
Noticable changes since V3:
- Patch 1:
- setting::get now returns by const reference instead of value.
- setting::set has a parameter passed by const ref instead of value.
- Since the setting value is returned by ref, remove the setting::get_p
function which returned the value by address. The (unique) caller of
this function can get the address from the reference instead.
- A setting can be costructed from a pre type-erased and pre validated
setting::erased_arg value. This is mainly used to avoid turning the
best part of add_setshow_cmd_full into a templated function.
- Use '= delete' to explicitly disable var_type_uses<T> for any T which
is not used to store a setting's value. This improves the compiler
error message when such overload is used accidently.
- Patch 2:
- Use the fact that setting::get returns by reference and avoids making
few un necessary copies of strings.
- Patch 3:
- Follow the changes introduced in patch 1 and allow construction of a
setting::erased_arg usign callback functions.
- Patch 4: Unchanged.
Noticeable changes since V2:
- Patch 1:
- cmd_list_element.var is now a gdb::optional<setting>, and the
setting ctor ensures a setting cannot exist if it does not contain a
value.
- Instead of storing a setting with param_smob (scheme) or
parmpy_object (python), keep the var_types and value as they are and
create a setting wrapper around them only when required.
- Patch 2 is unchanged.
- Patch 3:
- Store callback function pointers with a type erased 'void (*) ()'
instead of using a union. This requires significantly less code.
- Add a STORAGE property that can tell the caller if the setting
accesses its value via pointer or using the callback methods.
This is used in show_remote_protocol_packet_cmd in order to ensure
that accessing the setting by pointer is valid.
- Patch 4 kept unchanged except for conflict resolutions after rebasing
on top of current trunk.
Noticeable changes since V1:
- Based on the feedbacks form Tom Tromey, the API have been simplified and
the amount of templated code have been reduced. Instead of using
something like
setting.get<var_integer, var_zinetger, var_zuinteger_unlimited> ()
in order to access the value of a setting backed by an int (which must
be either a var_integer, a var_zinetger or a var_zuinteger_unlimited),
we now write
setting.get<int> ()
The 'get' function still includes an assert that setting.var_type is one
of the var_types that is backed by an int. In this version, the call
site needs to know the mapping between var_types and underlying storage
type.
- Based on a comment from Simon Marchi, the name of the type that ties
together the var_type and the void * pointer has changed from 'param' to
'setting'.
- Minor formatting issues have been addressed.
---
This patch series proposes to refactor the way cmd_list_element.var is
handled. It is meant to be a cleanup series and is intended to offer a
basis on which to solve PR/28085.
Some commands contain a reference to a variable (setting) that can be set
or shown. To do that, the command element contains pieces of information:
a void* pointer to global variable, and a var_types field to indicate the
type of the setting. The var_type dictates what the data type of variable
pointed by the void* pointer must be.
When any piece of code within GDB tries to interact with such variable, it
first checks the type field and then casts the void* pointer into whatever
type pointer it needs to and dereferences it. There are few limitations
with this approach I try to address in this patch series:
- Any code that interact with a cmd_list_element.var is responsible to
do some cast to the appropriate type. This is done in multiple places
in the codebase and is quite easy to get wrong. The first patch tries
to cover that by introducing a safer abstraction around the void *
pointer. The accessors that hide the void * pointer adds runtime
checks that the pointer is casted to the appropriate type that matches
the var_type of the setting instance.
- The source of truth for any setting needs to be the variable pointed to.
However, as pointed out by Simon in PR/28085 this is not always true, and
there are bugs linked to this. In order to solve this problem, patch 3
allows getter and setter callbacks to be embedded in the abstraction
introduced in patch 1. When the setting is accessed, the callback is
called to compute or update the value based on the current state of GDB.
Patch 2, which was originally proposed by Simon Marchi[1] proposes to
change the variable type from char* to std::string for string like
setting. Finally Patch 4 does some more refactoring on how we detect
and notify observers when a given setting is updated.
None of those patches should introduce user visible changes.
The entire series have been tested on x86_64 and no regression have been
observed on the testsuite.
All feedbacks are welcome.
Lancelot SIX (3):
gdb: Introduce setting construct within cmd_list_element
gdb: Have setter and getter callbacks for settings
gdb: Setting setter return a bool to tell if the value changed
Simon Marchi (1):
gdb: make string-like set show commands use std::string variable
gdb/auto-load.c | 50 ++--
gdb/breakpoint.c | 22 +-
gdb/build-id.c | 4 +-
gdb/cli/cli-cmds.c | 148 ++++++-----
gdb/cli/cli-decode.c | 459 +++++++++++++++++++++++++++++------
gdb/cli/cli-decode.h | 9 +-
gdb/cli/cli-logging.c | 23 +-
gdb/cli/cli-option.c | 9 +-
gdb/cli/cli-option.h | 4 +-
gdb/cli/cli-setshow.c | 188 ++++++--------
gdb/cli/cli-setshow.h | 4 +-
gdb/command.h | 335 ++++++++++++++++++++++++-
gdb/compile/compile.c | 46 ++--
gdb/corefile.c | 17 +-
gdb/defs.h | 4 +-
gdb/disasm.c | 11 +-
gdb/disasm.h | 2 +-
gdb/dwarf2/dwz.c | 2 +-
gdb/dwarf2/index-cache.c | 10 +-
gdb/dwarf2/read.c | 10 +-
gdb/event-top.c | 12 +-
gdb/fork-child.c | 7 +-
gdb/guile/scm-param.c | 175 +++++++------
gdb/infcmd.c | 14 +-
gdb/linux-thread-db.c | 17 +-
gdb/main.c | 17 +-
gdb/maint-test-options.c | 11 +-
gdb/maint-test-settings.c | 8 +-
gdb/maint.c | 2 +-
gdb/mi/mi-cmd-env.c | 18 +-
gdb/proc-api.c | 5 +-
gdb/python/py-param.c | 65 +++--
gdb/python/python-internal.h | 2 +-
gdb/python/python.c | 29 ++-
gdb/remote-sim.c | 7 +-
gdb/remote.c | 13 +-
gdb/serial.c | 8 +-
gdb/solib.c | 20 +-
gdb/source.c | 66 ++---
gdb/source.h | 5 +-
gdb/stack.c | 22 +-
gdb/symfile.c | 49 ++--
gdb/symtab.c | 46 ++--
gdb/target-descriptions.c | 2 +-
gdb/top.c | 112 ++++-----
gdb/top.h | 2 +-
gdb/tracepoint.c | 29 +--
gdb/tracepoint.h | 2 +-
48 files changed, 1395 insertions(+), 727 deletions(-)
--
2.33.0
next reply other threads:[~2021-09-29 21:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-29 21:50 Lancelot SIX [this message]
2021-09-29 21:50 ` [PATCH v4 1/4] gdb: Introduce setting construct within cmd_list_element Lancelot SIX
2021-09-30 12:12 ` Simon Marchi
2021-09-30 12:30 ` Simon Marchi
2021-09-30 23:15 ` Lancelot SIX
2021-10-01 1:16 ` Simon Marchi
2021-09-30 12:39 ` Simon Marchi
2021-09-29 21:50 ` [PATCH v4 2/4] gdb: make string-like set show commands use std::string variable Lancelot SIX
2021-09-29 21:50 ` [PATCH v4 3/4] gdb: Have setter and getter callbacks for settings Lancelot SIX
2021-09-30 12:42 ` Simon Marchi
2021-10-05 19:10 ` Simon Marchi
2021-10-05 20:27 ` Lancelot SIX
2021-10-05 20:39 ` Simon Marchi
2021-10-05 22:21 ` Lancelot SIX
2021-09-29 21:50 ` [PATCH v4 4/4] gdb: Setting setter return a bool to tell if the value changed Lancelot SIX
2021-09-30 12:43 ` Simon Marchi
2021-09-30 12:44 ` [PATCH v4 0/4] Refactor cmd_list_element.var Simon Marchi
2021-10-03 19:22 ` Lancelot SIX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210929215011.1489639-1-lsix@lancelotsix.com \
--to=lsix@lancelotsix.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).