diff mbox series

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 | expand

Commit Message

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

Comments

Adhemerval Zanella June 9, 2020, 2:18 p.m. UTC | #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.
Adhemerval Zanella June 9, 2020, 2:39 p.m. UTC | #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.
>
H.J. Lu June 9, 2020, 2:46 p.m. UTC | #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.
> >
Adhemerval Zanella June 9, 2020, 4:19 p.m. UTC | #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.
diff mbox series

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;