[RFA] Constify find_condition_and_thread

[RFA] Constify find_condition_and_thread

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

Hi,

I'm getting ready to submit a rewrite of my explicit location patchset, 
and I found $SUBJECT to be necessary.

I admit, I am not 100% happy with this, but the use of parse_exp_1 
really complicates things. I made an attempt at "fixing" parse_exp_1, 
but that leads to an enormous maze of changes.

Comments?
Keith

ChangeLog
  2012-03-01  Keith Seitz  <keiths@redhat.com>

	* cli/cli-utils.c (skip_to_space_const): New function.
	* cli/cli-utils.h (skip_to_space_const): Declare.
	* breakpoint.c (find_condition_and_thread): Constify TOK
	and update function.

[-- Attachment #2: constify-find_condition_and_thread.patch --]
[-- Type: text/x-patch, Size: 3690 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb57a57..a1877e3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9374,7 +9374,7 @@ invalid_thread_id_error (int id)
    If no thread is found, *THREAD is set to -1.  */
 
 static void
-find_condition_and_thread (char *tok, CORE_ADDR pc,
+find_condition_and_thread (const char *tok, CORE_ADDR pc,
 			   char **cond_string, int *thread, int *task,
 			   char **rest)
 {
@@ -9385,12 +9385,12 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
 
   while (tok && *tok)
     {
-      char *end_tok;
+      const char *end_tok;
       int toklen;
-      char *cond_start = NULL;
-      char *cond_end = NULL;
+      const char *cond_start = NULL;
+      const char *cond_end = NULL;
 
-      tok = skip_spaces (tok);
+      tok = skip_spaces_const (tok);
 
       if ((*tok == '"' || *tok == ',') && rest)
 	{
@@ -9398,41 +9398,57 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
 	  return;
 	}
 
-      end_tok = skip_to_space (tok);
+      end_tok = skip_to_space_const (tok);
 
       toklen = end_tok - tok;
 
       if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
 	{
+	  char *copy, *orig;
+	  struct cleanup *cleanup;
 	  struct expression *expr;
 
 	  tok = cond_start = end_tok + 1;
-	  expr = parse_exp_1 (&tok, pc, block_for_pc (pc), 0);
+	  orig = copy = xstrdup (tok);
+	  cleanup = make_cleanup (xfree, orig);
+	  expr = parse_exp_1 (&copy, pc, block_for_pc (pc), 0);
 	  xfree (expr);
+	  tok += copy - orig;
+	  do_cleanups (cleanup);
 	  cond_end = tok;
 	  *cond_string = savestring (cond_start, cond_end - cond_start);
 	}
       else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
 	{
-	  char *tmptok;
+	  char *tmptok, *copy;
 
 	  tok = end_tok + 1;
-	  tmptok = tok;
-	  *thread = strtol (tok, &tok, 0);
-	  if (tok == tmptok)
-	    error (_("Junk after thread keyword."));
+	  tmptok = copy = xstrdup (tok);
+	  *thread = strtol (copy, &copy, 0);
+	  if (copy == tmptok)
+	    {
+	      xfree (tmptok);
+	      error (_("Junk after thread keyword."));
+	    }
+	  tok += copy - tmptok;
+	  xfree (tmptok);
 	  if (!valid_thread_id (*thread))
 	    invalid_thread_id_error (*thread);
 	}
       else if (toklen >= 1 && strncmp (tok, "task", toklen) == 0)
 	{
-	  char *tmptok;
+	  char *tmptok, *copy;
 
 	  tok = end_tok + 1;
-	  tmptok = tok;
-	  *task = strtol (tok, &tok, 0);
-	  if (tok == tmptok)
-	    error (_("Junk after task keyword."));
+	  tmptok = copy = xstrdup (tok);
+	  *task = strtol (copy, &copy, 0);
+	  if (copy == tmptok)
+	    {
+	      xfree (tmptok);
+	      error (_("Junk after task keyword."));
+	    }
+	  tok += copy - tmptok;
+	  xfree (tmptok);
 	  if (!valid_task_id (*task))
 	    error (_("Unknown task %d."), *task);
 	}
diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index 933fb89..844dee4 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -247,6 +247,18 @@ skip_to_space (char *chp)
   return chp;
 }
 
+/* A const-correct version of the above.  */
+
+const char *
+skip_to_space_const (const char *chp)
+{
+  if (chp == NULL)
+    return NULL;
+  while (*chp && !isspace (*chp))
+    chp++;
+  return chp;
+}
+
 /* See documentation in cli-utils.h.  */
 
 char *
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index 6f28e13..ae8ca7b 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -103,6 +103,10 @@ extern const char *skip_spaces_const (const char *inp);
 
 extern char *skip_to_space (char *inp);
 
+/* A const-correct version of the above.  */
+
+extern const char *skip_to_space_const (const char *inp);
+
 /* Reverse S to the last non-whitespace character without skipping past
    START.  */
 

Re: [RFA] Constify find_condition_and_thread

[Eli Zaretskii]

> Date: Fri, 01 Mar 2013 10:18:37 -0800
> From: Keith Seitz <keiths@redhat.com>
> 
> -	  expr = parse_exp_1 (&tok, pc, block_for_pc (pc), 0);
> +	  orig = copy = xstrdup (tok);
> +	  cleanup = make_cleanup (xfree, orig);
> +	  expr = parse_exp_1 (&copy, pc, block_for_pc (pc), 0);
>  	  xfree (expr);
> +	  tok += copy - orig;
> +	  do_cleanups (cleanup);

If we really need this kind of dance, just to avoid explicit casts to
'char *', there should be a comment explaining why we do this.

Thanks.

Re: [RFA] Constify find_condition_and_thread

[Pedro Alves]

On 03/01/2013 06:40 PM, Eli Zaretskii wrote:
>> Date: Fri, 01 Mar 2013 10:18:37 -0800
>> From: Keith Seitz <keiths@redhat.com>
>>
>> -	  expr = parse_exp_1 (&tok, pc, block_for_pc (pc), 0);
>> +	  orig = copy = xstrdup (tok);
>> +	  cleanup = make_cleanup (xfree, orig);
>> +	  expr = parse_exp_1 (&copy, pc, block_for_pc (pc), 0);
>>  	  xfree (expr);
>> +	  tok += copy - orig;
>> +	  do_cleanups (cleanup);
> 
> If we really need this kind of dance, just to avoid explicit casts to
> 'char *', there should be a comment explaining why we do this.

Yeah.  I think casting the const out would be okay, and
better than forcing a xstrdup. (run time cost, and extra
complication for not that much gain over cast).

An alternative would be to perhaps make the interface of
parse_exp_1 similar to strtol -- split input and output
pointers, with the explicit guarantee that the output pointer
points somewhere within the input string.  Is it really that
messy?  There doesn't seem to be many parse_exp_1 callers.

-- 
Pedro Alves

Re: [RFA] Constify find_condition_and_thread

[Pedro Alves]

On 03/01/2013 06:18 PM, Keith Seitz wrote:
>        else if (toklen >= 1 && strncmp (tok, "task", toklen) == 0)
>  	{
> -	  char *tmptok;
> +	  char *tmptok, *copy;
>  
>  	  tok = end_tok + 1;
> -	  tmptok = tok;
> -	  *task = strtol (tok, &tok, 0);
> -	  if (tok == tmptok)
> -	    error (_("Junk after task keyword."));
> +	  tmptok = copy = xstrdup (tok);
> +	  *task = strtol (copy, &copy, 0);
> +	  if (copy == tmptok)
> +	    {
> +	      xfree (tmptok);
> +	      error (_("Junk after task keyword."));
> +	    }

Why do we really need the xstrdups in this and the "task" cases?

> +const char *
> +skip_to_space_const (const char *chp)
> +{
> +  if (chp == NULL)
> +    return NULL;
> +  while (*chp && !isspace (*chp))
> +    chp++;
> +  return chp;
> +}

Note we in addition redo skip_to_space as:

char *
skip_to_space (char *chp)
{
  return (char *) skip_to_space_const (chp);
}

(or make that an inline function / #define)
-- 
Pedro Alves

Re: [RFA] Constify find_condition_and_thread

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> An alternative would be to perhaps make the interface of
Pedro> parse_exp_1 similar to strtol -- split input and output
Pedro> pointers, with the explicit guarantee that the output pointer
Pedro> points somewhere within the input string.  Is it really that
Pedro> messy?  There doesn't seem to be many parse_exp_1 callers.

I think the problem is that the parsers probably do modify the input
string.  So, you couldn't pass in a readonly string.

E.g., see c-exp.y:parse_number.  It modifies the string, but then
modifies it back.

Tom

Re: [RFA] Constify find_condition_and_thread

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Keith Seitz <keiths@redhat.com>,        gdb-patches@sourceware.org
> Date: Fri, 01 Mar 2013 12:33:46 -0700
> 
> I think the problem is that the parsers probably do modify the input
> string.  So, you couldn't pass in a readonly string.

I figured that much.  But if that's the problem, perhaps the caller
should xstrdup the original string and pass a modifiable one to its
callees, instead of forcing low-level subroutines do that.