Add "%d" support to _dl_debug_vdprintf

Message ID 20200605150324.460300-1-hjl.tools@gmail.com
State New
Headers show
Series
  • Add "%d" support to _dl_debug_vdprintf
Related show

Commit Message

Anton Blanchard via Libc-alpha June 5, 2020, 3:03 p.m.
"%d" will be used to print out signed value.
---
 elf/dl-misc.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

-- 
2.26.2

Comments

Anton Blanchard via Libc-alpha June 9, 2020, 2:18 p.m. | #1
On 05/06/2020 12:03, H.J. Lu via Libc-alpha wrote:
> "%d" will be used to print out signed value.


LGTM with some smalls nits below.

> ---

>  elf/dl-misc.c | 31 +++++++++++++++++++++++++++++--

>  1 file changed, 29 insertions(+), 2 deletions(-)

> 

> diff --git a/elf/dl-misc.c b/elf/dl-misc.c

> index ab70481fda..c82c8ae6fa 100644

> --- a/elf/dl-misc.c

> +++ b/elf/dl-misc.c

> @@ -167,6 +167,7 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>  	  switch (*fmt)

>  	    {

>  	      /* Integer formatting.  */

> +	    case 'd':

>  	    case 'u':

>  	    case 'x':

>  	      {


Ok.

> @@ -179,11 +180,34 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>  #else

>  		unsigned long int num = va_arg (arg, unsigned int);

>  #endif

> +		bool negative = false;

> +		if (*fmt == 'd')

> +		  {

> +#if LONG_MAX != INT_MAX

> +		    if (long_mod)

> +		      {

> +			if ((long) num < 0)


Full type specify on cast.

> +			  negative = true;

> +		      }

> +		    else

> +		      {

> +			if ((int) num < 0)> +			  {

> +			    num = (unsigned int) num;

> +			    negative = true;

> +			  }

> +		      }

> +#else

> +		    if ((int) num < 0)

> +		      negative = true;

> +#endif

> +		  }

> +

>  		/* We use alloca() to allocate the buffer with the most

>  		   pessimistic guess for the size.  Using alloca() allows

>  		   having more than one integer formatting in a call.  */

> -		char *buf = (char *) alloca (3 * sizeof (unsigned long int));

> -		char *endp = &buf[3 * sizeof (unsigned long int)];

> +		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));

> +		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];


I think we can remove alloca here and use INT_STRLEN_BOUND (since the
string is not '\0' bounded due the writev usage).  Something like:

  char buf[INT_STRLEN_BOUND (unsigned long int)];
  char *endp = &buf[INT_STRLEN_BOUND (unsigned long int)];

>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);

>  

>  		/* Pad to the width the user specified.  */

> @@ -191,6 +215,9 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>  		  while (endp - cp < width)

>  		    *--cp = fill;

>  

> +		if (negative)

> +		  *--cp = '-';

> +

>  		iov[niov].iov_base = cp;

>  		iov[niov].iov_len = endp - cp;

>  		++niov;

> 


Ok.
Anton Blanchard via Libc-alpha June 9, 2020, 2:39 p.m. | #2
On 09/06/2020 11:18, Adhemerval Zanella wrote:
> 

> 

> On 05/06/2020 12:03, H.J. Lu via Libc-alpha wrote:

>> "%d" will be used to print out signed value.

> 

> LGTM with some smalls nits below.

> 

>> ---

>>  elf/dl-misc.c | 31 +++++++++++++++++++++++++++++--

>>  1 file changed, 29 insertions(+), 2 deletions(-)

>>

>> diff --git a/elf/dl-misc.c b/elf/dl-misc.c

>> index ab70481fda..c82c8ae6fa 100644

>> --- a/elf/dl-misc.c

>> +++ b/elf/dl-misc.c

>> @@ -167,6 +167,7 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>>  	  switch (*fmt)

>>  	    {

>>  	      /* Integer formatting.  */

>> +	    case 'd':

>>  	    case 'u':

>>  	    case 'x':

>>  	      {

> 

> Ok.

> 

>> @@ -179,11 +180,34 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>>  #else

>>  		unsigned long int num = va_arg (arg, unsigned int);

>>  #endif

>> +		bool negative = false;

>> +		if (*fmt == 'd')

>> +		  {

>> +#if LONG_MAX != INT_MAX

>> +		    if (long_mod)

>> +		      {

>> +			if ((long) num < 0)

> 

> Full type specify on cast.

> 

>> +			  negative = true;

>> +		      }

>> +		    else

>> +		      {

>> +			if ((int) num < 0)> +			  {

>> +			    num = (unsigned int) num;

>> +			    negative = true;

>> +			  }

>> +		      }

>> +#else

>> +		    if ((int) num < 0)

>> +		      negative = true;

>> +#endif

>> +		  }

>> +

>>  		/* We use alloca() to allocate the buffer with the most

>>  		   pessimistic guess for the size.  Using alloca() allows

>>  		   having more than one integer formatting in a call.  */

>> -		char *buf = (char *) alloca (3 * sizeof (unsigned long int));

>> -		char *endp = &buf[3 * sizeof (unsigned long int)];

>> +		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));

>> +		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];

> 

> I think we can remove alloca here and use INT_STRLEN_BOUND (since the

> string is not '\0' bounded due the writev usage).  Something like:


Sigh, you can't actually remove the alloca here. 

> 

>   char buf[INT_STRLEN_BOUND (unsigned long int)];

>   char *endp = &buf[INT_STRLEN_BOUND (unsigned long int)];

> 

>>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);

>>  

>>  		/* Pad to the width the user specified.  */

>> @@ -191,6 +215,9 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>>  		  while (endp - cp < width)

>>  		    *--cp = fill;

>>  

>> +		if (negative)

>> +		  *--cp = '-';

>> +

>>  		iov[niov].iov_base = cp;

>>  		iov[niov].iov_len = endp - cp;

>>  		++niov;

>>

> 

> Ok.

>
Anton Blanchard via Libc-alpha June 9, 2020, 2:46 p.m. | #3
On Tue, Jun 9, 2020 at 7:39 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>

>

>

> On 09/06/2020 11:18, Adhemerval Zanella wrote:

> >

> >

> > On 05/06/2020 12:03, H.J. Lu via Libc-alpha wrote:

> >> "%d" will be used to print out signed value.

> >

> > LGTM with some smalls nits below.

> >

> >> ---

> >>  elf/dl-misc.c | 31 +++++++++++++++++++++++++++++--

> >>  1 file changed, 29 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/elf/dl-misc.c b/elf/dl-misc.c

> >> index ab70481fda..c82c8ae6fa 100644

> >> --- a/elf/dl-misc.c

> >> +++ b/elf/dl-misc.c

> >> @@ -167,6 +167,7 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

> >>        switch (*fmt)

> >>          {

> >>            /* Integer formatting.  */

> >> +        case 'd':

> >>          case 'u':

> >>          case 'x':

> >>            {

> >

> > Ok.

> >

> >> @@ -179,11 +180,34 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

> >>  #else

> >>              unsigned long int num = va_arg (arg, unsigned int);

> >>  #endif

> >> +            bool negative = false;

> >> +            if (*fmt == 'd')

> >> +              {

> >> +#if LONG_MAX != INT_MAX

> >> +                if (long_mod)

> >> +                  {

> >> +                    if ((long) num < 0)

> >

> > Full type specify on cast.

> >

> >> +                      negative = true;

> >> +                  }

> >> +                else

> >> +                  {

> >> +                    if ((int) num < 0)> +                     {

> >> +                        num = (unsigned int) num;

> >> +                        negative = true;

> >> +                      }

> >> +                  }

> >> +#else

> >> +                if ((int) num < 0)

> >> +                  negative = true;

> >> +#endif

> >> +              }

> >> +

> >>              /* We use alloca() to allocate the buffer with the most

> >>                 pessimistic guess for the size.  Using alloca() allows

> >>                 having more than one integer formatting in a call.  */

> >> -            char *buf = (char *) alloca (3 * sizeof (unsigned long int));

> >> -            char *endp = &buf[3 * sizeof (unsigned long int)];

> >> +            char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));

> >> +            char *endp = &buf[1 + 3 * sizeof (unsigned long int)];

> >

> > I think we can remove alloca here and use INT_STRLEN_BOUND (since the

> > string is not '\0' bounded due the writev usage).  Something like:

>

> Sigh, you can't actually remove the alloca here.


Why not?  It seems to work:

gdb) p endp
$3 = 0x7fffffffd674 "\377\177"
(gdb) p buf
$4 = "X\345\377\367\377\177\000\000\371-2147483647"
(gdb) p &buf
$5 = (char (*)[20]) 0x7fffffffd660
(gdb) p 0x7fffffffd674 - 0x7fffffffd660
$6 = 20
(gdb) list
204 #endif
205   }
206
207 char buf[INT_STRLEN_BOUND (long int)];
208 char *endp = &buf[INT_STRLEN_BOUND (long int)];
209 char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
210
211 /* Pad to the width the user specified.  */
212 if (width != -1)
213   while (endp - cp < width)
(gdb)

> >

> >   char buf[INT_STRLEN_BOUND (unsigned long int)];

> >   char *endp = &buf[INT_STRLEN_BOUND (unsigned long int)];

> >

> >>              char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);

> >>

> >>              /* Pad to the width the user specified.  */

> >> @@ -191,6 +215,9 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

> >>                while (endp - cp < width)

> >>                  *--cp = fill;

> >>

> >> +            if (negative)

> >> +              *--cp = '-';

> >> +

> >>              iov[niov].iov_base = cp;

> >>              iov[niov].iov_len = endp - cp;

> >>              ++niov;

> >>

> >

> > Ok.

> >




-- 
H.J.
Anton Blanchard via Libc-alpha June 9, 2020, 4:19 p.m. | #4
On 09/06/2020 11:46, H.J. Lu wrote:
> On Tue, Jun 9, 2020 at 7:39 AM Adhemerval Zanella

> <adhemerval.zanella@linaro.org> wrote:

>>

>>

>>

>> On 09/06/2020 11:18, Adhemerval Zanella wrote:

>>>

>>>

>>> On 05/06/2020 12:03, H.J. Lu via Libc-alpha wrote:

>>>> "%d" will be used to print out signed value.

>>>

>>> LGTM with some smalls nits below.

>>>

>>>> ---

>>>>  elf/dl-misc.c | 31 +++++++++++++++++++++++++++++--

>>>>  1 file changed, 29 insertions(+), 2 deletions(-)

>>>>

>>>> diff --git a/elf/dl-misc.c b/elf/dl-misc.c

>>>> index ab70481fda..c82c8ae6fa 100644

>>>> --- a/elf/dl-misc.c

>>>> +++ b/elf/dl-misc.c

>>>> @@ -167,6 +167,7 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>>>>        switch (*fmt)

>>>>          {

>>>>            /* Integer formatting.  */

>>>> +        case 'd':

>>>>          case 'u':

>>>>          case 'x':

>>>>            {

>>>

>>> Ok.

>>>

>>>> @@ -179,11 +180,34 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)

>>>>  #else

>>>>              unsigned long int num = va_arg (arg, unsigned int);

>>>>  #endif

>>>> +            bool negative = false;

>>>> +            if (*fmt == 'd')

>>>> +              {

>>>> +#if LONG_MAX != INT_MAX

>>>> +                if (long_mod)

>>>> +                  {

>>>> +                    if ((long) num < 0)

>>>

>>> Full type specify on cast.

>>>

>>>> +                      negative = true;

>>>> +                  }

>>>> +                else

>>>> +                  {

>>>> +                    if ((int) num < 0)> +                     {

>>>> +                        num = (unsigned int) num;

>>>> +                        negative = true;

>>>> +                      }

>>>> +                  }

>>>> +#else

>>>> +                if ((int) num < 0)

>>>> +                  negative = true;

>>>> +#endif

>>>> +              }

>>>> +

>>>>              /* We use alloca() to allocate the buffer with the most

>>>>                 pessimistic guess for the size.  Using alloca() allows

>>>>                 having more than one integer formatting in a call.  */

>>>> -            char *buf = (char *) alloca (3 * sizeof (unsigned long int));

>>>> -            char *endp = &buf[3 * sizeof (unsigned long int)];

>>>> +            char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));

>>>> +            char *endp = &buf[1 + 3 * sizeof (unsigned long int)];

>>>

>>> I think we can remove alloca here and use INT_STRLEN_BOUND (since the

>>> string is not '\0' bounded due the writev usage).  Something like:

>>

>> Sigh, you can't actually remove the alloca here.

> 

> Why not?  It seems to work:

> 


Because the iov uses the allocated buffer on the _dl_writev and it will
an invalid pointer if we allocate on the stack without alloca.

Patch

diff --git a/elf/dl-misc.c b/elf/dl-misc.c
index ab70481fda..c82c8ae6fa 100644
--- a/elf/dl-misc.c
+++ b/elf/dl-misc.c
@@ -167,6 +167,7 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 	  switch (*fmt)
 	    {
 	      /* Integer formatting.  */
+	    case 'd':
 	    case 'u':
 	    case 'x':
 	      {
@@ -179,11 +180,34 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 #else
 		unsigned long int num = va_arg (arg, unsigned int);
 #endif
+		bool negative = false;
+		if (*fmt == 'd')
+		  {
+#if LONG_MAX != INT_MAX
+		    if (long_mod)
+		      {
+			if ((long) num < 0)
+			  negative = true;
+		      }
+		    else
+		      {
+			if ((int) num < 0)
+			  {
+			    num = (unsigned int) num;
+			    negative = true;
+			  }
+		      }
+#else
+		    if ((int) num < 0)
+		      negative = true;
+#endif
+		  }
+
 		/* We use alloca() to allocate the buffer with the most
 		   pessimistic guess for the size.  Using alloca() allows
 		   having more than one integer formatting in a call.  */
-		char *buf = (char *) alloca (3 * sizeof (unsigned long int));
-		char *endp = &buf[3 * sizeof (unsigned long int)];
+		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
+		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
 		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
 
 		/* Pad to the width the user specified.  */
@@ -191,6 +215,9 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 		  while (endp - cp < width)
 		    *--cp = fill;
 
+		if (negative)
+		  *--cp = '-';
+
 		iov[niov].iov_base = cp;
 		iov[niov].iov_len = endp - cp;
 		++niov;