Re: [RFA] "constify" parse_exp_1

Pedro Alves
2013-03-08T12:27:39Z

On 03/08/2013 01:01 AM, Keith Seitz wrote:
> else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
> {
> - char *tmptok;
> + char *endp;
> + const char *tmptok;
>
> tok = end_tok + 1;
> tmptok = tok;
> - *thread = strtol (tok, &tok, 0);
> + *thread = strtol (tok, &endp, 0);
> + tok = endp;
> if (tok == tmptok)
> error (_("Junk after thread keyword."));

I don't think we don't need two temporaries. The issue is
that the code passed "tok" for both input and output.

Seems like the simpler:

else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
{
char *tmptok;

tok = end_tok + 1;
- tmptok = tok;
- *thread = strtol (tok, &tok, 0);
+ *thread = strtol (tok, &tmptok, 0);
if (tok == tmptok)
error (_("Junk after thread keyword."));
+ tok = tmptok;
...

should work. See patchlet below (untested).
Fine with me to s/endp/tmptok/ if you want.

Same for "task".

> @@ -10845,12 +10850,12 @@ watch_command_1 (char *arg, int accessfl
> const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
> struct value *val, *mark, *result;
> struct frame_info *frame;
> - char *exp_start = NULL;
> - char *exp_end = NULL;
> - char *tok, *end_tok;
> + const char *exp_start = NULL;
> + const char *exp_end = NULL;
> + const char *tok, *end_tok;
> int toklen = -1;
> - char *cond_start = NULL;
> - char *cond_end = NULL;
> + const char *cond_start = NULL;
> + const char *cond_end = NULL;
> enum bptype bp_type;
> int thread = -1;
> int pc = 0;
> @@ -10859,15 +10864,19 @@ watch_command_1 (char *arg, int accessfl
> int use_mask = 0;
> CORE_ADDR mask = 0;
> struct watchpoint *w;
> + const char *tmp, *start, *end_arg, *cbuf;
> + char *buf;
>
> /* Make sure that we actually have parameters to parse. */
> if (arg != NULL && arg[0] != '\0')
> {
> - char *value_start;
> + const char *value_start;
> +
> + end_arg = arg + strlen (arg);
>
> /* Look for "parameter value" pairs at the end
> of the arguments string. */
> - for (tok = arg + strlen (arg) - 1; tok > arg; tok--)
> + for (tok = end_arg - 1; tok > arg; tok--)
> {
> /* Skip whitespace at the end of the argument list. */
> while (tok > arg && (*tok == ' ' || *tok == '\t'))
> @@ -10937,15 +10946,22 @@ watch_command_1 (char *arg, int accessfl
>
> /* Truncate the string and get rid of the "parameter value" pair before
> the arguments string is parsed by the parse_exp_1 function. */
> - *tok = '\0';
> + end_arg = tok;
> }
> }
> + else
> + end_arg = arg;
>
> /* Parse the rest of the arguments. */
> innermost_block = NULL;
> - exp_start = arg;
> - exp = parse_exp_1 (&arg, 0, 0, 0);
> - exp_end = arg;
> + buf = alloca (end_arg - arg + 1);
> + strncpy (buf, arg, end_arg - arg);
> + buf[end_arg - arg] = '\0';
> + exp_start = buf;
> + tmp = start = buf;
> + exp = parse_exp_1 (&tmp, 0, 0, 0);
> + cbuf = exp_end = exp_start + (tmp - start);
> +

I find this hard to grok. :-/ "tmp, buf, cbuf, I'm lost, argh."
I don't think we actually need all that hair. Using
savestring+cleanup makes this much clearer to me (see below).

Looking at this wondered something (not for this patch):

/* Remove trailing whitespace from the expression before saving it.
This makes the eventual display of the expression string a bit
prettier. */
while (exp_end > exp_start && (exp_end[-1] == ' ' || exp_end[-1] == '\t'))
--exp_end;

I thought parse_exp_1 stopped before any trailing whitespace?

/* Read an expression from the string *STRINGPTR points to,
parse it, and return a pointer to a struct expression that we malloc.
Use block BLOCK as the lexical context for variable names;
if BLOCK is zero, use the block of the selected stack frame.
Meanwhile, advance *STRINGPTR to point after the expression,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at the first nonwhite character that is not part of the expression
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(possibly a null character).

If COMMA is nonzero, stop if a comma is reached. */

struct expression *
parse_exp_1 (const char **stringptr, CORE_ADDR pc, const struct block *block,
int comma)

> if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
> {
> struct expression *cond;
> + const char *tmp, *start;
>
> innermost_block = NULL;
> tok = cond_start = end_tok + 1;
> - cond = parse_exp_1 (&tok, 0, 0, 0);
> + tmp = start = tok;
> + cond = parse_exp_1 (&tmp, 0, 0, 0);
> + tok += tmp - start;

This hunk looks completely unnecessary.

> --- tracepoint.c 7 Mar 2013 21:57:30 -0000 1.283
> +++ tracepoint.c 8 Mar 2013 00:52:44 -0000
> @@ -749,9 +749,13 @@ validate_actionline (char **line, struct
> tmp_p = p;
> for (loc = t->base.loc; loc; loc = loc->next)
> {
> - p = tmp_p;
> - exp = parse_exp_1 (&p, loc->address,
> + const char *q, *o;
> +
> + o = q = tmp_p;
> +
> + exp = parse_exp_1 (&q, loc->address,
> block_for_pc (loc->address), 1);
> + p += q - o;
> old_chain = make_cleanup (free_current_contents, &exp);
>
> if (exp->elts[0].opcode == OP_VAR_VALUE)

Argh all that extra pointer arithmetic hurts my eyes.

Do we really need it? Does this:

- exp = parse_exp_1 (&p, loc->address,
+ exp = parse_exp_1 ((const char **) &p, loc->address,

break strict aliasing rules? If not, that's all it takes, and we can
later on work on progressively removing these casts outwards. If it
does, then I'd much rather see all that pointer manipulation that
appears many times in the patch wrapped in a helper function
that looks like parse_exp_1 but still takes a char** ...

--
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fbb6c62..2afdd60 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9416,14 +9416,12 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
{
char *endp;
- const char *tmptok;

tok = end_tok + 1;
- tmptok = tok;
*thread = strtol (tok, &endp, 0);
- tok = endp;
- if (tok == tmptok)
+ if (endp == tok)
error (_("Junk after thread keyword."));
+ tok = endp;
if (!valid_thread_id (*thread))
invalid_thread_id_error (*thread);
}
@@ -10845,13 +10843,16 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
int just_location, int internal)
{
volatile struct gdb_exception e;
+ struct cleanup *old_chain;
struct breakpoint *b, *scope_breakpoint = NULL;
struct expression *exp;
const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
struct value *val, *mark, *result;
struct frame_info *frame;
+ const char *end_arg;
const char *exp_start = NULL;
const char *exp_end = NULL;
+ const char *exp_end_spaces;
const char *tok, *end_tok;
int toklen = -1;
const char *cond_start = NULL;
@@ -10864,8 +10865,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
int use_mask = 0;
CORE_ADDR mask = 0;
struct watchpoint *w;
- const char *tmp, *start, *end_arg, *cbuf;
- char *buf;

/* Make sure that we actually have parameters to parse. */
if (arg != NULL && arg[0] != '\0')
@@ -10954,13 +10953,13 @@ watch_command_1 (char *arg, int accessflag, int from_tty,

/* Parse the rest of the arguments. */
innermost_block = NULL;
- buf = alloca (end_arg - arg + 1);
- strncpy (buf, arg, end_arg - arg);
- buf[end_arg - arg] = '\0';
- exp_start = buf;
- tmp = start = buf;
- exp = parse_exp_1 (&tmp, 0, 0, 0);
- cbuf = exp_end = exp_start + (tmp - start);
+
+ exp_start = savestring (arg, end_arg - arg);
+ old_chain = make_cleanup (xfree, (char *) exp_start);
+
+ exp_end = exp_start;
+ exp = parse_exp_1 (&exp_end, 0, 0, 0);
+ exp_end_spaces = exp_end;

/* Remove trailing whitespace from the expression before saving it.
This makes the eventual display of the expression string a bit
@@ -11005,20 +11004,17 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
else if (val != NULL)
release_value (val);

- tok = skip_spaces_const (cbuf);
+ tok = skip_spaces_const (exp_end_spaces);
end_tok = skip_to_space_const (tok);

toklen = end_tok - tok;
if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
{
struct expression *cond;
- const char *tmp, *start;

innermost_block = NULL;
tok = cond_start = end_tok + 1;
- tmp = start = tok;
- cond = parse_exp_1 (&tmp, 0, 0, 0);
- tok += tmp - start;
+ cond = parse_exp_1 (&tok, 0, 0, 0);

/* The watchpoint expression may not be local, but the condition
may still be. E.g.: `watch global if local > 0'. */
@@ -11161,6 +11157,8 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
}

install_breakpoint (internal, b, 1);
+
+ do_cleanups (old_chain);
}

/* Return count of debug registers needed to watch the given expression.