[PATCH] Split macro_buffer in two classes, fix Clang build

Pedro Alves
2020-11-06T16:21:07Z

GDB currently fails to build with (at least) Clang 10 and 11, due to:

$ make
CXX macroexp.o
../../src/gdb/macroexp.c:125:3: error: definition of implicit copy constructor for 'macro_buffer' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated-copy-dtor]
~macro_buffer ()
^

Now, we could just add the copy constructor, like we already have a
copy assignment operator. And like that assignment operator, we would
assert that only shared buffers can be copied from.

However, it is hard to see why only shared buffers need to be copied.
I mean, it must be true, otherwise macro support would be broken,
since currently GDB is relying on the default implementation of the
copy constructor, which just copies the fields, which can't work
correctly for the non-shared version. Still, it's not easy to tell
from the code that that is indeed correct, that there isn't some
corner case that would require copying a non-shared buffer.

Or to put it simply - the tangling of shared and non-shared buffers in
the same macro_buffer struct makes this structure hard to understand.

My reaction was -- try splitting the macro_buffer class into two
classes, one for non-shared buffers, and another for shared buffers.

Comments and asserts like these:

...
SRC must be a shared buffer; DEST must not be one. */

static void
scan (struct macro_buffer *dest,
struct macro_buffer *src,
struct macro_name_list *no_loop,
const macro_scope &scope)
{
gdb_assert (src->shared);
gdb_assert (! dest->shared);

... made me suspect it should be possible. Then after the split it
should be easier to reimplement either of the classes if we want.

So I decided to try splitting the struct in two distinct types, and
see where that leads. It turns out that there is really no good
reason for a single struct, no code that wants to work with either
shared or non-shared buffers. It's always shared for input being
parsed, and non-shared for output.

This commit is the result. I named the new classes
shared_macro_buffer and growable_macro_buffer.

A future direction could be for example to make shared_macro_buffer
wrap a string_view and growable_macro_buffer a std::string. With that
in mind, other than text/len, only the 'last_token' field is common to
both classes. I didn't feel like creating a base class just for that
single field.

I constified shared_macro_buffer's 'text' field, which of course had
some knock-on effects, fixed in the patch.

On the original warning issued by Clang -- now it is clear that really
only the shared version needs copy ctor/assign, so I added a copy
ctor, implemented on top of the assignment operator:

shared_macro_buffer (const shared_macro_buffer &src)
{
*this = src;
}

I disabled copy/assign for the growable version.

gdb/ChangeLog:

* macroexp.c (struct macro_buffer): Split in two classes. Add
uses adjusted.
(struct shared_macro_buffer): New, factored out from struct
macro_buffer.
(struct growable_macro_buffer): New, factored out from struct
macro_buffer.
(set_token, get_comment, get_identifier, get_pp_number)
(get_character_constant, get_string_literal, get_punctuator)
(get_next_token_for_substitution): Constify parameters.
(substitute_args): Constify locals.

Change-Id: I5712e30e826d949715703b2e9172adf04e63b152
---
gdb/macroexp.c | 276 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 134 insertions(+), 142 deletions(-)

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 68fb1967161..beee21c9fc6 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -26,37 +26,20 @@

\f
-/* A resizeable, substringable string type. */

+/* A string type that we can use to refer to substrings of other
+ strings. */

-/* A string type that we can resize, quickly append to, and use to
- refer to substrings of other strings. */
-struct macro_buffer
+struct shared_macro_buffer
{
- /* An array of characters. The first LEN bytes are the real text,
- but there are SIZE bytes allocated to the array. If SIZE is
- zero, then this doesn't point to a malloc'ed block. If SHARED is
- non-zero, then this buffer is actually a pointer into some larger
- string, and we shouldn't append characters to it, etc. Because
- of sharing, we can't assume in general that the text is
+ /* An array of characters. This buffer is a pointer into some
+ larger string and thus we can't assume in that the text is
null-terminated. */
- char *text;
+ const char *text;

/* The number of characters in the string. */
int len;

- /* The number of characters allocated to the string. If SHARED is
- non-zero, this is meaningless; in this case, we set it to zero so
- that any "do we have room to append something?" tests will fail,
- so we don't always have to check SHARED before using this field. */
- int size;
-
- /* Zero if TEXT can be safely realloc'ed (i.e., it's its own malloc
- block). Non-zero if TEXT is actually pointing into the middle of
- some other block, or to a string literal, and we shouldn't
- reallocate it. */
- bool shared;
-
/* For detecting token splicing.

This is the index in TEXT of the first character of the token
@@ -71,33 +54,15 @@ struct macro_buffer
is non-zero if it is an identifier token, zero otherwise. */
int is_identifier = 0;

-
- macro_buffer ()
+ shared_macro_buffer ()
: text (NULL),
- len (0),
- size (0),
- shared (false)
+ len (0)
{
}

- /* Set the macro buffer to the empty string, guessing that its
- final contents will fit in N bytes. (It'll get resized if it
- doesn't, so the guess doesn't have to be right.) Allocate the
- initial storage with xmalloc. */
- explicit macro_buffer (int n)
- : len (0),
- size (n),
- shared (false)
- {
- if (n > 0)
- text = (char *) xmalloc (n);
- else
- text = NULL;
- }
-
/* Set the macro buffer to refer to the LEN bytes at ADDR, as a
shared substring. */
- macro_buffer (const char *addr, int len)
+ shared_macro_buffer (const char *addr, int len)
{
set_shared (addr, len);
}
@@ -106,45 +71,81 @@ struct macro_buffer
shared substring. */
void set_shared (const char *addr, int len_)
{
- text = (char *) addr;
+ text = addr;
len = len_;
- size = 0;
- shared = true;
}

- macro_buffer& operator= (const macro_buffer &src)
+ shared_macro_buffer (const shared_macro_buffer &src)
+ {
+ *this = src;
+ }
+
+ shared_macro_buffer& operator= (const shared_macro_buffer &src)
{
- gdb_assert (src.shared);
- gdb_assert (shared);
set_shared (src.text, src.len);
last_token = src.last_token;
is_identifier = src.is_identifier;
return *this;
}
+};
+
+/* A string type that we can resize and quickly append to. */
+
+struct growable_macro_buffer
+{
+ /* An array of characters. The first LEN bytes are the real text,
+ but there are SIZE bytes allocated to the array. */
+ char *text;
+
+ /* The number of characters in the string. */
+ int len;
+
+ /* The number of characters allocated to the string. */
+ int size;
+
+ /* For detecting token splicing.
+
+ This is the index in TEXT of the first character of the token
+ that abuts the end of TEXT. If TEXT contains no tokens, then we
+ set this equal to LEN. If TEXT ends in whitespace, then there is
+ no token abutting the end of TEXT (it's just whitespace), and
+ again, we set this equal to LEN. We set this to -1 if we don't
+ know the nature of TEXT. */
+ int last_token = -1;
+
+ /* Set the macro buffer to the empty string, guessing that its
+ final contents will fit in N bytes. (It'll get resized if it
+ doesn't, so the guess doesn't have to be right.) Allocate the
+ initial storage with xmalloc. */
+ explicit growable_macro_buffer (int n)
+ : len (0),
+ size (n)
+ {
+ if (n > 0)
+ text = (char *) xmalloc (n);
+ else
+ text = NULL;
+ }

- ~macro_buffer ()
+ DISABLE_COPY_AND_ASSIGN (growable_macro_buffer);
+
+ ~growable_macro_buffer ()
{
- if (! shared && size)
- xfree (text);
+ xfree (text);
}

/* Release the text of the buffer to the caller. */
gdb::unique_xmalloc_ptr<char> release ()
{
- gdb_assert (! shared);
gdb_assert (size);
char *result = text;
text = NULL;
return gdb::unique_xmalloc_ptr<char> (result);
}

- /* Resize the buffer to be at least N bytes long. Raise an error if
- the buffer shouldn't be resized. */
+ /* Resize the buffer to be at least N bytes long. */
void resize_buffer (int n)
{
- /* We shouldn't be trying to resize shared strings. */
- gdb_assert (! shared);
-
if (size == 0)
size = n;
else
@@ -212,7 +213,7 @@ macro_is_identifier_nondigit (int c)

static void
-set_token (struct macro_buffer *tok, char *start, char *end)
+set_token (shared_macro_buffer *tok, const char *start, const char *end)
{
tok->set_shared (start, end - start);
tok->last_token = 0;
@@ -223,14 +224,14 @@ set_token (struct macro_buffer *tok, char *start, char *end)

static int
-get_comment (struct macro_buffer *tok, char *p, char *end)
+get_comment (shared_macro_buffer *tok, const char *p, const char *end)
{
if (p + 2 > end)
return 0;
else if (p[0] == '/'
&& p[1] == '*')
{
- char *tok_start = p;
+ const char *tok_start = p;

p += 2;

@@ -249,7 +250,7 @@ get_comment (struct macro_buffer *tok, char *p, char *end)
else if (p[0] == '/'
&& p[1] == '/')
{
- char *tok_start = p;
+ const char *tok_start = p;

p += 2;
for (; p < end; p++)
@@ -265,12 +266,12 @@ get_comment (struct macro_buffer *tok, char *p, char *end)

static int
-get_identifier (struct macro_buffer *tok, char *p, char *end)
+get_identifier (shared_macro_buffer *tok, const char *p, const char *end)
{
if (p < end
&& macro_is_identifier_nondigit (*p))
{
- char *tok_start = p;
+ const char *tok_start = p;

while (p < end
&& (macro_is_identifier_nondigit (*p)
@@ -287,7 +288,7 @@ get_identifier (struct macro_buffer *tok, char *p, char *end)

static int
-get_pp_number (struct macro_buffer *tok, char *p, char *end)
+get_pp_number (shared_macro_buffer *tok, const char *p, const char *end)
{
if (p < end
&& (macro_is_digit (*p)
@@ -295,7 +296,7 @@ get_pp_number (struct macro_buffer *tok, char *p, char *end)
&& p + 2 <= end
&& macro_is_digit (p[1]))))
{
- char *tok_start = p;
+ const char *tok_start = p;

while (p < end)
{
@@ -326,7 +327,8 @@ get_pp_number (struct macro_buffer *tok, char *p, char *end)
Signal an error if it contains a malformed or incomplete character
constant. */
static int
-get_character_constant (struct macro_buffer *tok, char *p, char *end)
+get_character_constant (shared_macro_buffer *tok,
+ const char *p, const char *end)
{
/* ISO/IEC 9899:1999 (E) Section 6.4.4.4 paragraph 1
But of course, what really matters is that we handle it the same
@@ -337,7 +339,7 @@ get_character_constant (struct macro_buffer *tok, char *p, char *end)
&& (p[0] == 'L' || p[0] == 'u' || p[0] == 'U')
&& p[1] == '\''))
{
- char *tok_start = p;
+ const char *tok_start = p;
int char_count = 0;

if (*p == '\'')
@@ -387,7 +389,7 @@ get_character_constant (struct macro_buffer *tok, char *p, char *end)
literal, and return 1. Otherwise, return zero. Signal an error if
it contains a malformed or incomplete string literal. */
static int
-get_string_literal (struct macro_buffer *tok, char *p, char *end)
+get_string_literal (shared_macro_buffer *tok, const char *p, const char *end)
{
if ((p + 1 <= end
&& *p == '"')
@@ -395,7 +397,7 @@ get_string_literal (struct macro_buffer *tok, char *p, char *end)
&& (p[0] == 'L' || p[0] == 'u' || p[0] == 'U')
&& p[1] == '"'))
{
- char *tok_start = p;
+ const char *tok_start = p;

if (*p == '"')
p++;
@@ -437,7 +439,7 @@ get_string_literal (struct macro_buffer *tok, char *p, char *end)

static int
-get_punctuator (struct macro_buffer *tok, char *p, char *end)
+get_punctuator (shared_macro_buffer *tok, const char *p, const char *end)
{
/* Here, speed is much less important than correctness and clarity. */

@@ -493,18 +495,16 @@ get_punctuator (struct macro_buffer *tok, char *p, char *end)

/* Peel the next preprocessor token off of SRC, and put it in TOK.
Mutate TOK to refer to the first token in SRC, and mutate SRC to
- refer to the text after that token. SRC must be a shared buffer;
- the resulting TOK will be shared, pointing into the same string SRC
- does. Initialize TOK's last_token field. Return non-zero if we
- succeed, or 0 if we didn't find any more tokens in SRC. */
+ refer to the text after that token. The resulting TOK will point
+ into the same string SRC does. Initialize TOK's last_token field.
+ Return non-zero if we succeed, or 0 if we didn't find any more
+ tokens in SRC. */
+
static int
-get_token (struct macro_buffer *tok,
- struct macro_buffer *src)
+get_token (shared_macro_buffer *tok, shared_macro_buffer *src)
{
- char *p = src->text;
- char *end = p + src->len;
-
- gdb_assert (src->shared);
+ const char *p = src->text;
+ const char *end = p + src->len;

/* From the ISO C standard, ISO/IEC 9899:1999 (E), section 6.4:

@@ -583,11 +583,11 @@ get_token (struct macro_buffer *tok,
fine to return with DEST set to "x y". Similarly, "<" and "<" must
yield "< <", not "<<", etc. */
static void
-append_tokens_without_splicing (struct macro_buffer *dest,
- struct macro_buffer *src)
+append_tokens_without_splicing (growable_macro_buffer *dest,
+ shared_macro_buffer *src)
{
int original_dest_len = dest->len;
- struct macro_buffer dest_tail, new_token;
+ shared_macro_buffer dest_tail, new_token;

gdb_assert (src->last_token != -1);
gdb_assert (dest->last_token != -1);
@@ -653,7 +653,7 @@ append_tokens_without_splicing (struct macro_buffer *dest,
stringify; it is LEN bytes long. */

static void
-stringify (struct macro_buffer *dest, const char *arg, int len)
+stringify (growable_macro_buffer *dest, const char *arg, int len)
{
/* Trim initial whitespace from ARG. */
while (len > 0 && macro_is_whitespace (*arg))
@@ -702,7 +702,7 @@ gdb::unique_xmalloc_ptr<char>
macro_stringify (const char *str)
{
int len = strlen (str);
- struct macro_buffer buffer (len);
+ growable_macro_buffer buffer (len);

stringify (&buffer, str, len);
buffer.appendc ('\0');
@@ -780,17 +780,17 @@ currently_rescanning (struct macro_name_list *list, const char *name)
following the invocation. */

static bool
-gather_arguments (const char *name, struct macro_buffer *src, int nargs,
- std::vector<struct macro_buffer> *args_ptr)
+gather_arguments (const char *name, shared_macro_buffer *src, int nargs,
+ std::vector<shared_macro_buffer> *args_ptr)
{
- struct macro_buffer tok;
- std::vector<struct macro_buffer> args;
+ shared_macro_buffer tok;
+ std::vector<shared_macro_buffer> args;

/* Does SRC start with an opening paren token? Read from a copy of
SRC, so SRC itself is unaffected if we don't find an opening
paren. */
{
- struct macro_buffer temp (src->text, src->len);
+ shared_macro_buffer temp (src->text, src->len);

if (! get_token (&tok, &temp)
|| tok.len != 1
@@ -803,7 +803,7 @@ gather_arguments (const char *name, struct macro_buffer *src, int nargs,

for (;;)
{
- struct macro_buffer *arg;
+ shared_macro_buffer *arg;
int depth;

/* Initialize the next argument. */
@@ -874,8 +874,8 @@ gather_arguments (const char *name, struct macro_buffer *src, int nargs,

/* The `expand' and `substitute_args' functions both invoke `scan'
recursively, so we need a forward declaration somewhere. */
-static void scan (struct macro_buffer *dest,
- struct macro_buffer *src,
+static void scan (growable_macro_buffer *dest,
+ shared_macro_buffer *src,
struct macro_name_list *no_loop,
const macro_scope &scope);

@@ -891,8 +891,8 @@ static void scan (struct macro_buffer *dest,
index. If TOK is not an argument, return -1. */

static int
-find_parameter (const struct macro_buffer *tok,
- int is_varargs, const struct macro_buffer *va_arg_name,
+find_parameter (const shared_macro_buffer *tok,
+ int is_varargs, const shared_macro_buffer *va_arg_name,
int argc, const char * const *argv)
{
int i;
@@ -916,11 +916,11 @@ find_parameter (const struct macro_buffer *tok,
updates the passed-in state variables. */

static void
-get_next_token_for_substitution (struct macro_buffer *replacement_list,
- struct macro_buffer *token,
- char **start,
- struct macro_buffer *lookahead,
- char **lookahead_start,
+get_next_token_for_substitution (shared_macro_buffer *replacement_list,
+ shared_macro_buffer *token,
+ const char **start,
+ shared_macro_buffer *lookahead,
+ const char **lookahead_start,
int *lookahead_valid,
bool *keep_going)
{
@@ -952,27 +952,27 @@ get_next_token_for_substitution (struct macro_buffer *replacement_list,
NO_LOOP. */

static void
-substitute_args (struct macro_buffer *dest,
+substitute_args (growable_macro_buffer *dest,
struct macro_definition *def,
- int is_varargs, const struct macro_buffer *va_arg_name,
- const std::vector<struct macro_buffer> &argv,
+ int is_varargs, const shared_macro_buffer *va_arg_name,
+ const std::vector<shared_macro_buffer> &argv,
struct macro_name_list *no_loop,
const macro_scope &scope)
{
/* The token we are currently considering. */
- struct macro_buffer tok;
+ shared_macro_buffer tok;
/* The replacement list's pointer from just before TOK was lexed. */
- char *original_rl_start;
+ const char *original_rl_start;
/* We have a single lookahead token to handle token splicing. */
- struct macro_buffer lookahead;
+ shared_macro_buffer lookahead;
/* The lookahead token might not be valid. */
int lookahead_valid;
/* The replacement list's pointer from just before LOOKAHEAD was
lexed. */
- char *lookahead_rl_start;
+ const char *lookahead_rl_start;

/* A macro buffer for the macro's replacement list. */
- struct macro_buffer replacement_list (def->replacement,
+ shared_macro_buffer replacement_list (def->replacement,
strlen (def->replacement));

gdb_assert (dest->len == 0);
@@ -1190,7 +1190,7 @@ substitute_args (struct macro_buffer *dest,
mutates its source, so we need to scan a new buffer
referring to the argument's text, not the argument
itself. */
- struct macro_buffer arg_src (argv[arg].text, argv[arg].len);
+ shared_macro_buffer arg_src (argv[arg].text, argv[arg].len);
scan (dest, &arg_src, no_loop, scope);
substituted = 1;
}
@@ -1217,9 +1217,9 @@ substitute_args (struct macro_buffer *dest,
we don't expand it.) If we return zero, leave SRC unchanged. */
static int
expand (const char *id,
- struct macro_definition *def,
- struct macro_buffer *dest,
- struct macro_buffer *src,
+ struct macro_definition *def,
+ growable_macro_buffer *dest,
+ shared_macro_buffer *src,
struct macro_name_list *no_loop,
const macro_scope &scope)
{
@@ -1236,7 +1236,7 @@ expand (const char *id,
/* What kind of macro are we expanding? */
if (def->kind == macro_object_like)
{
- struct macro_buffer replacement_list (def->replacement,
+ shared_macro_buffer replacement_list (def->replacement,
strlen (def->replacement));

scan (dest, &replacement_list, &new_no_loop, scope);
@@ -1244,7 +1244,7 @@ expand (const char *id,
}
else if (def->kind == macro_function_like)
{
- struct macro_buffer va_arg_name;
+ shared_macro_buffer va_arg_name;
int is_varargs = 0;

if (def->argc >= 1)
@@ -1272,7 +1272,7 @@ expand (const char *id,
}
}

- std::vector<struct macro_buffer> argv;
+ std::vector<shared_macro_buffer> argv;
/* If we couldn't find any argument list, then we don't expand
this macro. */
if (!gather_arguments (id, src, is_varargs ? def->argc : -1,
@@ -1304,21 +1304,21 @@ expand (const char *id,
splicing operator "##" don't get macro references expanded,
so we can't really tell whether it's appropriate to macro-
expand an argument until we see how it's being used. */
- struct macro_buffer substituted (0);
+ growable_macro_buffer substituted (0);
substitute_args (&substituted, def, is_varargs, &va_arg_name,
argv, no_loop, scope);

/* Now `substituted' is the macro's replacement list, with all
- argument values substituted into it properly. Re-scan it for
+ argument values substituted into it properly. Re-scan it for
macro references, but don't expand invocations of this macro.

We create a new buffer, `substituted_src', which points into
- `substituted', and scan that. We can't scan `substituted'
+ `substituted', and scan that. We can't scan `substituted'
itself, since the tokenization process moves the buffer's
text pointer around, and we still need to be able to find
`substituted's original text buffer after scanning it so we
can free it. */
- struct macro_buffer substituted_src (substituted.text, substituted.len);
+ shared_macro_buffer substituted_src (substituted.text, substituted.len);
scan (dest, &substituted_src, &new_no_loop, scope);

return 1;
@@ -1333,19 +1333,14 @@ expand (const char *id,
expansion to DEST and return non-zero. Otherwise, return zero, and
leave DEST unchanged.

- SRC_FIRST and SRC_REST must be shared buffers; DEST must not be one.
SRC_FIRST must be a string built by get_token. */
static int
-maybe_expand (struct macro_buffer *dest,
- struct macro_buffer *src_first,
- struct macro_buffer *src_rest,
+maybe_expand (growable_macro_buffer *dest,
+ shared_macro_buffer *src_first,
+ shared_macro_buffer *src_rest,
struct macro_name_list *no_loop,
const macro_scope &scope)
{
- gdb_assert (src_first->shared);
- gdb_assert (src_rest->shared);
- gdb_assert (! dest->shared);
-
/* Is this token an identifier? */
if (src_first->is_identifier)
{
@@ -1371,22 +1366,19 @@ maybe_expand (struct macro_buffer *dest,

/* Expand macro references in SRC, appending the results to DEST.
Assume we are re-scanning the result of expanding the macros named
- in NO_LOOP, and don't try to re-expand references to them.
+ in NO_LOOP, and don't try to re-expand references to them. */

- SRC must be a shared buffer; DEST must not be one. */
static void
-scan (struct macro_buffer *dest,
- struct macro_buffer *src,
+scan (growable_macro_buffer *dest,
+ shared_macro_buffer *src,
struct macro_name_list *no_loop,
const macro_scope &scope)
{
- gdb_assert (src->shared);
- gdb_assert (! dest->shared);

for (;;)
{
- struct macro_buffer tok;
- char *original_src_start = src->text;
+ shared_macro_buffer tok;
+ const char *original_src_start = src->text;

/* Find the next token in SRC. */
if (! get_token (&tok, src))
@@ -1419,9 +1411,9 @@ scan (struct macro_buffer *dest,
gdb::unique_xmalloc_ptr<char>
macro_expand (const char *source, const macro_scope &scope)
{
- struct macro_buffer src (source, strlen (source));
+ shared_macro_buffer src (source, strlen (source));

- struct macro_buffer dest (0);
+ growable_macro_buffer dest (0);
dest.last_token = 0;

scan (&dest, &src, 0, scope);
@@ -1441,13 +1433,13 @@ macro_expand_once (const char *source, const macro_scope &scope)
gdb::unique_xmalloc_ptr<char>
macro_expand_next (const char **lexptr, const macro_scope &scope)
{
- struct macro_buffer tok;
+ shared_macro_buffer tok;

/* Set up SRC to refer to the input text, pointed to by *lexptr. */
- struct macro_buffer src (*lexptr, strlen (*lexptr));
+ shared_macro_buffer src (*lexptr, strlen (*lexptr));

/* Set up DEST to receive the expansion, if there is one. */
- struct macro_buffer dest (0);
+ growable_macro_buffer dest (0);
dest.last_token = 0;

/* Get the text's first preprocessing token. */

base-commit: 2c72361c810f4b6223949363f635376e4311ac7a
--
2.14.5