PATCH: Add type_sprint() function to return type in string form

PATCH: Add type_sprint() function to return type in string form

[Jason Molenda]

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

There are a few places in gdb where code prints an error message
with a type included in it.  The only way for these functions to
print a type is with type_print(), which takes a ui_file stream to
print its output to.  This means they either have to send the output
to gdb_stderr, or build up a fake memory ui_file and retrieve the
contents.

This patch adds a type_sprint() which does the latter and returns the
xmalloc()'ed string.

The current code gives bogus data to an MI client.  We had a bug that
gave this output at Apple:

-> 64-var-list-children "var7" 2
<- 64^error,msg="."
-> 65-exec-status
<- (gdb) 
<- &"Type TWindow has no component named TWindow"
<- 65^done,status="stopped"

This happens because this particular error reporter looked like this:

  fprintf_unfiltered (gdb_stderr, "Type ");
  type_print (type, "", gdb_stderr, -1);
  fprintf_unfiltered (gdb_stderr, " has no component named ");
  fputs_filtered (name, gdb_stderr);
  error (".");

Similar code shows up in a few places, so it made sense to create a 
single function to format a type into a string and return the string.

I can drop the ada-lang.c part of the patch if that will complicate
the approval, but given that this code is clearly derived from
gdbtypes.c, I don't think the change is particularly controversial.

This patch adds no new testsuite failures on RHL7.1 (gcc 2.96, stabs).

2003-04-18  Jason Molenda  (jmolenda @ apple.com)

        * typeprint.c (type_sprint): New function to return string form
        of types.  Use as type_print, sans a stream.
        * value.h (type_sprint): Add prototype.
        * ada-lang.c (ada_lookup_struct_elt_type): Use type_sprint() when
        building error message.
        * gdbtypes.c (lookup_struct_elt_type): Ditto.
        * varobj.c (varobj_get_type): Use type_sprint() to get the type
        in a string form.

[-- Attachment #2: pa.txt --]
[-- Type: text/plain, Size: 6433 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.4107
diff -u -p -r1.4107 ChangeLog
--- ChangeLog	18 Apr 2003 20:20:21 -0000	1.4107
+++ ChangeLog	18 Apr 2003 22:15:24 -0000
@@ -1,3 +1,14 @@
+2003-04-18  Jason Molenda  (jmolenda@apple.com)
+
+	* typeprint.c (type_sprint): New function to return string form
+	of types.  Use as type_print, sans a stream.
+	* value.h (type_sprint): Add prototype.
+	* ada-lang.c (ada_lookup_struct_elt_type): Use type_sprint() when
+	building error message.
+	* gdbtypes.c (lookup_struct_elt_type): Ditto.
+	* varobj.c (varobj_get_type): Use type_sprint() to get the type
+	in a string form.
+
 2003-04-18  Jim Blandy  <jimb@redhat.com>
 
 	* s390-tdep.c (s390_frame_align): New function.
Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.23
diff -u -p -r1.23 ada-lang.c
--- ada-lang.c	2 Apr 2003 03:02:46 -0000	1.23
+++ ada-lang.c	18 Apr 2003 22:15:24 -0000
@@ -5610,6 +5610,7 @@ ada_lookup_struct_elt_type (struct type 
 			    int *dispp)
 {
   int i;
+  char *type_for_printing;
 
   if (name == NULL)
     goto BadName;
@@ -5628,9 +5629,9 @@ ada_lookup_struct_elt_type (struct type 
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      error (" is not a structure or union type");
+      type_for_printing = type_sprint (type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s is not a structure or union type", type_for_printing);
     }
 
   type = to_static_fixed_type (type);
@@ -5690,10 +5691,10 @@ BadName:
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      fprintf_unfiltered (gdb_stderr, " has no component named ");
-      error ("%s", name == NULL ? "<null>" : name);
+      type_for_printing = type_sprint (type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s has no component named %s", type_for_printing, 
+             name == NULL ? "<null>" : name);
     }
 
   return NULL;
Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.71
diff -u -p -r1.71 gdbtypes.c
--- gdbtypes.c	7 Feb 2003 21:44:00 -0000	1.71
+++ gdbtypes.c	18 Apr 2003 22:15:25 -0000
@@ -1218,6 +1218,7 @@ struct type *
 lookup_struct_elt_type (struct type *type, char *name, int noerr)
 {
   int i;
+  char *type_for_printing;
 
   for (;;)
     {
@@ -1233,9 +1234,9 @@ lookup_struct_elt_type (struct type *typ
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      error (" is not a structure or union type.");
+      type_for_printing = type_sprint (type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s is not a structure or union type.", type_for_printing);
     }
 
 #if 0
@@ -1281,11 +1282,10 @@ lookup_struct_elt_type (struct type *typ
 
   target_terminal_ours ();
   gdb_flush (gdb_stdout);
-  fprintf_unfiltered (gdb_stderr, "Type ");
-  type_print (type, "", gdb_stderr, -1);
-  fprintf_unfiltered (gdb_stderr, " has no component named ");
-  fputs_filtered (name, gdb_stderr);
-  error (".");
+  type_for_printing = type_sprint (type, "", -1); 
+  make_cleanup (xfree, type_for_printing);
+  error ("Type %s has no component named %s.", type_for_printing, name);
+
   return (struct type *) -1;	/* For lint */
 }
 
Index: typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/typeprint.c,v
retrieving revision 1.17
diff -u -p -r1.17 typeprint.c
--- typeprint.c	25 Feb 2003 21:36:21 -0000	1.17
+++ typeprint.c	18 Apr 2003 22:15:25 -0000
@@ -109,6 +109,27 @@ type_print (struct type *type, char *var
   LA_PRINT_TYPE (type, varstring, stream, show, 0);
 }
 
+/* Returns a xmalloc'ed string of the type name instead of printing it
+   to STREAM.  */
+
+char *
+type_sprint (struct type *type, char *varstring, int show)
+{
+  struct ui_file *stb;
+  struct cleanup *wipe;
+  long length;
+  char *type_name;
+  
+  stb = mem_fileopen ();
+  wipe = make_cleanup_ui_file_delete (stb);
+
+  type_print (type, varstring, stb, show);
+  type_name = ui_file_xstrdup (stb, &length);
+  do_cleanups (wipe);
+
+  return type_name;
+}
+
 /* Print type of EXP, or last thing in value history if EXP == NULL.
    show is passed to type_print.  */
 
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.45
diff -u -p -r1.45 value.h
--- value.h	12 Apr 2003 17:41:26 -0000	1.45
+++ value.h	18 Apr 2003 22:15:25 -0000
@@ -498,6 +498,8 @@ extern void modify_field (char *addr, LO
 extern void type_print (struct type * type, char *varstring,
 			struct ui_file * stream, int show);
 
+extern char *type_sprint (struct type *type, char *varstring, int show);
+
 extern char *baseclass_addr (struct type *type, int index, char *valaddr,
 			     struct value **valuep, int *errp);
 
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.38
diff -u -p -r1.38 varobj.c
--- varobj.c	4 Dec 2002 00:05:54 -0000	1.38
+++ varobj.c	18 Apr 2003 22:15:25 -0000
@@ -728,27 +728,17 @@ char *
 varobj_get_type (struct varobj *var)
 {
   struct value *val;
-  struct cleanup *old_chain;
-  struct ui_file *stb;
-  char *thetype;
-  long length;
 
   /* For the "fake" variables, do not return a type. (It's type is
      NULL, too.) */
   if (CPLUS_FAKE_CHILD (var))
     return NULL;
 
-  stb = mem_fileopen ();
-  old_chain = make_cleanup_ui_file_delete (stb);
-
   /* To print the type, we simply create a zero ``struct value *'' and
      cast it to our type. We then typeprint this variable. */
   val = value_zero (var->type, not_lval);
-  type_print (VALUE_TYPE (val), "", stb, -1);
 
-  thetype = ui_file_xstrdup (stb, &length);
-  do_cleanups (old_chain);
-  return thetype;
+  return (type_sprint (VALUE_TYPE (val), "",  -1));
 }
 
 enum varobj_languages

Re: PATCH: Add type_sprint() function to return type in string form

[Daniel Jacobowitz]

On Fri, Apr 18, 2003 at 03:24:26PM -0700, Jason Molenda wrote:
> There are a few places in gdb where code prints an error message
> with a type included in it.  The only way for these functions to
> print a type is with type_print(), which takes a ui_file stream to
> print its output to.  This means they either have to send the output
> to gdb_stderr, or build up a fake memory ui_file and retrieve the
> contents.
> 
> This patch adds a type_sprint() which does the latter and returns the
> xmalloc()'ed string.
> 
> The current code gives bogus data to an MI client.  We had a bug that
> gave this output at Apple:
> 
> -> 64-var-list-children "var7" 2
> <- 64^error,msg="."
> -> 65-exec-status
> <- (gdb) 
> <- &"Type TWindow has no component named TWindow"
> <- 65^done,status="stopped"
> 
> This happens because this particular error reporter looked like this:
> 
>   fprintf_unfiltered (gdb_stderr, "Type ");
>   type_print (type, "", gdb_stderr, -1);
>   fprintf_unfiltered (gdb_stderr, " has no component named ");
>   fputs_filtered (name, gdb_stderr);
>   error (".");
> 
> Similar code shows up in a few places, so it made sense to create a 
> single function to format a type into a string and return the string.
> 
> I can drop the ada-lang.c part of the patch if that will complicate
> the approval, but given that this code is clearly derived from
> gdbtypes.c, I don't think the change is particularly controversial.
> 
> This patch adds no new testsuite failures on RHL7.1 (gcc 2.96, stabs).

I think this is OK.  Sit on it for another day or two and check it in
if no one objects in that time, please.

> 
> 2003-04-18  Jason Molenda  (jmolenda @ apple.com)
> 
>         * typeprint.c (type_sprint): New function to return string form
>         of types.  Use as type_print, sans a stream.
>         * value.h (type_sprint): Add prototype.
>         * ada-lang.c (ada_lookup_struct_elt_type): Use type_sprint() when
>         building error message.
>         * gdbtypes.c (lookup_struct_elt_type): Ditto.
>         * varobj.c (varobj_get_type): Use type_sprint() to get the type
>         in a string form.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

> On Fri, Apr 18, 2003 at 03:24:26PM -0700, Jason Molenda wrote:
> 
>> There are a few places in gdb where code prints an error message
>> with a type included in it.  The only way for these functions to
>> print a type is with type_print(), which takes a ui_file stream to
>> print its output to.  This means they either have to send the output
>> to gdb_stderr, or build up a fake memory ui_file and retrieve the
>> contents.
>> 
>> This patch adds a type_sprint() which does the latter and returns the
>> xmalloc()'ed string.

Good interface choice!  Just some tweaks.

sprint makes me think of the nasty sprintf family, which this is 
definitly not.  So ..  suggest calling it something like type_xstrdup: 
x-> xmalloc family; strdup -> allocate a string duplicate (like 
ui_file_xstrdup, but yes pushing it a bit).  The alternative would be 
type_xasprint (&string, ...) but I think that is getting ugly :-)

The old error message should never have included a period ("."), might 
as well remove it while tweaking the code.

>> The current code gives bogus data to an MI client.  We had a bug that
>> gave this output at Apple:
>> 
>> -> 64-var-list-children "var7" 2
>> <- 64^error,msg="."
>> -> 65-exec-status
>> <- (gdb) 
>> <- &"Type TWindow has no component named TWindow"
>> <- 65^done,status="stopped"
>> 
>> This happens because this particular error reporter looked like this:

Ulgh.  Testcase tweak?

>>   fprintf_unfiltered (gdb_stderr, "Type ");
>>   type_print (type, "", gdb_stderr, -1);
>>   fprintf_unfiltered (gdb_stderr, " has no component named ");
>>   fputs_filtered (name, gdb_stderr);
>>   error (".");
>> 
>> Similar code shows up in a few places, so it made sense to create a 
>> single function to format a type into a string and return the string.
>> 
>> I can drop the ada-lang.c part of the patch if that will complicate
>> the approval, but given that this code is clearly derived from
>> gdbtypes.c, I don't think the change is particularly controversial.
>> 
>> This patch adds no new testsuite failures on RHL7.1 (gcc 2.96, stabs).
> 
> 
> I think this is OK.  Sit on it for another day or two and check it in
> if no one objects in that time, please.

yup.  For ada, a best guess is sufficient.

Andrew


>> 2003-04-18  Jason Molenda  (jmolenda @ apple.com)
>> 
>>         * typeprint.c (type_sprint): New function to return string form
>>         of types.  Use as type_print, sans a stream.
>>         * value.h (type_sprint): Add prototype.
>>         * ada-lang.c (ada_lookup_struct_elt_type): Use type_sprint() when
>>         building error message.
>>         * gdbtypes.c (lookup_struct_elt_type): Ditto.
>>         * varobj.c (varobj_get_type): Use type_sprint() to get the type
>>         in a string form.
> 
> 
> 
> -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer 

Re: PATCH: Add type_sprint() function to return type in string form

[Daniel Jacobowitz]

On Tue, Apr 22, 2003 at 12:48:09PM -0400, Andrew Cagney wrote:
> >On Fri, Apr 18, 2003 at 03:24:26PM -0700, Jason Molenda wrote:
> >
> >>There are a few places in gdb where code prints an error message
> >>with a type included in it.  The only way for these functions to
> >>print a type is with type_print(), which takes a ui_file stream to
> >>print its output to.  This means they either have to send the output
> >>to gdb_stderr, or build up a fake memory ui_file and retrieve the
> >>contents.
> >>
> >>This patch adds a type_sprint() which does the latter and returns the
> >>xmalloc()'ed string.
> 
> Good interface choice!  Just some tweaks.
> 
> sprint makes me think of the nasty sprintf family, which this is 
> definitly not.  So ..  suggest calling it something like type_xstrdup: 
> x-> xmalloc family; strdup -> allocate a string duplicate (like 
> ui_file_xstrdup, but yes pushing it a bit).  The alternative would be 
> type_xasprint (&string, ...) but I think that is getting ugly :-)

How about type_asprint?  I really don't think that the x is necessary,
but the a would be a convenient reminder.

> The old error message should never have included a period ("."), might 
> as well remove it while tweaking the code.

Didn't we settle on full sentence error messages the last time this
came up?


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

> On Tue, Apr 22, 2003 at 12:48:09PM -0400, Andrew Cagney wrote:
> 
>> >On Fri, Apr 18, 2003 at 03:24:26PM -0700, Jason Molenda wrote:
>> >
> 
>> >>There are a few places in gdb where code prints an error message
>> >>with a type included in it.  The only way for these functions to
>> >>print a type is with type_print(), which takes a ui_file stream to
>> >>print its output to.  This means they either have to send the output
>> >>to gdb_stderr, or build up a fake memory ui_file and retrieve the
>> >>contents.
>> >>
>> >>This patch adds a type_sprint() which does the latter and returns the
>> >>xmalloc()'ed string.
> 
>> 
>> Good interface choice!  Just some tweaks.
>> 
>> sprint makes me think of the nasty sprintf family, which this is 
>> definitly not.  So ..  suggest calling it something like type_xstrdup: 
>> x-> xmalloc family; strdup -> allocate a string duplicate (like 
>> ui_file_xstrdup, but yes pushing it a bit).  The alternative would be 
>> type_xasprint (&string, ...) but I think that is getting ugly :-)
> 
> 
> How about type_asprint?  I really don't think that the x is necessary,
> but the a would be a convenient reminder.

GDB should use neither sprint* nor asprint*.  type_sprint or 
type_asprint would confuse this.

The xasprint* family all have the signature:

	...xasprint* (char **buf, ....)

which doesn't apply here.

>> The old error message should never have included a period ("."), might 
>> as well remove it while tweaking the code.
> 
> 
> Didn't we settle on full sentence error messages the last time this
> came up?

For the moment, it's a full sentence, with a leading upper case letter 
but no trailing period :-)  The period doesn't make much sence to a GUI. 
  The ARI is looking for this.

Andrew

Re: PATCH: Add type_sprint() function to return type in string form

[Daniel Jacobowitz]

On Tue, Apr 22, 2003 at 01:28:57PM -0400, Andrew Cagney wrote:
> >On Tue, Apr 22, 2003 at 12:48:09PM -0400, Andrew Cagney wrote:
> >
> >>>On Fri, Apr 18, 2003 at 03:24:26PM -0700, Jason Molenda wrote:
> >>>
> >
> >>>>There are a few places in gdb where code prints an error message
> >>>>with a type included in it.  The only way for these functions to
> >>>>print a type is with type_print(), which takes a ui_file stream to
> >>>>print its output to.  This means they either have to send the output
> >>>>to gdb_stderr, or build up a fake memory ui_file and retrieve the
> >>>>contents.
> >>>>
> >>>>This patch adds a type_sprint() which does the latter and returns the
> >>>>xmalloc()'ed string.
> >
> >>
> >>Good interface choice!  Just some tweaks.
> >>
> >>sprint makes me think of the nasty sprintf family, which this is 
> >>definitly not.  So ..  suggest calling it something like type_xstrdup: 
> >>x-> xmalloc family; strdup -> allocate a string duplicate (like 
> >>ui_file_xstrdup, but yes pushing it a bit).  The alternative would be 
> >>type_xasprint (&string, ...) but I think that is getting ugly :-)
> >
> >
> >How about type_asprint?  I really don't think that the x is necessary,
> >but the a would be a convenient reminder.
> 
> GDB should use neither sprint* nor asprint*.

I disagree.  Why on earth wouldn't we use sprintf?  Just because it can
be used incorrectly is no excuse.

>  type_sprint or 
> type_asprint would confuse this.

Even given the above assumption, I disagree with your conclusion.

> The xasprint* family all have the signature:
> 
> 	...xasprint* (char **buf, ....)
> 
> which doesn't apply here.

Perhaps it should apply?

> >>The old error message should never have included a period ("."), might 
> >>as well remove it while tweaking the code.
> >
> >
> >Didn't we settle on full sentence error messages the last time this
> >came up?
> 
> For the moment, it's a full sentence, with a leading upper case letter 
> but no trailing period :-)  The period doesn't make much sence to a GUI. 
>  The ARI is looking for this.

Huh, so it is.  Odd.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

>> GDB should use neither sprint* nor asprint*.
> 
> 
> I disagree.  Why on earth wouldn't we use sprintf?  Just because it can
> be used incorrectly is no excuse.

You're kidding right?

The ARI indicates that all sprintf calls should be replaced with either 
snprintf or xasprintf.

Replacing sprintf with functions that are immune to buffer overrun 
problems, eliminates an entire class of bug.

Even something as simple as:
	char buf[100000000];
	sprintf (buf, _("a"));
is broken.

>> The xasprint* family all have the signature:
>> 
>> 	...xasprint* (char **buf, ....)
>> 
>> which doesn't apply here.
> 
> 
> Perhaps it should apply?

Sure, but the interface would sux.

Andrew

Re: PATCH: Add type_sprint() function to return type in string form

[Daniel Jacobowitz]

On Tue, Apr 22, 2003 at 03:21:02PM -0400, Andrew Cagney wrote:
> 
> >>GDB should use neither sprint* nor asprint*.
> >
> >
> >I disagree.  Why on earth wouldn't we use sprintf?  Just because it can
> >be used incorrectly is no excuse.
> 
> You're kidding right?
> 
> The ARI indicates that all sprintf calls should be replaced with either 
> snprintf or xasprintf.
> 
> Replacing sprintf with functions that are immune to buffer overrun 
> problems, eliminates an entire class of bug.
> 
> Even something as simple as:
> 	char buf[100000000];
> 	sprintf (buf, _("a"));
> is broken.

No, I'm not kidding.  I've worked in computer security for about six
years now and I still use sprintf.  It's simple, it's effective, and if
you use it reasonably carefully, nothing will go wrong.

Not to mention that there are a number of buggy implementations of
snprintf; they're slowly starting to fade from use, thank the lord, but
you still see 'em now and again.

Of course, in those six years I've had this argument about ten times. 
It seems to be about a 50/50 split between developers.

And nothing involving translation is simple.

> >>The xasprint* family all have the signature:
> >>
> >>	...xasprint* (char **buf, ....)
> >>
> >>which doesn't apply here.
> >
> >
> >Perhaps it should apply?
> 
> Sure, but the interface would sux.

Then what would you prefer?  I have a really negative gut reaction to
putting dup in the name; it's not duplicating anything. 
type_as_string?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

>> Sure, but the interface would sux.
> 
> 
> Then what would you prefer?  I have a really negative gut reaction to
> putting dup in the name; it's not duplicating anything. 
> type_as_string?

It is making an xmalloc'ed string duplicate of the type.

Jason, your choice:

	type_xasprint (char **buf, ...)

or

	char *type_xstrdup (....);

Andrew

Re: PATCH: Add type_sprint() function to return type in string form

[Daniel Jacobowitz]

On Tue, Apr 22, 2003 at 04:16:26PM -0400, Andrew Cagney wrote:
> 
> >>Sure, but the interface would sux.
> >
> >
> >Then what would you prefer?  I have a really negative gut reaction to
> >putting dup in the name; it's not duplicating anything. 
> >type_as_string?
> 
> It is making an xmalloc'ed string duplicate of the type.

A duplicate is the same as the original.

A string is not the same as a type.

> Jason, your choice:
> 
> 	type_xasprint (char **buf, ...)
> 
> or
> 
> 	char *type_xstrdup (....);
> 
> Andrew
> 
> 
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: Add type_sprint() function to return type in string form

[Jason Molenda]

I really don't care about the name of the function.

Daniel's reasoning behind type_xasprint makes sense, although I
prefer the convenience of just getting the pointer to the xmalloc'ed
memory as a return value, in which case type_xstrdup would be more
appropriate.

Coin toss says... tails - it's type_xasprint.

I'll write an updated patch tonight when I get home, and see if I can
find a way to elicit the error in the testsuite.


Jason

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

>> You're kidding right?
>> 
>> The ARI indicates that all sprintf calls should be replaced with either 
>> snprintf or xasprintf.
>> 
>> Replacing sprintf with functions that are immune to buffer overrun 
>> problems, eliminates an entire class of bug.
>> 
>> Even something as simple as:
>> 	char buf[100000000];
>> 	sprintf (buf, _("a"));
>> is broken.
> 
> 
> No, I'm not kidding.  I've worked in computer security for about six
> years now and I still use sprintf.  It's simple, it's effective, and if
> you use it reasonably carefully, nothing will go wrong.

That also applies to snprintf and xasprintf.

> Not to mention that there are a number of buggy implementations of
> snprintf; they're slowly starting to fade from use, thank the lord, but
> you still see 'em now and again.

If GDB encounters a system with a buggy snprintf implementation, it 
should use the one in libiberty.  If libiberty's is buggy, some one 
should fix it.

> Of course, in those six years I've had this argument about ten times. 
> It seems to be about a 50/50 split between developers.

> And nothing involving translation is simple.

There are tradeoffs.

Things like the 80/20 rule (80% of bugs fixed with 20% of the effort); 
zero tolerance (complete elimination of risky coding pratices).

This was seen with Kevin's recent elimination of of complain that 
flushed out numerous -Wformat problems.

As for replacing the existing sprintf calls, like STREQ et.al., anyone 
wanting to do this will need to come up with a way of demonstrating that 
the translation was `probably' correct.  One trick is to only convert 
lines that GCOV identifies as being executed.

Andrew

Re: PATCH: Add type_sprint() function to return type in string form

[Kevin Buettner]

On Apr 22,  3:21pm, Andrew Cagney wrote:

> The ARI indicates that all sprintf calls should be replaced with either 
> snprintf or xasprintf.

I think the ARI is a useful tool for noticing potential problems in
gdb, but I don't think you should be using the ARI for making claims
about what is or is not acceptable in GDB.  If you can point to past
discussion(s) where this was decided, then that would be okay.  (I'm not
convinced that the ARI is a codification of all such past discussions.)

After all, the ARI also says "You can not take this seriously!"

Kevin

Re: PATCH: Add type_sprint() function to return type in string form

[Jason Molenda]

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

Hi all, here's the follow-up patch incorporating the comments.

I didn't have time to make a test case for this.  I want to at least
elicit the error message before I commit this, so I'll get something
together tomorrow night or the night after.  I don't know how easy
it will be to add to the testsuite, but I'll at least do something
by hand to get the error.

Jason

2003-04-23  Jason Molenda  (jmolenda at apple dot com)
 
        * typeprint.c (type_xasprint): New function to return human-readable
        string representation of a type.
        * value.h (type_xasprint): Add prototype.
        * ada-lang.c (ada_lookup_struct_elt_type): Use type_xasprint() when
        building error message.
        * gdbtypes.c (lookup_struct_elt_type): Ditto.
        * varobj.c (varobj_get_type): Use type_xasprint() to get the type
        in a string form.

[-- Attachment #2: pa.txt --]
[-- Type: text/plain, Size: 6585 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.4122
diff -u -p -r1.4122 ChangeLog
--- ChangeLog	22 Apr 2003 23:18:26 -0000	1.4122
+++ ChangeLog	23 Apr 2003 08:28:41 -0000
@@ -1,3 +1,14 @@
+2003-04-23  Jason Molenda  (jmolenda at apple dot com)
+
+	* typeprint.c (type_xasprint): New function to return human-readable
+        string representation of a type.
+	* value.h (type_xasprint): Add prototype.
+	* ada-lang.c (ada_lookup_struct_elt_type): Use type_xasprint() when
+	building error message.
+	* gdbtypes.c (lookup_struct_elt_type): Ditto.
+	* varobj.c (varobj_get_type): Use type_xasprint() to get the type
+	in a string form.
+
 2003-04-22  Kevin Buettner  <kevinb@redhat.com>
 
 	* dwarf2loc.c (dwarf2_evaluate_loc_desc): Invoke DWARF2_REG_TO_REGNUM
Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.24
diff -u -p -r1.24 ada-lang.c
--- ada-lang.c	21 Apr 2003 16:48:37 -0000	1.24
+++ ada-lang.c	23 Apr 2003 08:28:42 -0000
@@ -5611,6 +5611,7 @@ ada_lookup_struct_elt_type (struct type 
 			    int *dispp)
 {
   int i;
+  char *type_for_printing;
 
   if (name == NULL)
     goto BadName;
@@ -5629,9 +5630,9 @@ ada_lookup_struct_elt_type (struct type 
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      error (" is not a structure or union type");
+      type_xasprint (&type_for_printing, type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s is not a structure or union type", type_for_printing);
     }
 
   type = to_static_fixed_type (type);
@@ -5691,10 +5692,10 @@ BadName:
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      fprintf_unfiltered (gdb_stderr, " has no component named ");
-      error ("%s", name == NULL ? "<null>" : name);
+      type_xasprint (&type_for_printing, type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s has no component named %s", type_for_printing, 
+             name == NULL ? "<null>" : name);
     }
 
   return NULL;
Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.71
diff -u -p -r1.71 gdbtypes.c
--- gdbtypes.c	7 Feb 2003 21:44:00 -0000	1.71
+++ gdbtypes.c	23 Apr 2003 08:28:44 -0000
@@ -1218,6 +1218,7 @@ struct type *
 lookup_struct_elt_type (struct type *type, char *name, int noerr)
 {
   int i;
+  char *type_for_printing;
 
   for (;;)
     {
@@ -1233,9 +1234,9 @@ lookup_struct_elt_type (struct type *typ
     {
       target_terminal_ours ();
       gdb_flush (gdb_stdout);
-      fprintf_unfiltered (gdb_stderr, "Type ");
-      type_print (type, "", gdb_stderr, -1);
-      error (" is not a structure or union type.");
+      type_xasprint (&type_for_printing, type, "", -1);
+      make_cleanup (xfree, type_for_printing);
+      error ("Type %s is not a structure or union type", type_for_printing);
     }
 
 #if 0
@@ -1281,11 +1282,10 @@ lookup_struct_elt_type (struct type *typ
 
   target_terminal_ours ();
   gdb_flush (gdb_stdout);
-  fprintf_unfiltered (gdb_stderr, "Type ");
-  type_print (type, "", gdb_stderr, -1);
-  fprintf_unfiltered (gdb_stderr, " has no component named ");
-  fputs_filtered (name, gdb_stderr);
-  error (".");
+  type_xasprint (&type_for_printing, type, "", -1); 
+  make_cleanup (xfree, type_for_printing);
+  error ("Type %s has no component named %s", type_for_printing, name);
+
   return (struct type *) -1;	/* For lint */
 }
 
Index: typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/typeprint.c,v
retrieving revision 1.17
diff -u -p -r1.17 typeprint.c
--- typeprint.c	25 Feb 2003 21:36:21 -0000	1.17
+++ typeprint.c	23 Apr 2003 08:28:45 -0000
@@ -109,6 +109,25 @@ type_print (struct type *type, char *var
   LA_PRINT_TYPE (type, varstring, stream, show, 0);
 }
 
+/* Sets BUF to an xmalloc'ed string of the type name, instead of printing it
+   to STREAM like type_print() does.  */
+
+void
+type_xasprint (char **buf, struct type *type, char *varstring, int show)
+{
+  struct ui_file *stb;
+  struct cleanup *wipe;
+  long length;
+
+  *buf = NULL;
+  stb = mem_fileopen ();
+  wipe = make_cleanup_ui_file_delete (stb);
+
+  type_print (type, varstring, stb, show);
+  *buf = ui_file_xstrdup (stb, &length);
+  do_cleanups (wipe);
+}
+
 /* Print type of EXP, or last thing in value history if EXP == NULL.
    show is passed to type_print.  */
 
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.46
diff -u -p -r1.46 value.h
--- value.h	21 Apr 2003 16:48:40 -0000	1.46
+++ value.h	23 Apr 2003 08:28:45 -0000
@@ -498,6 +498,9 @@ extern void modify_field (char *addr, LO
 extern void type_print (struct type * type, char *varstring,
 			struct ui_file * stream, int show);
 
+extern void type_xasprint (char **buf, struct type *type, 
+                            char *varstring, int show);
+
 extern char *baseclass_addr (struct type *type, int index, char *valaddr,
 			     struct value **valuep, int *errp);
 
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.38
diff -u -p -r1.38 varobj.c
--- varobj.c	4 Dec 2002 00:05:54 -0000	1.38
+++ varobj.c	23 Apr 2003 08:28:46 -0000
@@ -728,27 +728,19 @@ char *
 varobj_get_type (struct varobj *var)
 {
   struct value *val;
-  struct cleanup *old_chain;
-  struct ui_file *stb;
-  char *thetype;
-  long length;
+  char *type_string;
 
   /* For the "fake" variables, do not return a type. (It's type is
      NULL, too.) */
   if (CPLUS_FAKE_CHILD (var))
     return NULL;
 
-  stb = mem_fileopen ();
-  old_chain = make_cleanup_ui_file_delete (stb);
-
   /* To print the type, we simply create a zero ``struct value *'' and
      cast it to our type. We then typeprint this variable. */
   val = value_zero (var->type, not_lval);
-  type_print (VALUE_TYPE (val), "", stb, -1);
 
-  thetype = ui_file_xstrdup (stb, &length);
-  do_cleanups (old_chain);
-  return thetype;
+  type_xasprint (&type_string, VALUE_TYPE (val), "",  -1);
+  return (type_string);
 }
 
 enum varobj_languages

Re: PATCH: Add type_sprint() function to return type in string form

[Andrew Cagney]

> Hi all, here's the follow-up patch incorporating the comments.
> 
> I didn't have time to make a test case for this.  I want to at least
> elicit the error message before I commit this, so I'll get something
> together tomorrow night or the night after.  I don't know how easy
> it will be to add to the testsuite, but I'll at least do something
> by hand to get the error.

I assume this is in.  I'm just amazed that the existing testsuite didn't 
trigger this.  Even a simple tweak of an existing test should do the trick.

Andrew


> 2003-04-23  Jason Molenda  (jmolenda at apple dot com)
>  
>         * typeprint.c (type_xasprint): New function to return human-readable
>         string representation of a type.
>         * value.h (type_xasprint): Add prototype.
>         * ada-lang.c (ada_lookup_struct_elt_type): Use type_xasprint() when
>         building error message.
>         * gdbtypes.c (lookup_struct_elt_type): Ditto.
>         * varobj.c (varobj_get_type): Use type_xasprint() to get the type
>         in a string form.
> 
>