* [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition
@ 2024-06-24 8:14 Tom de Vries
2024-06-24 8:14 ` [PATCH 2/2] [gdb/symtab] Malloc/free less " Tom de Vries
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Tom de Vries @ 2024-06-24 8:14 UTC (permalink / raw)
To: gdb-patches
In parse_macro_definition, there's a loop:
...
for (p = body; *p; p++)
if (*p == ' ' || *p == '(')
break;
...
whose post-condition is:
...
gdb_assert (*p == ' ' || *p == '(' || *p == '\0');
...
Consequently, in the following:
...
if (*p == ' ' || *p == '\0')
<BODY1>
else if (*p == '(')
<BODY2>
else
<BODY3>
...
BODY3 is dead code.
Remove it, and get rid of unnecessary indentation by using an early-exit:
....
if (*p == ' ' || *p == '\0')
{
<BODY1>
return;
}
gdb_assert (*p == '(');
<BODY2>
...
Tested on aarch64-linux.
---
gdb/dwarf2/macro.c | 117 ++++++++++++++++++++++-----------------------
1 file changed, 56 insertions(+), 61 deletions(-)
diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index a511d0a3b44..bc781c2cb92 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -154,88 +154,83 @@ parse_macro_definition (struct macro_source_file *file, int line,
}
macro_define_object (file, line, name.c_str (), replacement);
+ return;
}
- else if (*p == '(')
- {
- /* It's a function-like macro. */
- std::string name (body, p - body);
- int argc = 0;
- int argv_size = 1;
- char **argv = XNEWVEC (char *, argv_size);
-
- p++;
- p = consume_improper_spaces (p, body);
+ /* It's a function-like macro. */
+ gdb_assert (*p == '(');
+ std::string name (body, p - body);
+ int argc = 0;
+ int argv_size = 1;
+ char **argv = XNEWVEC (char *, argv_size);
- /* Parse the formal argument list. */
- while (*p && *p != ')')
- {
- /* Find the extent of the current argument name. */
- const char *arg_start = p;
+ p++;
- while (*p && *p != ',' && *p != ')' && *p != ' ')
- p++;
-
- if (! *p || p == arg_start)
- dwarf2_macro_malformed_definition_complaint (body);
- else
- {
- /* Make sure argv has room for the new argument. */
- if (argc >= argv_size)
- {
- argv_size *= 2;
- argv = XRESIZEVEC (char *, argv, argv_size);
- }
+ p = consume_improper_spaces (p, body);
- argv[argc++] = savestring (arg_start, p - arg_start);
- }
+ /* Parse the formal argument list. */
+ while (*p && *p != ')')
+ {
+ /* Find the extent of the current argument name. */
+ const char *arg_start = p;
- p = consume_improper_spaces (p, body);
+ while (*p && *p != ',' && *p != ')' && *p != ' ')
+ p++;
- /* Consume the comma, if present. */
- if (*p == ',')
+ if (! *p || p == arg_start)
+ dwarf2_macro_malformed_definition_complaint (body);
+ else
+ {
+ /* Make sure argv has room for the new argument. */
+ if (argc >= argv_size)
{
- p++;
-
- p = consume_improper_spaces (p, body);
+ argv_size *= 2;
+ argv = XRESIZEVEC (char *, argv, argv_size);
}
+
+ argv[argc++] = savestring (arg_start, p - arg_start);
}
- if (*p == ')')
+ p = consume_improper_spaces (p, body);
+
+ /* Consume the comma, if present. */
+ if (*p == ',')
{
p++;
- if (*p == ' ')
- /* Perfectly formed definition, no complaints. */
- macro_define_function (file, line, name.c_str (),
- argc, (const char **) argv,
- p + 1);
- else if (*p == '\0')
- {
- /* Complain, but do define it. */
- dwarf2_macro_malformed_definition_complaint (body);
- macro_define_function (file, line, name.c_str (),
- argc, (const char **) argv,
- p);
- }
- else
- /* Just complain. */
- dwarf2_macro_malformed_definition_complaint (body);
+ p = consume_improper_spaces (p, body);
+ }
+ }
+
+ if (*p == ')')
+ {
+ p++;
+
+ if (*p == ' ')
+ /* Perfectly formed definition, no complaints. */
+ macro_define_function (file, line, name.c_str (),
+ argc, (const char **) argv,
+ p + 1);
+ else if (*p == '\0')
+ {
+ /* Complain, but do define it. */
+ dwarf2_macro_malformed_definition_complaint (body);
+ macro_define_function (file, line, name.c_str (),
+ argc, (const char **) argv,
+ p);
}
else
/* Just complain. */
dwarf2_macro_malformed_definition_complaint (body);
-
- {
- int i;
-
- for (i = 0; i < argc; i++)
- xfree (argv[i]);
- }
- xfree (argv);
}
else
+ /* Just complain. */
dwarf2_macro_malformed_definition_complaint (body);
+
+ for (int i = 0; i < argc; i++)
+ xfree (argv[i]);
+
+ xfree (argv);
}
/* Skip some bytes from BYTES according to the form given in FORM.
base-commit: 18b13d11d37c8b7e7f9d0548d54e0cb3a01e81fb
--
2.35.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] [gdb/symtab] Malloc/free less in parse_macro_definition
2024-06-24 8:14 [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition Tom de Vries
@ 2024-06-24 8:14 ` Tom de Vries
2024-06-24 18:20 ` Tom Tromey
2024-06-24 13:08 ` [PATCH 1/2] [gdb/symtab] Remove dead code " Alexandra Petlanova Hajkova
2024-06-24 18:23 ` Tom Tromey
2 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2024-06-24 8:14 UTC (permalink / raw)
To: gdb-patches
I noticed that parse_macro_definition does malloc and free, both for the
argv array and its elements.
Improve this by:
- using an std::vector for the argv array,
- using an obstack for the elements, and
- sharing allocations between calls to parse_macro_definition.
Tested on aarch64-linux.
---
gdb/dwarf2/macro.c | 60 +++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index bc781c2cb92..83965627d5b 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -36,6 +36,31 @@
#include "complaints.h"
#include "objfiles.h"
+/* Vector of C strings. Allows to push a string_view-like string while
+ managing the memory required for that. */
+
+class vector_c_string : public std::vector<const char *>
+{
+public:
+ /* Push a copy of ARG[0..ARG_SIZE-1]. */
+ void push_back (const char *arg, size_t arg_size)
+ {
+ const char *dup = obstack_strndup (&m_obstack, arg, arg_size);
+ std::vector<const char *>::push_back (dup);
+ }
+
+ /* Clear the vector, and de-allocate the copies. */
+ void clear ()
+ {
+ std::vector<const char *>::clear ();
+ m_obstack.clear ();
+ }
+
+private:
+ /* Used to allocate copies. */
+ auto_obstack m_obstack;
+};
+
static void
dwarf2_macro_malformed_definition_complaint (const char *arg1)
{
@@ -105,6 +130,9 @@ static void
parse_macro_definition (struct macro_source_file *file, int line,
const char *body)
{
+ /* By making this static we share allocation between invocations. */
+ static vector_c_string argv;
+
const char *p;
/* The body string takes one of two forms. For object-like macro
@@ -160,9 +188,8 @@ parse_macro_definition (struct macro_source_file *file, int line,
/* It's a function-like macro. */
gdb_assert (*p == '(');
std::string name (body, p - body);
- int argc = 0;
- int argv_size = 1;
- char **argv = XNEWVEC (char *, argv_size);
+
+ argv.clear ();
p++;
@@ -181,14 +208,10 @@ parse_macro_definition (struct macro_source_file *file, int line,
dwarf2_macro_malformed_definition_complaint (body);
else
{
- /* Make sure argv has room for the new argument. */
- if (argc >= argv_size)
- {
- argv_size *= 2;
- argv = XRESIZEVEC (char *, argv, argv_size);
- }
-
- argv[argc++] = savestring (arg_start, p - arg_start);
+ /* Push a zero-terminated copy of arg. This wouldn't be
+ necessary if macro_define_function handled string views
+ args. */
+ argv.push_back (arg_start, p - arg_start);
}
p = consume_improper_spaces (p, body);
@@ -208,16 +231,14 @@ parse_macro_definition (struct macro_source_file *file, int line,
if (*p == ' ')
/* Perfectly formed definition, no complaints. */
- macro_define_function (file, line, name.c_str (),
- argc, (const char **) argv,
- p + 1);
+ macro_define_function (file, line, name.c_str (), argv.size (),
+ argv.data (), p + 1);
else if (*p == '\0')
{
/* Complain, but do define it. */
dwarf2_macro_malformed_definition_complaint (body);
- macro_define_function (file, line, name.c_str (),
- argc, (const char **) argv,
- p);
+ macro_define_function (file, line, name.c_str (), argv.size (),
+ argv.data (), p);
}
else
/* Just complain. */
@@ -226,11 +247,6 @@ parse_macro_definition (struct macro_source_file *file, int line,
else
/* Just complain. */
dwarf2_macro_malformed_definition_complaint (body);
-
- for (int i = 0; i < argc; i++)
- xfree (argv[i]);
-
- xfree (argv);
}
/* Skip some bytes from BYTES according to the form given in FORM.
--
2.35.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition
2024-06-24 8:14 [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition Tom de Vries
2024-06-24 8:14 ` [PATCH 2/2] [gdb/symtab] Malloc/free less " Tom de Vries
@ 2024-06-24 13:08 ` Alexandra Petlanova Hajkova
2024-06-24 18:23 ` Tom Tromey
2 siblings, 0 replies; 6+ messages in thread
From: Alexandra Petlanova Hajkova @ 2024-06-24 13:08 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 951 bytes --]
On Mon, Jun 24, 2024 at 10:13 AM Tom de Vries <tdevries@suse.de> wrote:
> In parse_macro_definition, there's a loop:
> ...
> for (p = body; *p; p++)
> if (*p == ' ' || *p == '(')
> break;
> ...
> whose post-condition is:
> ...
> gdb_assert (*p == ' ' || *p == '(' || *p == '\0');
> ...
>
> Consequently, in the following:
> ...
> if (*p == ' ' || *p == '\0')
> <BODY1>
> else if (*p == '(')
> <BODY2>
> else
> <BODY3>
> ...
> BODY3 is dead code.
>
> Remove it, and get rid of unnecessary indentation by using an early-exit:
> ....
> if (*p == ' ' || *p == '\0')
> {
> <BODY1>
> return;
> }
>
> gdb_assert (*p == '(');
> <BODY2>
> ...
>
> Tested on aarch64-linux.
> ---
>
>
Look reasonable. Also I can confirm this change does not cause any
resgrassions for ppc64le Fedora Rawhide.
Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] [gdb/symtab] Malloc/free less in parse_macro_definition
2024-06-24 8:14 ` [PATCH 2/2] [gdb/symtab] Malloc/free less " Tom de Vries
@ 2024-06-24 18:20 ` Tom Tromey
2024-06-26 6:33 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2024-06-24 18:20 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> +/* Vector of C strings. Allows to push a string_view-like string while
Tom> + managing the memory required for that. */
Tom> +
Tom> +class vector_c_string : public std::vector<const char *>
I think it's generally considered bad practice to inherit from std
types.
Tom> + /* By making this static we share allocation between invocations. */
Tom> + static vector_c_string argv;
I think we should avoid static locals. For one thing they inhibit our
ability to use threads.
If allocation is a performance issue or something then maybe the storage
can be hoisted to some more-outer function somewhere.
I wonder if this could share some code with:
https://inbox.sourceware.org/gdb-patches/20240426154439.94728-1-tromey@adacore.com/
Like maybe a different refactoring is in order.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition
2024-06-24 8:14 [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition Tom de Vries
2024-06-24 8:14 ` [PATCH 2/2] [gdb/symtab] Malloc/free less " Tom de Vries
2024-06-24 13:08 ` [PATCH 1/2] [gdb/symtab] Remove dead code " Alexandra Petlanova Hajkova
@ 2024-06-24 18:23 ` Tom Tromey
2 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2024-06-24 18:23 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> Remove it, and get rid of unnecessary indentation by using an early-exit:
Tom> ....
Tom> if (*p == ' ' || *p == '\0')
Tom> {
Tom> <BODY1>
Tom> return;
Tom> }
Looks good. Thank you.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] [gdb/symtab] Malloc/free less in parse_macro_definition
2024-06-24 18:20 ` Tom Tromey
@ 2024-06-26 6:33 ` Tom de Vries
0 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries @ 2024-06-26 6:33 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 6/24/24 20:20, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>
> Tom> +/* Vector of C strings. Allows to push a string_view-like string while
> Tom> + managing the memory required for that. */
> Tom> +
> Tom> +class vector_c_string : public std::vector<const char *>
>
> I think it's generally considered bad practice to inherit from std
> types.
>
Thanks for pointing that out. I googled a bit, and understood that this
is due to std::vector having a non-virtual destructor.
> Tom> + /* By making this static we share allocation between invocations. */
> Tom> + static vector_c_string argv;
>
> I think we should avoid static locals. For one thing they inhibit our
> ability to use threads.
>
> If allocation is a performance issue or something then maybe the storage
> can be hoisted to some more-outer function somewhere.
>
Done.
> I wonder if this could share some code with:
>
> https://inbox.sourceware.org/gdb-patches/20240426154439.94728-1-tromey@adacore.com/
>
> Like maybe a different refactoring is in order.
I've borrowed from that patch, and submitted a v2 (named slightly
differently) here (
https://sourceware.org/pipermail/gdb-patches/2024-June/210218.html ).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-26 6:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-24 8:14 [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition Tom de Vries
2024-06-24 8:14 ` [PATCH 2/2] [gdb/symtab] Malloc/free less " Tom de Vries
2024-06-24 18:20 ` Tom Tromey
2024-06-26 6:33 ` Tom de Vries
2024-06-24 13:08 ` [PATCH 1/2] [gdb/symtab] Remove dead code " Alexandra Petlanova Hajkova
2024-06-24 18:23 ` Tom Tromey
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).