[rfc] Prompt memory management/cleanups

[rfc] Prompt memory management/cleanups

[Phil Muldoon]

While working on the Python patch to allow prompt substitution in
Python, I noticed that in some cases prompts were leaking memory.  In
several cases, scenarios like: PROMPT (0) = xstrdup(someprompt), did not
xfree the prompt first.  I decided to use functions calls to replace
access to PROMPT/SUFFIX/PREFIX so that memory management can be
performed centrally.  Attached is a patch.  I'd appreciate comments on
this; the testsuite shows no regressions, but my knowledge of the prompt
area of GDB is about three days old. I'd especially like comments on the
acceptance on NULL in the set_prompt/prefix/suffix functions.  Pop/push
prompt uses different prompt levels and requires that, after use, they
are cleaned.  It normally just does this with xfree.  I'm unsure if my
API for that scenario is preferable.

Cheers

Phil

--

2011-07-20  Phil Muldoon  <pmuldoon@redhat.com>

	* event-top.c (cli_command_loop): Use get_prompt, get_suffix,
	get_prefix.
	(display_gdb_prompt): Likewise.
	(change_annotation_level): Likewise.
	(push_prompt): Likewise.
	(pop_prompt): Likewise.
	(handle_stop_sig): Use get_prompt with a level.
	* top.c (command_loop): Use get_prompt with a level.
	(set_async_annotation_level): Use set_prompt with a level.
	(get_prefix): New function.
	(set_prefix): Ditto.
	(set_suffix): Ditto.
	(get_suffix): Ditto.
	(get_prompt): Accept a level argument.
	(set_prompt): Accept a level argument.  Account for NULL prompts.
	Free old prompts.  Set new_async_prompt if level is 0.
	(init_main): Use set_prompt with a level.  Do not set
	new_async_prompt.
	* top.h: Declare set_suffix, get_suffix, set_prefix, get_prefix.
	Modify set_prompt, get_prompt to account for levels.
	* tui/tui-interp.c (tui_command_loop): Use get_prompt with a
	level.


--

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 72cbfdc..3546ac0 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -190,17 +190,17 @@ cli_command_loop (void)
     {
       int length;
       char *a_prompt;
-      char *gdb_prompt = get_prompt ();
+      char *gdb_prompt = get_prompt (0);
 
       /* Tell readline what the prompt to display is and what function
          it will need to call after a whole line is read.  This also
          displays the first prompt.  */
-      length = strlen (PREFIX (0)) 
-	+ strlen (gdb_prompt) + strlen (SUFFIX (0)) + 1;
+      length = strlen (get_prefix (0))
+	+ strlen (gdb_prompt) + strlen (get_suffix(0)) + 1;
       a_prompt = (char *) alloca (length);
-      strcpy (a_prompt, PREFIX (0));
+      strcpy (a_prompt, get_prefix (0));
       strcat (a_prompt, gdb_prompt);
-      strcat (a_prompt, SUFFIX (0));
+      strcat (a_prompt, get_suffix (0));
       rl_callback_handler_install (a_prompt, input_handler);
     }
   else
@@ -258,7 +258,7 @@ void
 display_gdb_prompt (char *new_prompt)
 {
   int prompt_length = 0;
-  char *gdb_prompt = get_prompt ();
+  char *gdb_prompt = get_prompt (0);
 
   /* Reset the nesting depth used when trace-commands is set.  */
   reset_command_nest_depth ();
@@ -291,19 +291,21 @@ display_gdb_prompt (char *new_prompt)
 
   if (!new_prompt)
     {
       /* Just use the top of the prompt stack.  */
-      prompt_length = strlen (PREFIX (0)) +
-	strlen (SUFFIX (0)) +
+      prompt_length = strlen (get_prefix (0)) +
+	strlen (get_suffix (0)) +
 	strlen (gdb_prompt) + 1;
 
       new_prompt = (char *) alloca (prompt_length);
 
       /* Prefix needs to have new line at end.  */
-      strcpy (new_prompt, PREFIX (0));
+      strcpy (new_prompt, get_prefix (0));
       strcat (new_prompt, gdb_prompt);
       /* Suffix needs to have a new line at end and \032 \032 at
          beginning.  */
-      strcat (new_prompt, SUFFIX (0));
+      strcat (new_prompt, get_suffix (0));
     }
 
   if (async_command_editing_p)
@@ -333,7 +335,7 @@ change_annotation_level (void)
 {
   char *prefix, *suffix;
 
-  if (!PREFIX (0) || !PROMPT (0) || !SUFFIX (0))
+  if (!get_prefix (0) || !get_prompt (0) || !get_suffix (0))
     {
       /* The prompt stack has not been initialized to "", we are
          using gdb w/o the --async switch.  */
@@ -343,7 +345,7 @@ change_annotation_level (void)
 
   if (annotation_level > 1)
     {
-      if (!strcmp (PREFIX (0), "") && !strcmp (SUFFIX (0), ""))
+      if (!strcmp (get_prefix (0), "") && !strcmp (get_suffix (0), ""))
 	{
 	  /* Push a new prompt if the previous annotation_level was not >1.  */
 	  prefix = (char *) alloca (strlen (async_annotation_suffix) + 10);
@@ -361,7 +363,7 @@ change_annotation_level (void)
     }
   else
     {
-      if (strcmp (PREFIX (0), "") && strcmp (SUFFIX (0), ""))
+      if (strcmp (get_prefix (0), "") && strcmp (get_suffix (0), ""))
 	{
 	  /* Pop the top of the stack, we are going back to annotation < 1.  */
 	  pop_prompt ();
@@ -377,17 +379,17 @@ void
 push_prompt (char *prefix, char *prompt, char *suffix)
 {
   the_prompts.top++;
-  PREFIX (0) = xstrdup (prefix);
+  set_prefix (prefix, 0);
 
   /* Note that this function is used by the set annotate 2
      command.  This is why we take care of saving the old prompt
      in case a new one is not specified.  */
   if (prompt)
-    PROMPT (0) = xstrdup (prompt);
+    set_prompt (prompt, 0);
   else
-    PROMPT (0) = xstrdup (PROMPT (-1));
+    set_prompt (get_prompt (-1), 0);
 
-  SUFFIX (0) = xstrdup (suffix);
+  set_suffix (suffix, 0);
 }
 
 /* Pops the top of the prompt stack, and frees the memory allocated
@@ -397,20 +399,17 @@ pop_prompt (void)
 {
   /* If we are not during a 'synchronous' execution command, in which
      case, the top prompt would be empty.  */
-  if (strcmp (PROMPT (0), ""))
+  if (strcmp (get_prompt (0), ""))
     /* This is for the case in which the prompt is set while the
        annotation level is 2.  The top prompt will be changed, but when
        we return to annotation level < 2, we want that new prompt to be
        in effect, until the user does another 'set prompt'.  */
-    if (strcmp (PROMPT (0), PROMPT (-1)))
-      {
-	xfree (PROMPT (-1));
-	PROMPT (-1) = xstrdup (PROMPT (0));
-      }
-
-  xfree (PREFIX (0));
-  xfree (PROMPT (0));
-  xfree (SUFFIX (0));
+    if (strcmp (get_prompt (0), get_prompt (-1)))
+      set_prompt (get_prompt (0), -1);
+
+  set_prefix (NULL, 0);
+  set_prompt (NULL, 0);
+  set_suffix (NULL, 0);
   the_prompts.top--;
 }
 
@@ -955,7 +954,7 @@ handle_stop_sig (int sig)
 static void
 async_stop_sig (gdb_client_data arg)
 {
-  char *prompt = get_prompt ();
+  char *prompt = get_prompt (0);
 
 #if STOP_SIGNAL == SIGTSTP
   signal (SIGTSTP, SIG_DFL);
@@ -1033,7 +1032,7 @@ set_async_annotation_level (char *args, int from_tty,
 void
 set_async_prompt (char *args, int from_tty, struct cmd_list_element *c)
 {
-  PROMPT (0) = xstrdup (new_async_prompt);
+  set_prompt (new_async_prompt, 0);
 }
 
 /* Set things up for readline to be invoked via the alternate
diff --git a/gdb/top.c b/gdb/top.c
index ecb91f2..b0ac474 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -533,7 +533,7 @@ command_loop (void)
   while (instream && !feof (instream))
     {
       if (window_hook && instream == stdin)
-	(*window_hook) (instream, get_prompt ());
+	(*window_hook) (instream, get_prompt (0));
 
       quit_flag = 0;
       if (instream == stdin && stdin_is_tty)
@@ -542,7 +542,7 @@ command_loop (void)
 
       /* Get a command-line.  This calls the readline package.  */
       command = command_line_input (instream == stdin ?
-				    get_prompt () : (char *) NULL,
+				    get_prompt (0) : (char *) NULL,
 				    instream == stdin, "prompt");
       if (command == 0)
 	{
@@ -1124,24 +1124,104 @@ and \"show warranty\" for details.\n");
     }
 }
 \f
+
+
+/* get_prefix: access method for the GDB prefix string.  */
+
+char *
+get_prefix (int level)
+{
+  return PREFIX (level);
+}
+
+/* set_prefix: set method for the GDB prefix string.  */
+
+void
+set_prefix (const char *s, int level)
+{
+  /* If S is NULL, just free the PREFIX at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (PREFIX (level));
+      PREFIX (level) = NULL;
+    }
+  else
+  /* If S == PREFIX then do not free it or set it.  If we did
+     that, S (which points to PREFIX), would become garbage.  */
+    if (s != PREFIX (level))
+      {
+	xfree (PREFIX (level));
+	PREFIX (level) =  xstrdup (s);
+      }
+}
+
+/* get_suffix: access method for the GDB suffix string.  */
+
+char *
+get_suffix (int level)
+{
+  return SUFFIX (level);
+}
+
+/* set_suffix: set method for the GDB suffix string.  */
+
+void
+set_suffix (const char *s, int level)
+{
+  /* If S is NULL, just free the SUFFIX at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (SUFFIX (level));
+      SUFFIX (level) = NULL;
+    }
+  else
+    /* If S == SUFFIX then do not free it or set it.  If we did
+       that, S (which points to SUFFIX), would become garbage.  */
+    if (s != SUFFIX (level))
+      {
+	xfree (SUFFIX (level));
+	SUFFIX (level) =  xstrdup (s);
+      }
+}
+
 /* get_prompt: access method for the GDB prompt string.  */
 
 char *
-get_prompt (void)
+get_prompt (int level)
 {
-  return PROMPT (0);
+  return PROMPT (level);
 }
 
 void
-set_prompt (char *s)
+set_prompt (const char *s, int level)
 {
-/* ??rehrauer: I don't know why this fails, since it looks as though
-   assignments to prompt are wrapped in calls to xstrdup...
-   if (prompt != NULL)
-     xfree (prompt);
- */
-  PROMPT (0) = xstrdup (s);
+  /* If S is NULL, just free the PROMPT at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (PROMPT (level));
+      PROMPT (level) = NULL;
+    }
+  else
+    /* If S == PROMPT then do not free it or set it.  If we did
+       that, S (which points to PROMPT), would become garbage.  */
+    if (s != PROMPT (level))
+      {
+	xfree (PROMPT (level));
+	PROMPT (level) =  xstrdup (s);
+
+	if (level == 0)
+	  {
+	  /* Also, free and set new_async_prompt so prompt changes sync up
+	     with set/show prompt.  */
+	    xfree (new_async_prompt);
+	    new_async_prompt = xstrdup (PROMPT (level));
+	  }
+      }
 }
+
 \f
 
 struct qt_args
@@ -1531,13 +1611,11 @@ init_main (void)
      whatever the DEFAULT_PROMPT is.  */
   the_prompts.top = 0;
   PREFIX (0) = "";
-  PROMPT (0) = xstrdup (DEFAULT_PROMPT);
+  set_prompt (DEFAULT_PROMPT, 0);
   SUFFIX (0) = "";
   /* Set things up for annotation_level > 1, if the user ever decides
      to use it.  */
   async_annotation_suffix = "prompt";
-  /* Set the variable associated with the setshow prompt command.  */
-  new_async_prompt = xstrdup (PROMPT (0));
 
   /* If gdb was started with --annotate=2, this is equivalent to the
      user entering the command 'set annotate 2' at the gdb prompt, so
diff --git a/gdb/top.h b/gdb/top.h
index 4a6e8fb..bdc29c0 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -51,11 +51,27 @@ extern struct cleanup *prepare_execute_command (void);
 
 /* This function returns a pointer to the string that is used
    by gdb for its command prompt.  */
-extern char *get_prompt (void);
+extern char *get_prompt (int);
 
 /* This function copies the specified string into the string that
    is used by gdb for its command prompt.  */
-extern void set_prompt (char *);
+extern void set_prompt (const char *, int);
+
+/* This function returns a pointer to the string that is used
+   by gdb for its command prompt prefix.  */
+extern char *get_prefix (int);
+
+/* This function copies the specified string into the string that
+   is used by gdb for its command prompt prefix.  */
+extern void set_prefix (const char *, int);
+
+/* This function returns a pointer to the string that is used
+   by gdb for its command prompt suffix.  */
+extern char *get_suffix (int);
+
+/* This function copies the specified string into the string that
+   is used by gdb for its command prompt suffix.  */
+extern void set_suffix (const char *, int);
 
 /* From random places.  */
 extern int readnow_symbol_files;
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index d79de2b..5ed692a 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -145,7 +145,7 @@ tui_command_loop (void *data)
     {
       int length;
       char *a_prompt;
-      char *gdb_prompt = get_prompt ();
+      char *gdb_prompt = get_prompt (0);
 
       /* Tell readline what the prompt to display is and what function
          it will need to call after a whole line is read. This also

Re: [rfc] Prompt memory management/cleanups

[Pedro Alves]

On Wednesday 20 July 2011 14:05:11, Phil Muldoon wrote:
> +set_prompt (const char *s, int level)
>  {
> -/* ??rehrauer: I don't know why this fails, since it looks as though
> -   assignments to prompt are wrapped in calls to xstrdup...
> -   if (prompt != NULL)
> -     xfree (prompt);
> - */
> -  PROMPT (0) = xstrdup (s);
> +  /* If S is NULL, just free the PROMPT at level LEVEL and set to
> +     NULL.  */
> +  if (s == NULL)
> +    {
> +      xfree (PROMPT (level));
> +      PROMPT (level) = NULL;
> +    }


> +  else
> +    /* If S == PROMPT then do not free it or set it.  If we did
> +       that, S (which points to PROMPT), would become garbage.  */
> +    if (s != PROMPT (level))
> +      {

This looks strange, and I suppose complicates the callers' life a bit,
having to know when are they giving up ownership of the string
or not.  What would need to change at the callers if we dropped
that s != PROMPT() check?

-- 
Pedro Alves

Re: [rfc] Prompt memory management/cleanups

[Phil Muldoon]

Pedro Alves <pedro@codesourcery.com> writes:

> On Wednesday 20 July 2011 14:05:11, Phil Muldoon wrote:
>> +set_prompt (const char *s, int level)
>>  {
>> -/* ??rehrauer: I don't know why this fails, since it looks as though
>> -   assignments to prompt are wrapped in calls to xstrdup...
>> -   if (prompt != NULL)
>> -     xfree (prompt);
>> - */
>> -  PROMPT (0) = xstrdup (s);
>> +  /* If S is NULL, just free the PROMPT at level LEVEL and set to
>> +     NULL.  */
>> +  if (s == NULL)
>> +    {
>> +      xfree (PROMPT (level));
>> +      PROMPT (level) = NULL;
>> +    }
>
>
>> +  else
>> +    /* If S == PROMPT then do not free it or set it.  If we did
>> +       that, S (which points to PROMPT), would become garbage.  */
>> +    if (s != PROMPT (level))
>> +      {
>
> This looks strange, and I suppose complicates the callers' life a bit,
> having to know when are they giving up ownership of the string
> or not.  What would need to change at the callers if we dropped
> that s != PROMPT() check?

This is a fail-safe for the Python prompt substitution patch.  So if they
did (the equivalent in Python):

s = get_prompt (0)
set_prompt (s, 0)

Without that check, 'PROMPT (level)' would be freed, but 's' points to
that.  So you set garbage.   get_prompt returns a pointer, not a copy.

Internal uses of set_prompt we can just do the checks wherever we want.
Basically checking if your pointer to the prompt is in-fact the current
prompt.  I thought about doing this at the callers, but it just
replicated the code everywhere, and in static (IE internal GDB code) that
seemed pointless.  We could just xstrdup s, then free the prompt anyway, and
then set it.  But then the s they passed to the function, will still be
pointing to xfree'd space; I am not sure if that matters.  This seemed
the safest way, and not allow prompts to become corrupted.  I am
completely open to changing it if there is a better way.

Cheers,

Phil

Re: [rfc] Prompt memory management/cleanups

[Pedro Alves]

On Wednesday 20 July 2011 15:30:19, Phil Muldoon wrote:
> s = get_prompt (0)
> set_prompt (s, 0)
> 
> Without that check, 'PROMPT (level)' would be freed, but 's' points to
> that.  So you set garbage.   get_prompt returns a pointer, not a copy.

I'm probably missing something, but isn't it just
a matter of instead of having:

+       xfree (PROMPT (level));
+       PROMPT (level) =  xstrdup (s);

you have:

+		 char *newp = xstrdup (s);
+       xfree (PROMPT (level));
+       PROMPT (level) = newp;

?

-- 
Pedro Alves

Re: [rfc] Prompt memory management/cleanups

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> While working on the Python patch to allow prompt substitution in
Phil> Python, I noticed that in some cases prompts were leaking memory.  In
Phil> several cases, scenarios like: PROMPT (0) = xstrdup(someprompt), did not
Phil> xfree the prompt first.  I decided to use functions calls to replace
Phil> access to PROMPT/SUFFIX/PREFIX so that memory management can be
Phil> performed centrally.  Attached is a patch.  I'd appreciate comments on
Phil> this; the testsuite shows no regressions, but my knowledge of the prompt
Phil> area of GDB is about three days old. I'd especially like comments on the
Phil> acceptance on NULL in the set_prompt/prefix/suffix functions.  Pop/push
Phil> prompt uses different prompt levels and requires that, after use, they
Phil> are cleaned.  It normally just does this with xfree.  I'm unsure if my
Phil> API for that scenario is preferable.

The patch seems fine, but I have a few questions.

Does anything ever set the prefix or suffix to a non-empty value?
I couldn't find an instance.  So how about just removing all this stuff
instead?

Do we still need the PROMPT, PREFIX, and SUFFIX defines in event-top.h?

Tom

Re: [rfc] Prompt memory management/cleanups

[Phil Muldoon]

Pedro Alves <pedro@codesourcery.com> writes:

> On Wednesday 20 July 2011 15:30:19, Phil Muldoon wrote:
>> s = get_prompt (0)
>> set_prompt (s, 0)
>> 
>> Without that check, 'PROMPT (level)' would be freed, but 's' points to
>> that.  So you set garbage.   get_prompt returns a pointer, not a copy.
>
> I'm probably missing something, but isn't it just
> a matter of instead of having:
>
> +       xfree (PROMPT (level));
> +       PROMPT (level) =  xstrdup (s);
>
> you have:
>
> +		 char *newp = xstrdup (s);
> +       xfree (PROMPT (level));
> +       PROMPT (level) = newp;
>
> ?

Yeah I noted we could do that in my reply.  Sure we can do that, I'm not
opposed to it.  But I am not sure on your objection to the check we make
first instead of the xstrdup?  If PROMPT (level) == s, then there is no
need to copy the contents of s into PROMPT, it is already there?  The
user is effectively asking for a noop?

Cheers,

Phil

Re: [rfc] Prompt memory management/cleanups

[Pedro Alves]

On Wednesday 20 July 2011 16:04:16, Phil Muldoon wrote:
> Pedro Alves <pedro@codesourcery.com> writes:
> 
> > On Wednesday 20 July 2011 15:30:19, Phil Muldoon wrote:
> >> s = get_prompt (0)
> >> set_prompt (s, 0)
> >> 
> >> Without that check, 'PROMPT (level)' would be freed, but 's' points to
> >> that.  So you set garbage.   get_prompt returns a pointer, not a copy.
> >
> > I'm probably missing something, but isn't it just
> > a matter of instead of having:
> >
> > +       xfree (PROMPT (level));
> > +       PROMPT (level) =  xstrdup (s);
> >
> > you have:
> >
> > +		 char *newp = xstrdup (s);
> > +       xfree (PROMPT (level));
> > +       PROMPT (level) = newp;
> >
> > ?
> 
> Yeah I noted we could do that in my reply.  Sure we can do that, I'm not
> opposed to it.  But I am not sure on your objection to the check we make
> first instead of the xstrdup?  If PROMPT (level) == s, then there is no
> need to copy the contents of s into PROMPT, it is already there?  The
> user is effectively asking for a noop?

You've asked for comments on the API, and IMO this makes for
a weird API, because the caller of set_prompt needs to know
whether set_prompt will take ownership of the pointer or not
depending on where the pointer came from.  I haven't looked
at the callers -- that's why I asked what would need to
change.  :-)

-- 
Pedro Alves

Re: [rfc] Prompt memory management/cleanups

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> While working on the Python patch to allow prompt substitution in
> Phil> Python, I noticed that in some cases prompts were leaking memory.  In
> Phil> several cases, scenarios like: PROMPT (0) = xstrdup(someprompt), did not
> Phil> xfree the prompt first.  I decided to use functions calls to replace
> Phil> access to PROMPT/SUFFIX/PREFIX so that memory management can be
> Phil> performed centrally.  Attached is a patch.  I'd appreciate comments on
> Phil> this; the testsuite shows no regressions, but my knowledge of the prompt
> Phil> area of GDB is about three days old. I'd especially like comments on the
> Phil> acceptance on NULL in the set_prompt/prefix/suffix functions.  Pop/push
> Phil> prompt uses different prompt levels and requires that, after use, they
> Phil> are cleaned.  It normally just does this with xfree.  I'm unsure if my
> Phil> API for that scenario is preferable.
>
> The patch seems fine, but I have a few questions.
>
> Does anything ever set the prefix or suffix to a non-empty value?
> I couldn't find an instance.  So how about just removing all this stuff
> instead?

The only use I can find is "", which may have meaning to the annotations
stuff.  This seems unlikely to me too.  If we think nobody uses it or is
likely to use it in the future, we can declare those elements dead, and
just remove them?

> Do we still need the PROMPT, PREFIX, and SUFFIX defines in event-top.h?

No.  They should not be accessible except from top.c and event-top.c
anymore. So I will remove them.

Cheers,

Phil

Re: [rfc] Prompt memory management/cleanups

[Tom Tromey]

Phil> The only use I can find is "", which may have meaning to the annotations
Phil> stuff.  This seems unlikely to me too.  If we think nobody uses it or is
Phil> likely to use it in the future, we can declare those elements dead, and
Phil> just remove them?

Yes, zap them.

Tom

Re: [rfc] Prompt memory management/cleanups

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> You've asked for comments on the API, and IMO this makes for
Pedro> a weird API, because the caller of set_prompt needs to know
Pedro> whether set_prompt will take ownership of the pointer or not
Pedro> depending on where the pointer came from.  I haven't looked
Pedro> at the callers -- that's why I asked what would need to
Pedro> change.  :-)

The problem case is set_prompt(get_prompt()), but here the prompt code
already owns the pointer.  I guess it is a little weird, but it still
falls under the general rule of "you have to call get_prompt again to
get the prompt after set_prompt".

Tom

Re: [rfc] Prompt memory management/cleanups

[Pedro Alves]

On Wednesday 20 July 2011 16:20:57, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> You've asked for comments on the API, and IMO this makes for
> Pedro> a weird API, because the caller of set_prompt needs to know
> Pedro> whether set_prompt will take ownership of the pointer or not
> Pedro> depending on where the pointer came from.  I haven't looked
> Pedro> at the callers -- that's why I asked what would need to
> Pedro> change.  :-)
> 
> The problem case is set_prompt(get_prompt()), but here the prompt code
> already owns the pointer.  

I don't think that would change if you make set_prompt xstrdup
before xfree.

> I guess it is a little weird, but it still
> falls under the general rule of "you have to call get_prompt again to
> get the prompt after set_prompt".

The current code allows:

 p = get_prompt(0);
 set_prompt(p, 0);
 // p is still valid here

whereas:

 p = get_prompt(0);
 set_prompt("foo", 0);
 // p is invalid here

But if we already have that rule, then we can treat the
former as undefined.

I think we should still xstrdup before xfree though as
a general rule in these sort of scenarios.  Otherwise:

 p = get_prompt(0);  (p's len being > 0)
 set_prompt(p + 1, 0);

will end up with garbage.

-- 
Pedro Alves

Re: [rfc] Prompt memory management/cleanups

[Tom Tromey]

Pedro> I think we should still xstrdup before xfree though as
Pedro> a general rule in these sort of scenarios.

Yeah.

Tom

Re: [rfc] Prompt memory management/cleanups

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> While working on the Python patch to allow prompt substitution in
> Phil> Python, I noticed that in some cases prompts were leaking memory.  In
> Phil> several cases, scenarios like: PROMPT (0) = xstrdup(someprompt), did not
> Phil> xfree the prompt first.  I decided to use functions calls to replace
> Phil> access to PROMPT/SUFFIX/PREFIX so that memory management can be
> Phil> performed centrally.  Attached is a patch.  I'd appreciate comments on
> Phil> this; the testsuite shows no regressions, but my knowledge of the prompt
> Phil> area of GDB is about three days old. I'd especially like comments on the
> Phil> acceptance on NULL in the set_prompt/prefix/suffix functions.  Pop/push
> Phil> prompt uses different prompt levels and requires that, after use, they
> Phil> are cleaned.  It normally just does this with xfree.  I'm unsure if my
> Phil> API for that scenario is preferable.
>
> The patch seems fine, but I have a few questions.
>
> Does anything ever set the prefix or suffix to a non-empty value?
> I couldn't find an instance.  So how about just removing all this stuff
> instead?

Well it turns out we do. change_annotation_level inserts valid prefix
and suffixes.  So I abandoned that rewrite.

> Do we still need the PROMPT, PREFIX, and SUFFIX defines in event-top.h?

I have moved them to top.c

I have also incorporated Pedro's comments in the patch.

The ChangeLog remains the same as the initial patch.

OK?

Cheers,

Phil

--

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 22f9440..37882728 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -191,17 +191,17 @@ cli_command_loop (void)
     {
       int length;
       char *a_prompt;
-      char *gdb_prompt = get_prompt ();
+      char *gdb_prompt = get_prompt (0);
 
       /* Tell readline what the prompt to display is and what function
          it will need to call after a whole line is read.  This also
          displays the first prompt.  */
-      length = strlen (PREFIX (0)) 
-	+ strlen (gdb_prompt) + strlen (SUFFIX (0)) + 1;
+      length = strlen (get_prefix (0))
+	+ strlen (gdb_prompt) + strlen (get_suffix(0)) + 1;
       a_prompt = (char *) alloca (length);
-      strcpy (a_prompt, PREFIX (0));
+      strcpy (a_prompt, get_prefix (0));
       strcat (a_prompt, gdb_prompt);
-      strcat (a_prompt, SUFFIX (0));
+      strcat (a_prompt, get_suffix (0));
       rl_callback_handler_install (a_prompt, input_handler);
     }
   else
@@ -276,10 +276,10 @@ display_gdb_prompt (char *new_prompt)
   if (! new_prompt)
     {
       char *post_gdb_prompt = NULL;
-      char *pre_gdb_prompt = xstrdup (get_prompt ());
+      char *pre_gdb_prompt = xstrdup (get_prompt (0));
 
       observer_notify_before_prompt (pre_gdb_prompt);
-      post_gdb_prompt = get_prompt ();
+      post_gdb_prompt = get_prompt (0);
 
       /* If the observer changed the prompt, use that prompt.  */
       if (strcmp (pre_gdb_prompt, post_gdb_prompt) != 0)
@@ -317,18 +317,18 @@ display_gdb_prompt (char *new_prompt)
       if (! new_prompt)
 	{
 	  /* Just use the top of the prompt stack.  */
-	  prompt_length = strlen (PREFIX (0)) +
-	    strlen (SUFFIX (0)) +
-	    strlen (get_prompt()) + 1;
+	  prompt_length = strlen (get_prefix (0)) +
+	    strlen (get_suffix (0)) +
+	    strlen (get_prompt (0)) + 1;
 
 	  actual_gdb_prompt = (char *) alloca (prompt_length);
 
 	  /* Prefix needs to have new line at end.  */
-	  strcpy (actual_gdb_prompt, PREFIX (0));
-	  strcat (actual_gdb_prompt, get_prompt());
+	  strcpy (actual_gdb_prompt, get_prefix (0));
+	  strcat (actual_gdb_prompt, get_prompt (0));
 	  /* Suffix needs to have a new line at end and \032 \032 at
 	     beginning.  */
-	  strcat (actual_gdb_prompt, SUFFIX (0));
+	  strcat (actual_gdb_prompt, get_suffix (0));
 	}
       else
 	actual_gdb_prompt = new_prompt;;
@@ -361,7 +361,7 @@ change_annotation_level (void)
 {
   char *prefix, *suffix;
 
-  if (!PREFIX (0) || !PROMPT (0) || !SUFFIX (0))
+  if (!get_prefix (0) || !get_prompt (0) || !get_suffix (0))
     {
       /* The prompt stack has not been initialized to "", we are
          using gdb w/o the --async switch.  */
@@ -371,7 +371,7 @@ change_annotation_level (void)
 
   if (annotation_level > 1)
     {
-      if (!strcmp (PREFIX (0), "") && !strcmp (SUFFIX (0), ""))
+      if (!strcmp (get_prefix (0), "") && !strcmp (get_suffix (0), ""))
 	{
 	  /* Push a new prompt if the previous annotation_level was not >1.  */
 	  prefix = (char *) alloca (strlen (async_annotation_suffix) + 10);
@@ -389,7 +389,7 @@ change_annotation_level (void)
     }
   else
     {
-      if (strcmp (PREFIX (0), "") && strcmp (SUFFIX (0), ""))
+      if (strcmp (get_prefix (0), "") && strcmp (get_suffix (0), ""))
 	{
 	  /* Pop the top of the stack, we are going back to annotation < 1.  */
 	  pop_prompt ();
@@ -405,17 +405,17 @@ void
 push_prompt (char *prefix, char *prompt, char *suffix)
 {
   the_prompts.top++;
-  PREFIX (0) = xstrdup (prefix);
+  set_prefix (prefix, 0);
 
   /* Note that this function is used by the set annotate 2
      command.  This is why we take care of saving the old prompt
      in case a new one is not specified.  */
   if (prompt)
-    PROMPT (0) = xstrdup (prompt);
+    set_prompt (prompt, 0);
   else
-    PROMPT (0) = xstrdup (PROMPT (-1));
+    set_prompt (get_prompt (-1), 0);
 
-  SUFFIX (0) = xstrdup (suffix);
+  set_suffix (suffix, 0);
 }
 
 /* Pops the top of the prompt stack, and frees the memory allocated
@@ -425,20 +425,17 @@ pop_prompt (void)
 {
   /* If we are not during a 'synchronous' execution command, in which
      case, the top prompt would be empty.  */
-  if (strcmp (PROMPT (0), ""))
+  if (strcmp (get_prompt (0), ""))
     /* This is for the case in which the prompt is set while the
        annotation level is 2.  The top prompt will be changed, but when
        we return to annotation level < 2, we want that new prompt to be
        in effect, until the user does another 'set prompt'.  */
-    if (strcmp (PROMPT (0), PROMPT (-1)))
-      {
-	xfree (PROMPT (-1));
-	PROMPT (-1) = xstrdup (PROMPT (0));
-      }
-
-  xfree (PREFIX (0));
-  xfree (PROMPT (0));
-  xfree (SUFFIX (0));
+    if (strcmp (get_prompt (0), get_prompt (-1)))
+      set_prompt (get_prompt (0), -1);
+
+  set_prefix (NULL, 0);
+  set_prompt (NULL, 0);
+  set_suffix (NULL, 0);
   the_prompts.top--;
 }
 
@@ -983,7 +980,7 @@ handle_stop_sig (int sig)
 static void
 async_stop_sig (gdb_client_data arg)
 {
-  char *prompt = get_prompt ();
+  char *prompt = get_prompt (0);
 
 #if STOP_SIGNAL == SIGTSTP
   signal (SIGTSTP, SIG_DFL);
@@ -1061,7 +1058,7 @@ set_async_annotation_level (char *args, int from_tty,
 void
 set_async_prompt (char *args, int from_tty, struct cmd_list_element *c)
 {
-  PROMPT (0) = xstrdup (new_async_prompt);
+  set_prompt (new_async_prompt, 0);
 }
 
 /* Set things up for readline to be invoked via the alternate
diff --git a/gdb/event-top.h b/gdb/event-top.h
index 8550164..952abdb 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -69,9 +69,6 @@ struct prompts
     int top;
   };
 
-#define PROMPT(X) the_prompts.prompt_stack[the_prompts.top + X].prompt
-#define PREFIX(X) the_prompts.prompt_stack[the_prompts.top + X].prefix
-#define SUFFIX(X) the_prompts.prompt_stack[the_prompts.top + X].suffix
 
 /* Exported functions from event-top.c.
    FIXME: these should really go into top.h.  */
diff --git a/gdb/python/python.c b/gdb/python/python.c
index e3a8d19..16c0a6e 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -745,7 +745,7 @@ before_prompt_hook (const char *current_gdb_prompt)
   /* If a prompt has been set, PROMPT will not be NULL.  If it is
      NULL, do not set the prompt.  */
   if (prompt != NULL)
-    set_prompt (prompt);
+    set_prompt (prompt, 0);
 
   do_cleanups (cleanup);
   return;
diff --git a/gdb/top.c b/gdb/top.c
index 3ffd000..8dba0e0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -65,6 +65,10 @@
 #include "ui-out.h"
 #include "cli-out.h"
 
+#define PROMPT(X) the_prompts.prompt_stack[the_prompts.top + X].prompt
+#define PREFIX(X) the_prompts.prompt_stack[the_prompts.top + X].prefix
+#define SUFFIX(X) the_prompts.prompt_stack[the_prompts.top + X].suffix
+
 /* Default command line prompt.  This is overriden in some configs.  */
 
 #ifndef DEFAULT_PROMPT
@@ -533,7 +537,7 @@ command_loop (void)
   while (instream && !feof (instream))
     {
       if (window_hook && instream == stdin)
-	(*window_hook) (instream, get_prompt ());
+	(*window_hook) (instream, get_prompt (0));
 
       quit_flag = 0;
       if (instream == stdin && stdin_is_tty)
@@ -542,7 +546,7 @@ command_loop (void)
 
       /* Get a command-line.  This calls the readline package.  */
       command = command_line_input (instream == stdin ?
-				    get_prompt () : (char *) NULL,
+				    get_prompt (0) : (char *) NULL,
 				    instream == stdin, "prompt");
       if (command == 0)
 	{
@@ -1124,26 +1128,98 @@ and \"show warranty\" for details.\n");
     }
 }
 \f
-/* get_prompt: access method for the GDB prompt string.  */
+
+/* get_prefix: access method for the GDB prefix string.  */
 
 char *
-get_prompt (void)
+get_prefix (int level)
 {
-  return PROMPT (0);
+  return PREFIX (level);
 }
 
+/* set_prefix: set method for the GDB prefix string.  */
+
 void
-set_prompt (const char *s)
+set_prefix (const char *s, int level)
+{
+  /* If S is NULL, just free the PREFIX at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (PREFIX (level));
+      PREFIX (level) = NULL;
+    }
+  else
+    {
+      char *p = xstrdup (s);
+
+      xfree (PREFIX (level));
+      PREFIX (level) =  p;
+    }
+}
+
+/* get_suffix: access method for the GDB suffix string.  */
+
+char *
+get_suffix (int level)
 {
-  char *p = xstrdup (s);
+  return SUFFIX (level);
+}
 
-  xfree (PROMPT (0));
-  PROMPT (0) = p;
+/* set_suffix: set method for the GDB suffix string.  */
 
-  /* Also, free and set new_async_prompt so prompt changes sync up
-     with set/show prompt.  */
-  xfree (new_async_prompt);
-  new_async_prompt = xstrdup (PROMPT (0));
+void
+set_suffix (const char *s, int level)
+{
+  /* If S is NULL, just free the SUFFIX at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (SUFFIX (level));
+      SUFFIX (level) = NULL;
+    }
+  else
+    {
+      char *p = xstrdup (s);
+
+      xfree (SUFFIX (level));
+      SUFFIX (level) =  p;
+    }
+}
+
+ /* get_prompt: access method for the GDB prompt string.  */
+
+char *
+get_prompt (int level)
+{
+  return PROMPT (level);
+}
+
+void
+set_prompt (const char *s, int level)
+{
+  /* If S is NULL, just free the PROMPT at level LEVEL and set to
+     NULL.  */
+  if (s == NULL)
+    {
+      xfree (PROMPT (level));
+      PROMPT (level) = NULL;
+    }
+  else
+    {
+      char *p = xstrdup (s);
+
+      xfree (PROMPT (0));
+      PROMPT (0) = p;
+
+      if (level == 0)
+	{
+	  /* Also, free and set new_async_prompt so prompt changes sync up
+	     with set/show prompt.  */
+	    xfree (new_async_prompt);
+	    new_async_prompt = xstrdup (PROMPT (0));
+	  }
+      }
 }
 \f
 
@@ -1534,13 +1610,11 @@ init_main (void)
      whatever the DEFAULT_PROMPT is.  */
   the_prompts.top = 0;
   PREFIX (0) = "";
-  PROMPT (0) = xstrdup (DEFAULT_PROMPT);
+  set_prompt (DEFAULT_PROMPT, 0);
   SUFFIX (0) = "";
   /* Set things up for annotation_level > 1, if the user ever decides
      to use it.  */
   async_annotation_suffix = "prompt";
-  /* Set the variable associated with the setshow prompt command.  */
-  new_async_prompt = xstrdup (PROMPT (0));
 
   /* If gdb was started with --annotate=2, this is equivalent to the
      user entering the command 'set annotate 2' at the gdb prompt, so
diff --git a/gdb/top.h b/gdb/top.h
index 24ec2f2..3c1ec14 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -51,11 +51,27 @@ extern struct cleanup *prepare_execute_command (void);
 
 /* This function returns a pointer to the string that is used
    by gdb for its command prompt.  */
-extern char *get_prompt (void);
+extern char *get_prompt (int);
 
 /* This function copies the specified string into the string that
    is used by gdb for its command prompt.  */
-extern void set_prompt (const char *);
+extern void set_prompt (const char *, int level);
+
+/* This function returns a pointer to the string that is used
+   by gdb for its command prompt prefix.  */
+extern char *get_prefix (int);
+
+/* This function copies the specified string into the string that
+   is used by gdb for its command prompt prefix.  */
+extern void set_prefix (const char *, int);
+
+/* This function returns a pointer to the string that is used
+   by gdb for its command prompt suffix.  */
+extern char *get_suffix (int);
+
+/* This function copies the specified string into the string that
+   is used by gdb for its command prompt suffix.  */
+extern void set_suffix (const char *, int);
 
 /* From random places.  */
 extern int readnow_symbol_files;
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index d79de2b..919d1ac 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -145,17 +145,17 @@ tui_command_loop (void *data)
     {
       int length;
       char *a_prompt;
-      char *gdb_prompt = get_prompt ();
+      char *gdb_prompt = get_prompt (0);
 
       /* Tell readline what the prompt to display is and what function
          it will need to call after a whole line is read. This also
          displays the first prompt.  */
-      length = strlen (PREFIX (0)) 
-	+ strlen (gdb_prompt) + strlen (SUFFIX (0)) + 1;
+      length = strlen (get_prefix (0))
+	+ strlen (gdb_prompt) + strlen (get_suffix (0)) + 1;
       a_prompt = (char *) alloca (length);
-      strcpy (a_prompt, PREFIX (0));
+      strcpy (a_prompt, get_prefix (0));
       strcat (a_prompt, gdb_prompt);
-      strcat (a_prompt, SUFFIX (0));
+      strcat (a_prompt, get_suffix (0));
       rl_callback_handler_install (a_prompt, input_handler);
     }
   else

Re: [rfc] Prompt memory management/cleanups

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I have moved them to top.c
Phil> I have also incorporated Pedro's comments in the patch.

Phil> The ChangeLog remains the same as the initial patch.

Phil> OK?

Ok, thanks.

Tom

Re: [rfc] Prompt memory management/cleanups

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> I have moved them to top.c
> Phil> I have also incorporated Pedro's comments in the patch.
>
> Phil> The ChangeLog remains the same as the initial patch.
>
> Phil> OK?
>
> Ok, thanks.
>
> Tom

Thanks, so committed.

Cheers,

Phil