Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Kyrill Tkachov]

On 20/09/16 18:56, Pedro Alves wrote:

On 09/20/2016 05:49 PM, Kyrill Tkachov wrote:


Ok, I'm proposing a new function defined in system.h
(though I'm open to suggestions for other location).
It would be something like:

static inline int ATTRIBUTE_PRINTF_3
gcc_snprintf (char *str, size_t size, const char *format, ...)
{
   va_list ap;
   va_start(ap, format);
   size_t res = vsnprintf (str, size, format, ap);
   va_end (ap);
   /* vsnprintf returns >= size if input was truncated.  */
   gcc_assert (res < size);
   return res;
}

Would that be acceptable?

gdb has had exactly that for eons:

int
xsnprintf (char *str, size_t size, const char *format, ...)
{
   va_list args;
   int ret;

   va_start (args, format);
   ret = vsnprintf (str, size, format, args);
   gdb_assert (ret < size);
   va_end (args);

   return ret;
}

Maybe reuse the same name?  It follows the naming scheme of
xmalloc, etc., with x meaning it never fails.

Even better would be to put this in libiberty.


Thanks for the idea. I'll try to get that working.

Kyrill



And perhaps even better would be to get rid of the hardcoded
buffer sizes and use libiberty's existing xasprintf instead.

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Pedro Alves]

On 09/20/2016 05:49 PM, Kyrill Tkachov wrote:

> Ok, I'm proposing a new function defined in system.h
> (though I'm open to suggestions for other location).
> It would be something like:
> 
> static inline int ATTRIBUTE_PRINTF_3
> gcc_snprintf (char *str, size_t size, const char *format, ...)
> {
>   va_list ap;
>   va_start(ap, format);
>   size_t res = vsnprintf (str, size, format, ap);
>   va_end (ap);
>   /* vsnprintf returns >= size if input was truncated.  */
>   gcc_assert (res < size);
>   return res;
> }
> 
> Would that be acceptable?

gdb has had exactly that for eons:

int
xsnprintf (char *str, size_t size, const char *format, ...)
{
  va_list args;
  int ret;

  va_start (args, format);
  ret = vsnprintf (str, size, format, args);
  gdb_assert (ret < size);
  va_end (args);

  return ret;
}

Maybe reuse the same name?  It follows the naming scheme of
xmalloc, etc., with x meaning it never fails.

Even better would be to put this in libiberty.

And perhaps even better would be to get rid of the hardcoded
buffer sizes and use libiberty's existing xasprintf instead.

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Richard Earnshaw (lists)]

On 20/09/16 17:49, Kyrill Tkachov wrote:
> 
> On 20/09/16 15:13, Richard Earnshaw (lists) wrote:
>> On 08/09/16 12:00, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> This is a rebase and slight respin of [1] that I sent out last November
>>> to change all uses of
>>> sprintf to snprintf in the arm backend.
>>>
>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> [1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html
>>>
>>> 2016-09-08  Kyrylo Tkachov  
>>>
>>>  * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
>>>  rather than sprintf.
>>>  (arm_set_fixed_conv_libfunc): Likewise.
>>>  (arm_option_override): Likewise.
>>>  (neon_output_logic_immediate): Likewise.
>>>  (neon_output_shift_immediate): Likewise.
>>>  (arm_output_multireg_pop): Likewise.
>>>  (vfp_output_vstmd): Likewise.
>>>  (output_move_vfp): Likewise.
>>>  (output_move_neon): Likewise.
>>>  (output_return_instruction): Likewise.
>>>  (arm_elf_asm_cdtor): Likewise.
>>>  (arm_output_shift): Likewise.
>>>  (arm_output_iwmmxt_shift_immediate): Likewise.
>>>  (arm_output_iwmmxt_tinsr): Likewise.
>>>  * config/arm/neon.md (*neon_mov, VDX): Likewise.
>>>  (*neon_mov, VQXMOV): Likewise.
>>>  (neon_vc_insn): Likewise.
>>>  (neon_vc_insn_unspec): Likewise.
>> Most of these should assert that truncation did not occur (it's an
>> internal error if the buffers aren't large enough).  In a few cases we
>> should call output_operand_lossage on failure since it might be due to a
>> user writing inline assembly and overflowing a buffer.
>>
>> I don't think we should silently accept a truncated write.
> 
> Ok, I'm proposing a new function defined in system.h
> (though I'm open to suggestions for other location).
> It would be something like:
> 
> static inline int ATTRIBUTE_PRINTF_3
> gcc_snprintf (char *str, size_t size, const char *format, ...)
> {
>   va_list ap;
>   va_start(ap, format);
>   size_t res = vsnprintf (str, size, format, ap);
>   va_end (ap);
>   /* vsnprintf returns >= size if input was truncated.  */
>   gcc_assert (res < size);
>   return res;
> }
> 
> Would that be acceptable?
> 

Yes, that's sort of what I had in mind.

Not sure if it needs to be inline, though; *printf are not generally
performance-critical functions and this bloats code somewhat with two
function calls for each invocation.

R.

> Thanks,
> Kyrill
> 
>>
>> R.
>>
>>> arm-snprintf.patch
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index
>>> 946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
>>> 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable,
>>> machine_mode mode,
>>> char buffer[50];
>>>   if (num_suffix == 0)
>>> -sprintf (buffer, "__gnu_%s%s", funcname, modename);
>>> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname,
>>> modename);
>>> else
>>> -sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
>>> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
>>> +  modename, num_suffix);
>>>   set_optab_libfunc (optable, mode, buffer);
>>>   }
>>> @@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab
>>> optable, machine_mode to,
>>> && ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
>>>   maybe_suffix_2 = "2";
>>>   -  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,
>>> -   maybe_suffix_2);
>>> +  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
>>> +fromname, toname, maybe_suffix_2);
>>>   set_conv_libfunc (optable, to, from, buffer);
>>>   }
>>> @@ -3163,7 +3164,8 @@ arm_option_override (void)
>>> if (!arm_selected_tune)
>>>   arm_selected_tune = _cores[arm_selected_cpu->core];
>>>   -  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);
>>> +  snprintf (arm_arch_name, sizeof (arm_arch_name),
>>> +"__ARM_ARCH_%s__", arm_selected_cpu->arch);
>>> insn_flags = arm_selected_cpu->flags;
>>> arm_base_arch = arm_selected_cpu->base_arch;
>>>   @@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char
>>> *mnem, rtx *op2, machine_mode mode,
>>> gcc_assert (is_valid != 0);
>>>   if (quad)
>>> -sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
>>> +snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
>>> +  mnem, width);
>>> else
>>> -sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
>>> +snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
>>> +  mnem, width);
>>>   return templ;
>>>   }
>>> @@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char
>>> *mnem, char sign, rtx *op2,
>>> gcc_assert (is_valid != 0);
>>>   if (quad)
>>> -sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
>>> +

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Kyrill Tkachov]

On 20/09/16 15:13, Richard Earnshaw (lists) wrote:

On 08/09/16 12:00, Kyrill Tkachov wrote:

Hi all,

This is a rebase and slight respin of [1] that I sent out last November
to change all uses of
sprintf to snprintf in the arm backend.

Bootstrapped and tested on arm-none-linux-gnueabihf.
Ok for trunk?

Thanks,
Kyrill

[1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html

2016-09-08  Kyrylo Tkachov  

 * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
 rather than sprintf.
 (arm_set_fixed_conv_libfunc): Likewise.
 (arm_option_override): Likewise.
 (neon_output_logic_immediate): Likewise.
 (neon_output_shift_immediate): Likewise.
 (arm_output_multireg_pop): Likewise.
 (vfp_output_vstmd): Likewise.
 (output_move_vfp): Likewise.
 (output_move_neon): Likewise.
 (output_return_instruction): Likewise.
 (arm_elf_asm_cdtor): Likewise.
 (arm_output_shift): Likewise.
 (arm_output_iwmmxt_shift_immediate): Likewise.
 (arm_output_iwmmxt_tinsr): Likewise.
 * config/arm/neon.md (*neon_mov, VDX): Likewise.
 (*neon_mov, VQXMOV): Likewise.
 (neon_vc_insn): Likewise.
 (neon_vc_insn_unspec): Likewise.

Most of these should assert that truncation did not occur (it's an
internal error if the buffers aren't large enough).  In a few cases we
should call output_operand_lossage on failure since it might be due to a
user writing inline assembly and overflowing a buffer.

I don't think we should silently accept a truncated write.


Ok, I'm proposing a new function defined in system.h
(though I'm open to suggestions for other location).
It would be something like:

static inline int ATTRIBUTE_PRINTF_3
gcc_snprintf (char *str, size_t size, const char *format, ...)
{
  va_list ap;
  va_start(ap, format);
  size_t res = vsnprintf (str, size, format, ap);
  va_end (ap);
  /* vsnprintf returns >= size if input was truncated.  */
  gcc_assert (res < size);
  return res;
}

Would that be acceptable?

Thanks,
Kyrill



R.


arm-snprintf.patch


diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 
946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable, machine_mode 
mode,
char buffer[50];
  
if (num_suffix == 0)

-sprintf (buffer, "__gnu_%s%s", funcname, modename);
+snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname, modename);
else
-sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
+snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
+ modename, num_suffix);
  
set_optab_libfunc (optable, mode, buffer);

  }
@@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab optable, 
machine_mode to,
&& ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
  maybe_suffix_2 = "2";
  
-  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,

-  maybe_suffix_2);
+  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
+   fromname, toname, maybe_suffix_2);
  
set_conv_libfunc (optable, to, from, buffer);

  }
@@ -3163,7 +3164,8 @@ arm_option_override (void)
if (!arm_selected_tune)
  arm_selected_tune = _cores[arm_selected_cpu->core];
  
-  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);

+  snprintf (arm_arch_name, sizeof (arm_arch_name),
+   "__ARM_ARCH_%s__", arm_selected_cpu->arch);
insn_flags = arm_selected_cpu->flags;
arm_base_arch = arm_selected_cpu->base_arch;
  
@@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char *mnem, rtx *op2, machine_mode mode,

gcc_assert (is_valid != 0);
  
if (quad)

-sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
+snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
+ mnem, width);
else
-sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
+snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
+ mnem, width);
  
return templ;

  }
@@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char *mnem, char 
sign, rtx *op2,
gcc_assert (is_valid != 0);
  
if (quad)

-sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
+snprintf (templ, sizeof (templ),
+ "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
else
-sprintf (templ, "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
+snprintf (templ, sizeof (templ),
+ "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
  
return templ;

  }
@@ -17858,17 +17864,17 @@ arm_output_multireg_pop (rtx *operands, bool 
return_pc, rtx cond, bool reverse,
conditional = reverse ? "%?%D0" : "%?%d0";
/* Can't use POP if returning from an interrupt.  */
if ((regno_base == SP_REGNUM) && update && !(interrupt_p && return_pc))
-sprintf (pattern, "pop%s\t{", conditional);
+snprintf (pattern, 

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Richard Earnshaw (lists)]

On 08/09/16 12:00, Kyrill Tkachov wrote:
> Hi all,
> 
> This is a rebase and slight respin of [1] that I sent out last November
> to change all uses of
> sprintf to snprintf in the arm backend.
> 
> Bootstrapped and tested on arm-none-linux-gnueabihf.
> Ok for trunk?
> 
> Thanks,
> Kyrill
> 
> [1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html
> 
> 2016-09-08  Kyrylo Tkachov  
> 
> * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
> rather than sprintf.
> (arm_set_fixed_conv_libfunc): Likewise.
> (arm_option_override): Likewise.
> (neon_output_logic_immediate): Likewise.
> (neon_output_shift_immediate): Likewise.
> (arm_output_multireg_pop): Likewise.
> (vfp_output_vstmd): Likewise.
> (output_move_vfp): Likewise.
> (output_move_neon): Likewise.
> (output_return_instruction): Likewise.
> (arm_elf_asm_cdtor): Likewise.
> (arm_output_shift): Likewise.
> (arm_output_iwmmxt_shift_immediate): Likewise.
> (arm_output_iwmmxt_tinsr): Likewise.
> * config/arm/neon.md (*neon_mov, VDX): Likewise.
> (*neon_mov, VQXMOV): Likewise.
> (neon_vc_insn): Likewise.
> (neon_vc_insn_unspec): Likewise.

Most of these should assert that truncation did not occur (it's an
internal error if the buffers aren't large enough).  In a few cases we
should call output_operand_lossage on failure since it might be due to a
user writing inline assembly and overflowing a buffer.

I don't think we should silently accept a truncated write.

R.

> 
> arm-snprintf.patch
> 
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 
> 946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
>  100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable, 
> machine_mode mode,
>char buffer[50];
>  
>if (num_suffix == 0)
> -sprintf (buffer, "__gnu_%s%s", funcname, modename);
> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname, modename);
>else
> -sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
> +   modename, num_suffix);
>  
>set_optab_libfunc (optable, mode, buffer);
>  }
> @@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab optable, 
> machine_mode to,
>&& ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
>  maybe_suffix_2 = "2";
>  
> -  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,
> -maybe_suffix_2);
> +  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
> + fromname, toname, maybe_suffix_2);
>  
>set_conv_libfunc (optable, to, from, buffer);
>  }
> @@ -3163,7 +3164,8 @@ arm_option_override (void)
>if (!arm_selected_tune)
>  arm_selected_tune = _cores[arm_selected_cpu->core];
>  
> -  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);
> +  snprintf (arm_arch_name, sizeof (arm_arch_name),
> + "__ARM_ARCH_%s__", arm_selected_cpu->arch);
>insn_flags = arm_selected_cpu->flags;
>arm_base_arch = arm_selected_cpu->base_arch;
>  
> @@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char *mnem, rtx 
> *op2, machine_mode mode,
>gcc_assert (is_valid != 0);
>  
>if (quad)
> -sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
> +snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
> +   mnem, width);
>else
> -sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
> +snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
> +   mnem, width);
>  
>return templ;
>  }
> @@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char *mnem, char 
> sign, rtx *op2,
>gcc_assert (is_valid != 0);
>  
>if (quad)
> -sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
> +snprintf (templ, sizeof (templ),
> +   "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
>else
> -sprintf (templ, "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
> +snprintf (templ, sizeof (templ),
> +   "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
>  
>return templ;
>  }
> @@ -17858,17 +17864,17 @@ arm_output_multireg_pop (rtx *operands, bool 
> return_pc, rtx cond, bool reverse,
>conditional = reverse ? "%?%D0" : "%?%d0";
>/* Can't use POP if returning from an interrupt.  */
>if ((regno_base == SP_REGNUM) && update && !(interrupt_p && return_pc))
> -sprintf (pattern, "pop%s\t{", conditional);
> +snprintf (pattern, sizeof (pattern), "pop%s\t{", conditional);
>else
>  {
>/* Output ldmfd when the base register is SP, otherwise output ldmia.
>   It's just a convention, their semantics are identical.  */
>if (regno_base == SP_REGNUM)
> - sprintf (pattern, "ldmfd%s\t", conditional);
> + snprintf (pattern, sizeof (pattern), "ldmfd%s\t", conditional);
>else if 

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00429.html

Thanks,
Kyrill

On 08/09/16 12:00, Kyrill Tkachov wrote:

Hi all,

This is a rebase and slight respin of [1] that I sent out last November to 
change all uses of
sprintf to snprintf in the arm backend.

Bootstrapped and tested on arm-none-linux-gnueabihf.
Ok for trunk?

Thanks,
Kyrill

[1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html

2016-09-08  Kyrylo Tkachov  

* config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
rather than sprintf.
(arm_set_fixed_conv_libfunc): Likewise.
(arm_option_override): Likewise.
(neon_output_logic_immediate): Likewise.
(neon_output_shift_immediate): Likewise.
(arm_output_multireg_pop): Likewise.
(vfp_output_vstmd): Likewise.
(output_move_vfp): Likewise.
(output_move_neon): Likewise.
(output_return_instruction): Likewise.
(arm_elf_asm_cdtor): Likewise.
(arm_output_shift): Likewise.
(arm_output_iwmmxt_shift_immediate): Likewise.
(arm_output_iwmmxt_tinsr): Likewise.
* config/arm/neon.md (*neon_mov, VDX): Likewise.
(*neon_mov, VQXMOV): Likewise.
(neon_vc_insn): Likewise.
(neon_vc_insn_unspec): Likewise.