* [PATCH 0/3] Minor cleanups in disassemble code
@ 2024-03-21 18:10 Tom Tromey
2024-03-21 18:10 ` [PATCH 1/3] Constify get_disassembler_options Tom Tromey
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Tom Tromey @ 2024-03-21 18:10 UTC (permalink / raw)
To: gdb-patches
I happened to look at the disassemble code today, and I saw a few
minor changes that, IMO, make it a bit cleaner.
Regression tested on x86-64 Fedora 38.
---
Tom Tromey (3):
Constify get_disassembler_options
Remove some unnecessary casts
Use std::string for disassembler options
gdb/arc-tdep.c | 15 +++++++--------
gdb/arch-utils.c | 6 +++---
gdb/arm-tdep.c | 6 +++---
gdb/disasm.c | 22 +++++++++-------------
gdb/disasm.h | 2 +-
gdb/gdbarch-gen.h | 4 ++--
gdb/gdbarch.c | 6 +++---
gdb/gdbarch_components.py | 2 +-
gdb/mips-tdep.c | 14 +++++++-------
gdb/riscv-tdep.c | 2 +-
gdb/rs6000-tdep.c | 2 +-
gdb/s390-tdep.c | 2 +-
12 files changed, 39 insertions(+), 44 deletions(-)
---
base-commit: acaf48b921453c37fc2df4151699c912940bcd25
change-id: 20240321-disassemble-cleanup-5da01a245e74
Best regards,
--
Tom Tromey <tromey@adacore.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] Constify get_disassembler_options
2024-03-21 18:10 [PATCH 0/3] Minor cleanups in disassemble code Tom Tromey
@ 2024-03-21 18:10 ` Tom Tromey
2024-03-21 18:10 ` [PATCH 2/3] Remove some unnecessary casts Tom Tromey
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2024-03-21 18:10 UTC (permalink / raw)
To: gdb-patches
This changes get_disassembler_options to return a const char *.
---
gdb/arm-tdep.c | 2 +-
gdb/disasm.c | 2 +-
gdb/disasm.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 3b4ae15df07..82b06f63fcb 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9640,7 +9640,7 @@ show_disassembly_style_sfunc (struct ui_file *file, int from_tty,
struct cmd_list_element *c, const char *value)
{
struct gdbarch *gdbarch = get_current_arch ();
- char *options = get_disassembler_options (gdbarch);
+ const char *options = get_disassembler_options (gdbarch);
const char *style = "";
int len = 0;
const char *opt;
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 15d641cfc1f..5bab5cf6199 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -1291,7 +1291,7 @@ gdb_buffered_insn_length (struct gdbarch *gdbarch,
return result;
}
-char *
+const char *
get_disassembler_options (struct gdbarch *gdbarch)
{
char **disassembler_options = gdbarch_disassembler_options (gdbarch);
diff --git a/gdb/disasm.h b/gdb/disasm.h
index a2251445707..9282632b270 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -385,7 +385,7 @@ extern int gdb_buffered_insn_length (struct gdbarch *gdbarch,
/* Returns GDBARCH's disassembler options. */
-extern char *get_disassembler_options (struct gdbarch *gdbarch);
+extern const char *get_disassembler_options (struct gdbarch *gdbarch);
/* Sets the active gdbarch's disassembler options to OPTIONS. */
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] Remove some unnecessary casts
2024-03-21 18:10 [PATCH 0/3] Minor cleanups in disassemble code Tom Tromey
2024-03-21 18:10 ` [PATCH 1/3] Constify get_disassembler_options Tom Tromey
@ 2024-03-21 18:10 ` Tom Tromey
2024-03-21 18:10 ` [PATCH 3/3] Use std::string for disassembler options Tom Tromey
2024-03-22 17:59 ` [PATCH 0/3] Minor cleanups in disassemble code John Baldwin
3 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2024-03-21 18:10 UTC (permalink / raw)
To: gdb-patches
I found a few unnecessary casts when calling
set_gdbarch_disassembler_options_implicit.
---
gdb/mips-tdep.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 8339d2f1d4c..107d06191b4 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8736,14 +8736,14 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_print_insn (gdbarch, gdb_print_insn_mips);
if (mips_abi == MIPS_ABI_N64)
- set_gdbarch_disassembler_options_implicit
- (gdbarch, (const char *) mips_disassembler_options_n64);
+ set_gdbarch_disassembler_options_implicit (gdbarch,
+ mips_disassembler_options_n64);
else if (mips_abi == MIPS_ABI_N32)
- set_gdbarch_disassembler_options_implicit
- (gdbarch, (const char *) mips_disassembler_options_n32);
+ set_gdbarch_disassembler_options_implicit (gdbarch,
+ mips_disassembler_options_n32);
else
- set_gdbarch_disassembler_options_implicit
- (gdbarch, (const char *) mips_disassembler_options_o32);
+ set_gdbarch_disassembler_options_implicit (gdbarch,
+ mips_disassembler_options_o32);
set_gdbarch_disassembler_options (gdbarch, &mips_disassembler_options);
set_gdbarch_valid_disassembler_options (gdbarch,
disassembler_options_mips ());
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] Use std::string for disassembler options
2024-03-21 18:10 [PATCH 0/3] Minor cleanups in disassemble code Tom Tromey
2024-03-21 18:10 ` [PATCH 1/3] Constify get_disassembler_options Tom Tromey
2024-03-21 18:10 ` [PATCH 2/3] Remove some unnecessary casts Tom Tromey
@ 2024-03-21 18:10 ` Tom Tromey
2024-03-22 17:59 ` [PATCH 0/3] Minor cleanups in disassemble code John Baldwin
3 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2024-03-21 18:10 UTC (permalink / raw)
To: gdb-patches
I noticed that the disassembler_options code uses manual memory
management. It seemed simpler to replace this with std::string.
---
gdb/arc-tdep.c | 15 +++++++--------
gdb/arch-utils.c | 6 +++---
gdb/arm-tdep.c | 4 ++--
gdb/disasm.c | 20 ++++++++------------
gdb/gdbarch-gen.h | 4 ++--
gdb/gdbarch.c | 6 +++---
gdb/gdbarch_components.py | 2 +-
gdb/mips-tdep.c | 2 +-
gdb/riscv-tdep.c | 2 +-
gdb/rs6000-tdep.c | 2 +-
gdb/s390-tdep.c | 2 +-
11 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 7dd43cc239f..63a1fd918d1 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -299,7 +299,7 @@ static const struct arc_register_feature arc_common_aux_reg_feature =
}
};
-static char *arc_disassembler_options = NULL;
+static std::string arc_disassembler_options;
/* Functions are sorted in the order as they are used in the
_initialize_arc_tdep (), which uses the same order as gdbarch.h. Static
@@ -2365,7 +2365,6 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
= tdesc_architecture (info.target_desc);
if (tdesc_arch != NULL)
{
- xfree (arc_disassembler_options);
/* FIXME: It is not really good to change disassembler options
behind the scene, because that might override options
specified by the user. However as of now ARC doesn't support
@@ -2386,24 +2385,24 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
switch (tdesc_arch->mach)
{
case bfd_mach_arc_arc601:
- arc_disassembler_options = xstrdup ("cpu=arc601");
+ arc_disassembler_options = "cpu=arc601";
break;
case bfd_mach_arc_arc600:
- arc_disassembler_options = xstrdup ("cpu=arc600");
+ arc_disassembler_options = "cpu=arc600";
break;
case bfd_mach_arc_arc700:
- arc_disassembler_options = xstrdup ("cpu=arc700");
+ arc_disassembler_options = "cpu=arc700";
break;
case bfd_mach_arc_arcv2:
/* Machine arcv2 has three arches: ARCv2, EM and HS; where ARCv2
is treated as EM. */
if (arc_arch_is_hs (tdesc_arch))
- arc_disassembler_options = xstrdup ("cpu=hs38_linux");
+ arc_disassembler_options = "cpu=hs38_linux";
else
- arc_disassembler_options = xstrdup ("cpu=em4_fpuda");
+ arc_disassembler_options = "cpu=em4_fpuda";
break;
default:
- arc_disassembler_options = NULL;
+ arc_disassembler_options = "";
break;
}
}
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index ae3354f6579..d404d1fc38a 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1156,11 +1156,11 @@ pstring (const char *string)
}
static const char *
-pstring_ptr (char **string)
+pstring_ptr (std::string *string)
{
- if (string == NULL || *string == NULL)
+ if (string == nullptr)
return "(null)";
- return *string;
+ return string->c_str ();
}
/* Helper function to print a list of strings, represented as "const
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 82b06f63fcb..3a5fbe71b8b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -232,7 +232,7 @@ static const char *const arm_register_names[] =
"fps", "cpsr" }; /* 24 25 */
/* Holds the current set of options to be passed to the disassembler. */
-static char *arm_disassembler_options;
+static std::string arm_disassembler_options;
/* Valid register name styles. */
static const char **valid_disassembly_styles;
@@ -11042,7 +11042,7 @@ _initialize_arm_tdep ()
&setarmcmdlist, &showarmcmdlist,
&setlist, &showlist);
- arm_disassembler_options = xstrdup ("reg-names-std");
+ arm_disassembler_options = "reg-names-std";
const disasm_options_t *disasm_options
= &disassembler_options_arm ()->options;
int num_disassembly_styles = 0;
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 5bab5cf6199..cafbc078c60 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -1294,17 +1294,17 @@ gdb_buffered_insn_length (struct gdbarch *gdbarch,
const char *
get_disassembler_options (struct gdbarch *gdbarch)
{
- char **disassembler_options = gdbarch_disassembler_options (gdbarch);
- if (disassembler_options == NULL)
- return NULL;
- return *disassembler_options;
+ std::string *disassembler_options = gdbarch_disassembler_options (gdbarch);
+ if (disassembler_options == nullptr || disassembler_options->empty ())
+ return nullptr;
+ return disassembler_options->c_str ();
}
void
set_disassembler_options (const char *prospective_options)
{
struct gdbarch *gdbarch = get_current_arch ();
- char **disassembler_options = gdbarch_disassembler_options (gdbarch);
+ std::string *disassembler_options = gdbarch_disassembler_options (gdbarch);
const disasm_options_and_args_t *valid_options_and_args;
const disasm_options_t *valid_options;
gdb::unique_xmalloc_ptr<char> prospective_options_local
@@ -1317,11 +1317,8 @@ set_disassembler_options (const char *prospective_options)
to reset their disassembler options to NULL. */
if (options == NULL)
{
- if (disassembler_options != NULL)
- {
- free (*disassembler_options);
- *disassembler_options = NULL;
- }
+ if (disassembler_options != nullptr)
+ disassembler_options->clear ();
return;
}
@@ -1373,8 +1370,7 @@ set_disassembler_options (const char *prospective_options)
}
}
- free (*disassembler_options);
- *disassembler_options = xstrdup (options);
+ *disassembler_options = options;
}
static void
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 13dd0ed5d5e..ebcff80bb9e 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1715,8 +1715,8 @@ extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, g
extern const char * gdbarch_disassembler_options_implicit (struct gdbarch *gdbarch);
extern void set_gdbarch_disassembler_options_implicit (struct gdbarch *gdbarch, const char * disassembler_options_implicit);
-extern char ** gdbarch_disassembler_options (struct gdbarch *gdbarch);
-extern void set_gdbarch_disassembler_options (struct gdbarch *gdbarch, char ** disassembler_options);
+extern std::string * gdbarch_disassembler_options (struct gdbarch *gdbarch);
+extern void set_gdbarch_disassembler_options (struct gdbarch *gdbarch, std::string * disassembler_options);
extern const disasm_options_and_args_t * gdbarch_valid_disassembler_options (struct gdbarch *gdbarch);
extern void set_gdbarch_valid_disassembler_options (struct gdbarch *gdbarch, const disasm_options_and_args_t * valid_disassembler_options);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 80a04bf0caf..9319571deba 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -252,7 +252,7 @@ struct gdbarch
gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp = default_gnu_triplet_regexp;
gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size = default_addressable_memory_unit_size;
const char * disassembler_options_implicit = 0;
- char ** disassembler_options = 0;
+ std::string * disassembler_options = 0;
const disasm_options_and_args_t * valid_disassembler_options = 0;
gdbarch_type_align_ftype *type_align = default_type_align;
gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
@@ -5362,7 +5362,7 @@ set_gdbarch_disassembler_options_implicit (struct gdbarch *gdbarch,
gdbarch->disassembler_options_implicit = disassembler_options_implicit;
}
-char **
+std::string *
gdbarch_disassembler_options (struct gdbarch *gdbarch)
{
gdb_assert (gdbarch != NULL);
@@ -5374,7 +5374,7 @@ gdbarch_disassembler_options (struct gdbarch *gdbarch)
void
set_gdbarch_disassembler_options (struct gdbarch *gdbarch,
- char ** disassembler_options)
+ std::string * disassembler_options)
{
gdbarch->disassembler_options = disassembler_options;
}
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 762d48a0187..7d913ade621 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -2711,7 +2711,7 @@ Functions for allowing a target to modify its disassembler options.
)
Value(
- type="char **",
+ type="std::string *",
name="disassembler_options",
invalid=False,
printer="pstring_ptr (gdbarch->disassembler_options)",
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 107d06191b4..67bed1d2ba0 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -213,7 +213,7 @@ struct target_desc *mips_tdesc_gp32;
struct target_desc *mips_tdesc_gp64;
/* The current set of options to be passed to the disassembler. */
-static char *mips_disassembler_options;
+static std::string mips_disassembler_options;
/* Implicit disassembler options for individual ABIs. These tell
libopcodes to use general-purpose register names corresponding
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 68ef3233e8d..604a19fe2a6 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -133,7 +133,7 @@ static const char *riscv_feature_name_virtual = "org.gnu.gdb.riscv.virtual";
static const char *riscv_feature_name_vector = "org.gnu.gdb.riscv.vector";
/* The current set of options to be passed to the disassembler. */
-static char *riscv_disassembler_options;
+static std::string riscv_disassembler_options;
/* Cached information about a frame. */
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index c8450345be2..3157213cb1f 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -130,7 +130,7 @@
&& (regnum) < (tdep)->ppc_cefpr0_regnum + ppc_num_efprs)
/* Holds the current set of options to be passed to the disassembler. */
-static char *powerpc_disassembler_options;
+static std::string powerpc_disassembler_options;
/* The list of available "set powerpc ..." and "show powerpc ..."
commands. */
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 75983550a5c..174338e1885 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -45,7 +45,7 @@
#include "features/s390x-linux64.c"
/* Holds the current set of options to be passed to the disassembler. */
-static char *s390_disassembler_options;
+static std::string s390_disassembler_options;
/* Breakpoints. */
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Minor cleanups in disassemble code
2024-03-21 18:10 [PATCH 0/3] Minor cleanups in disassemble code Tom Tromey
` (2 preceding siblings ...)
2024-03-21 18:10 ` [PATCH 3/3] Use std::string for disassembler options Tom Tromey
@ 2024-03-22 17:59 ` John Baldwin
2024-03-22 19:12 ` Tom Tromey
3 siblings, 1 reply; 6+ messages in thread
From: John Baldwin @ 2024-03-22 17:59 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 3/21/24 11:10 AM, Tom Tromey wrote:
> I happened to look at the disassemble code today, and I saw a few
> minor changes that, IMO, make it a bit cleaner.
>
> Regression tested on x86-64 Fedora 38.
>
> ---
> Tom Tromey (3):
> Constify get_disassembler_options
> Remove some unnecessary casts
> Use std::string for disassembler options
>
> gdb/arc-tdep.c | 15 +++++++--------
> gdb/arch-utils.c | 6 +++---
> gdb/arm-tdep.c | 6 +++---
> gdb/disasm.c | 22 +++++++++-------------
> gdb/disasm.h | 2 +-
> gdb/gdbarch-gen.h | 4 ++--
> gdb/gdbarch.c | 6 +++---
> gdb/gdbarch_components.py | 2 +-
> gdb/mips-tdep.c | 14 +++++++-------
> gdb/riscv-tdep.c | 2 +-
> gdb/rs6000-tdep.c | 2 +-
> gdb/s390-tdep.c | 2 +-
> 12 files changed, 39 insertions(+), 44 deletions(-)
> ---
> base-commit: acaf48b921453c37fc2df4151699c912940bcd25
> change-id: 20240321-disassemble-cleanup-5da01a245e74
>
> Best regards,
These all seem sensible. It seems a bit odd to me that
gdbarch_disassembler_options returns a pointer to the string rather
than the string, but that was true before and not related to your
changes.
Approved-By: John Baldwin <jhb@FreeBSD.org>
--
John Baldwin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Minor cleanups in disassemble code
2024-03-22 17:59 ` [PATCH 0/3] Minor cleanups in disassemble code John Baldwin
@ 2024-03-22 19:12 ` Tom Tromey
0 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2024-03-22 19:12 UTC (permalink / raw)
To: John Baldwin; +Cc: Tom Tromey, gdb-patches
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
John> These all seem sensible. It seems a bit odd to me that
John> gdbarch_disassembler_options returns a pointer to the string rather
John> than the string, but that was true before and not related to your
John> changes.
I guess it's so it can be set and fetched with a single api, and so
there can be a default of not letting the user touch the options for an
arch. It is a little weird though now that you mention it. Like why
have globals. Cleanup for another day perhaps.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-22 19:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-21 18:10 [PATCH 0/3] Minor cleanups in disassemble code Tom Tromey
2024-03-21 18:10 ` [PATCH 1/3] Constify get_disassembler_options Tom Tromey
2024-03-21 18:10 ` [PATCH 2/3] Remove some unnecessary casts Tom Tromey
2024-03-21 18:10 ` [PATCH 3/3] Use std::string for disassembler options Tom Tromey
2024-03-22 17:59 ` [PATCH 0/3] Minor cleanups in disassemble code John Baldwin
2024-03-22 19:12 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox