Patchwork Fix DEMANGLE_COMPONENT_TEMPLATE_ARGLIST demangling (PR other/43838)

login
register
mail settings
Submitter Jakub Jelinek
Date June 9, 2010, 7:14 p.m.
Message ID <20100609191454.GG10293@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/55119/
State New
Headers show

Comments

Jakub Jelinek - June 9, 2010, 7:14 p.m.
Hi!

The dpi->len -= 2; trick to revert d_append_string (dpi, ", ");
is too fragile, as can be seen on the testcase below.
There are 2 problems:
1) the buffer can be flushed in the d_append_string (dpi, ", ");
   call.  If the recursive call doesn't print anything, it can't
   undo the ", " char addition - dpi->len will go negative.
2) if a flush (or more) happens during the recursive d_print_comp call
   then dpi->len might be equal to the saved len, but some characters
   were actually printed (if it is a multiple of 255 chars).
   By decrementing dpi->len then we eat two last characters, which
   won't be ", ".

1) is fixed by flushing before the d_append_string call if the buffer
is almost full.
2) is fixed by adding a counter how many flushes happened and comparing
   that to a saved value in addition to len.

Ok for trunk/4.5/4.4?

2010-06-09  Jakub Jelinek  <jakub@redhat.com>

	PR other/43838
	* cp-demangle.c (struct d_print_info): Add flush_count field.
	(d_print_init): Initialize it to 0.
	(d_print_flush): Increment it.
	(d_print_comp): If needed flush before appending ", ".  Only
	decrement dpi->len if no flushes happened during the recursive
	call.
	* testsuite/demangle-expected: Add a test for this.


	Jakub
Ian Taylor - June 9, 2010, 8:03 p.m.
Jakub Jelinek <jakub@redhat.com> writes:

> 2010-06-09  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR other/43838
> 	* cp-demangle.c (struct d_print_info): Add flush_count field.
> 	(d_print_init): Initialize it to 0.
> 	(d_print_flush): Increment it.
> 	(d_print_comp): If needed flush before appending ", ".  Only
> 	decrement dpi->len if no flushes happened during the recursive
> 	call.
> 	* testsuite/demangle-expected: Add a test for this.


> @@ -4047,12 +4051,18 @@ d_print_comp (struct d_print_info *dpi,
>        if (d_right (dc) != NULL)
>  	{
>  	  size_t len;
> +	  unsigned long int flush_count;
> +	  /* Make sure ", " isn't flushed by d_append_string, otherwise
> +	     dpi->len -= 2 wouldn't work.  */
> +	  if (dpi->len >= sizeof (dpi->buf) - 2)
> +	    d_print_flush (dpi);
>  	  d_append_string (dpi, ", ");
>  	  len = dpi->len;
> +	  flush_count = dpi->flush_count;
>  	  d_print_comp (dpi, d_right (dc));
>  	  /* If that didn't print anything (which can happen with empty
>  	     template argument packs), remove the comma and space.  */
> -	  if (dpi->len == len)
> +	  if (dpi->flush_count == flush_count && dpi->len == len)
>  	    dpi->len -= 2;
>  	}

I suppose this is OK, but it seems simpler and cleaner to just check
whether the d_print_comp will print anything before appending the
comma.  It seems like it should work to do

    struct demangle_component *right = d_right (dc);
    if (right != NULL && (d_left (right) != NULL || d_right (right) != NULL))
      {
        d_append_string (dpi, ", ");
        d_print_comp (dpi, right);
      }

Ian
Jakub Jelinek - June 9, 2010, 8:14 p.m.
On Wed, Jun 09, 2010 at 01:03:08PM -0700, Ian Lance Taylor wrote:
> I suppose this is OK, but it seems simpler and cleaner to just check
> whether the d_print_comp will print anything before appending the
> comma.  It seems like it should work to do
> 
>     struct demangle_component *right = d_right (dc);
>     if (right != NULL && (d_left (right) != NULL || d_right (right) != NULL))
>       {
>         d_append_string (dpi, ", ");
>         d_print_comp (dpi, right);
>       }

I believe e.g. DEMANGLE_COMPONENT_PACK_EXPANSION can print nothing
if d_find_pack returns non-NULL, but d_pack_length is 0.
Not sure if that's the only possibility.

	Jakub
Ian Taylor - June 10, 2010, 12:17 a.m.
Jakub Jelinek <jakub@redhat.com> writes:

> On Wed, Jun 09, 2010 at 01:03:08PM -0700, Ian Lance Taylor wrote:
>> I suppose this is OK, but it seems simpler and cleaner to just check
>> whether the d_print_comp will print anything before appending the
>> comma.  It seems like it should work to do
>> 
>>     struct demangle_component *right = d_right (dc);
>>     if (right != NULL && (d_left (right) != NULL || d_right (right) != NULL))
>>       {
>>         d_append_string (dpi, ", ");
>>         d_print_comp (dpi, right);
>>       }
>
> I believe e.g. DEMANGLE_COMPONENT_PACK_EXPANSION can print nothing
> if d_find_pack returns non-NULL, but d_pack_length is 0.
> Not sure if that's the only possibility.

I don't quite see how that can happen, but I don't have time to look
into this, so I'll approve your patch.

Ian
Jakub Jelinek - June 10, 2010, 7:40 a.m.
On Wed, Jun 09, 2010 at 05:17:21PM -0700, Ian Lance Taylor wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> 
> > On Wed, Jun 09, 2010 at 01:03:08PM -0700, Ian Lance Taylor wrote:
> >> I suppose this is OK, but it seems simpler and cleaner to just check
> >> whether the d_print_comp will print anything before appending the
> >> comma.  It seems like it should work to do
> >> 
> >>     struct demangle_component *right = d_right (dc);
> >>     if (right != NULL && (d_left (right) != NULL || d_right (right) != NULL))
> >>       {
> >>         d_append_string (dpi, ", ");
> >>         d_print_comp (dpi, right);
> >>       }
> >
> > I believe e.g. DEMANGLE_COMPONENT_PACK_EXPANSION can print nothing
> > if d_find_pack returns non-NULL, but d_pack_length is 0.
> > Not sure if that's the only possibility.
> 
> I don't quite see how that can happen, but I don't have time to look
> into this, so I'll approve your patch.

E.g.:

cplus_demangle ("_Z1fIIEEviDpT_", DMGL_PARAMS)

With your patch it gives
void f<>(int, )
and with my patch (likewise for current libiberty)
void f<>(int)

right is != NULL, but d_left (right) != NULL too.  d_left (right)
is DEMANGLE_COMPONENT_PACK_EXPANSION with d_pack_length (d_find_pack ()) ==
0.

I guess we'd need to write a function like d_print_comp that would return
true if d_print_comp will actually print any characters if we wanted to
avoid this printing + undo.

	Jakub

Patch

--- libiberty/cp-demangle.c.jj	2010-05-07 09:17:47.000000000 +0200
+++ libiberty/cp-demangle.c	2010-06-09 20:19:56.000000000 +0200
@@ -302,6 +302,8 @@  struct d_print_info
   /* The current index into any template argument packs we are using
      for printing.  */
   int pack_index;
+  /* Number of d_print_flush calls so far.  */
+  unsigned long int flush_count;
 };
 
 #ifdef CP_DEMANGLE_DEBUG
@@ -3285,6 +3287,7 @@  d_print_init (struct d_print_info *dpi, 
   dpi->last_char = '\0';
   dpi->templates = NULL;
   dpi->modifiers = NULL;
+  dpi->flush_count = 0;
 
   dpi->callback = callback;
   dpi->opaque = opaque;
@@ -3314,6 +3317,7 @@  d_print_flush (struct d_print_info *dpi)
   dpi->buf[dpi->len] = '\0';
   dpi->callback (dpi->buf, dpi->len, dpi->opaque);
   dpi->len = 0;
+  dpi->flush_count++;
 }
 
 /* Append characters and buffers for printing.  */
@@ -4047,12 +4051,18 @@  d_print_comp (struct d_print_info *dpi,
       if (d_right (dc) != NULL)
 	{
 	  size_t len;
+	  unsigned long int flush_count;
+	  /* Make sure ", " isn't flushed by d_append_string, otherwise
+	     dpi->len -= 2 wouldn't work.  */
+	  if (dpi->len >= sizeof (dpi->buf) - 2)
+	    d_print_flush (dpi);
 	  d_append_string (dpi, ", ");
 	  len = dpi->len;
+	  flush_count = dpi->flush_count;
 	  d_print_comp (dpi, d_right (dc));
 	  /* If that didn't print anything (which can happen with empty
 	     template argument packs), remove the comma and space.  */
-	  if (dpi->len == len)
+	  if (dpi->flush_count == flush_count && dpi->len == len)
 	    dpi->len -= 2;
 	}
       return;
--- libiberty/testsuite/demangle-expected.jj	2010-05-28 14:36:06.000000000 +0200
+++ libiberty/testsuite/demangle-expected	2010-06-09 20:36:34.000000000 +0200
@@ -3951,6 +3951,9 @@  decltype (({parm#1}.(operator-))()) h<A>
 --format=gnu-v3
 _Z1fDn
 f(decltype(nullptr))
+--format=gnu-v3
+_ZN5aaaaa6bbbbbb5cccccIN23ddddddddddddddddddddddd3eeeENS2_4ffff16ggggggggggggggggENS0_9hhhhhhhhhES6_S6_S6_S6_S6_S6_S6_EE
+aaaaa::bbbbbb::ccccc<ddddddddddddddddddddddd::eee, ddddddddddddddddddddddd::ffff::gggggggggggggggg, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh>
 #
 # Ada (GNAT) tests.
 #