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 <kyrylo.tkac...@arm.com>
>>>
>>> * 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<mode>, VDX): Likewise.
>>> (*neon_mov<mode>, VQXMOV): Likewise.
>>> (neon_vc<cmp_op><mode>_insn): Likewise.
>>> (neon_vc<cmp_op_unsp><mode>_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 = &all_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 (update)
>>> - sprintf (pattern, "ldmia%s\t", conditional);
>>> + snprintf (pattern, sizeof (pattern), "ldmia%s\t", conditional);
>>> else
>>> - sprintf (pattern, "ldm%s\t", conditional);
>>> + snprintf (pattern, sizeof (pattern), "ldm%s\t", conditional);
>>> strcat (pattern, reg_names[regno_base]);
>>> if (update)
>>> @@ -17924,7 +17930,8 @@ vfp_output_vstmd (rtx * operands)
>>> base = (REGNO (operands[1]) - FIRST_VFP_REGNUM) / 2;
>>> for (i = 1; i < XVECLEN (operands[2], 0); i++)
>>> {
>>> - p += sprintf (&pattern[p], ", d%d", base + i);
>>> + p += snprintf (&pattern[p], sizeof (pattern),
>>> + ", d%d", base + i);
>>> }
>>> strcpy (&pattern[p], "}");
>>> @@ -18719,7 +18726,7 @@ output_move_vfp (rtx *operands)
>>> break;
>>> }
>>> - sprintf (buff, templ,
>>> + snprintf (buff, sizeof (buff), templ,
>>> load ? "ld" : "st",
>>> dp ? "64" : "32",
>>> dp ? "P" : "",
>>> @@ -18859,7 +18866,8 @@ output_move_neon (rtx *operands)
>>> }
>>> else
>>> {
>>> - sprintf (buff, "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
>>> + snprintf (buff, sizeof (buff),
>>> + "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
>>> output_asm_insn (buff, ops);
>>> }
>>> }
>>> @@ -18867,7 +18875,8 @@ output_move_neon (rtx *operands)
>>> {
>>> ops[0] = gen_rtx_REG (DImode, REGNO (reg) + 2 * overlap);
>>> ops[1] = adjust_address (mem, SImode, 8 * overlap);
>>> - sprintf (buff, "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
>>> + snprintf (buff, sizeof (buff),
>>> + "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
>>> output_asm_insn (buff, ops);
>>> }
>>> @@ -18878,7 +18887,7 @@ output_move_neon (rtx *operands)
>>> gcc_unreachable ();
>>> }
>>> - sprintf (buff, templ, load ? "ld" : "st");
>>> + snprintf (buff, sizeof (buff), templ, load ? "ld" : "st");
>>> output_asm_insn (buff, ops);
>>> return "";
>>> @@ -19606,7 +19615,8 @@ output_return_instruction (rtx operand, bool
>>> really_return, bool reverse,
>>> gcc_assert (!cfun->calls_alloca || really_return);
>>> - sprintf (conditional, "%%?%%%c0", reverse ? 'D' : 'd');
>>> + snprintf (conditional, sizeof (conditional),
>>> + "%%?%%%c0", reverse ? 'D' : 'd');
>>> cfun->machine->return_used_this_function = 1;
>>> @@ -19657,7 +19667,8 @@ output_return_instruction (rtx operand,
>>> bool really_return, bool reverse,
>>> || ! really_return
>>> || ! IS_INTERRUPT (func_type)))
>>> {
>>> - sprintf (instr, "ldr%s\t%%|%s, [%%|sp], #4", conditional,
>>> + snprintf (instr, sizeof (instr),
>>> + "ldr%s\t%%|%s, [%%|sp], #4", conditional,
>>> (reg == LR_REGNUM) ? return_reg : reg_names[reg]);
>>> }
>>> else
>>> @@ -19677,7 +19688,8 @@ output_return_instruction (rtx operand, bool
>>> really_return, bool reverse,
>>> gcc_assert (stack_adjust == 0 || stack_adjust == 4);
>>> if (stack_adjust && arm_arch5 && TARGET_ARM)
>>> - sprintf (instr, "ldmib%s\t%%|sp, {", conditional);
>>> + snprintf (instr, sizeof (instr), "ldmib%s\t%%|sp, {",
>>> + conditional);
>>> else
>>> {
>>> /* If we can't use ldmib (SA110 bug),
>>> @@ -19685,17 +19697,20 @@ output_return_instruction (rtx operand,
>>> bool really_return, bool reverse,
>>> if (stack_adjust)
>>> live_regs_mask |= 1 << 3;
>>> - sprintf (instr, "ldmfd%s\t%%|sp, {", conditional);
>>> + snprintf (instr, sizeof (instr), "ldmfd%s\t%%|sp, {",
>>> + conditional);
>>> }
>>> }
>>> /* For interrupt returns we have to use an LDM rather than
>>> a POP so that we can use the exception return variant. */
>>> else if (IS_INTERRUPT (func_type))
>>> - sprintf (instr, "ldmfd%s\t%%|sp!, {", conditional);
>>> + snprintf (instr, sizeof (instr), "ldmfd%s\t%%|sp!, {",
>>> conditional);
>>> else
>>> - sprintf (instr, "pop%s\t{", conditional);
>>> + snprintf (instr, sizeof (instr), "pop%s\t{", conditional);
>>> - p = instr + strlen (instr);
>>> + size_t len = strlen (instr);
>>> + p = instr + len;
>>> + size_t rem_size = sizeof (instr) - len;
>>> for (reg = 0; reg <= SP_REGNUM; reg++)
>>> if (live_regs_mask & (1 << reg))
>>> @@ -19717,7 +19732,7 @@ output_return_instruction (rtx operand, bool
>>> really_return, bool reverse,
>>> if (live_regs_mask & (1 << LR_REGNUM))
>>> {
>>> - sprintf (p, "%s%%|%s}", first ? "" : ", ", return_reg);
>>> + snprintf (p, rem_size, "%s%%|%s}", first ? "" : ", ",
>>> return_reg);
>>> /* If returning from an interrupt, restore the CPSR. */
>>> if (IS_INTERRUPT (func_type))
>>> strcat (p, "^");
>>> @@ -19747,25 +19762,27 @@ output_return_instruction (rtx operand,
>>> bool really_return, bool reverse,
>>> case ARM_FT_ISR:
>>> case ARM_FT_FIQ:
>>> /* ??? This is wrong for unified assembly syntax. */
>>> - sprintf (instr, "sub%ss\t%%|pc, %%|lr, #4", conditional);
>>> + snprintf (instr, sizeof (instr), "sub%ss\t%%|pc, %%|lr, #4",
>>> + conditional);
>>> break;
>>> case ARM_FT_INTERWORKED:
>>> - gcc_assert (arm_arch5 || arm_arch4t);
>>> - sprintf (instr, "bx%s\t%%|lr", conditional);
>>> + snprintf (instr, sizeof (instr), "bx%s\t%%|lr", conditional);
>>> break;
>>> case ARM_FT_EXCEPTION:
>>> /* ??? This is wrong for unified assembly syntax. */
>>> - sprintf (instr, "mov%ss\t%%|pc, %%|lr", conditional);
>>> + snprintf (instr, sizeof (instr), "mov%ss\t%%|pc, %%|lr",
>>> + conditional);
>>> break;
>>> default:
>>> /* Use bx if it's available. */
>>> if (arm_arch5 || arm_arch4t)
>>> - sprintf (instr, "bx%s\t%%|lr", conditional);
>>> + snprintf (instr, sizeof (instr), "bx%s\t%%|lr", conditional);
>>> else
>>> - sprintf (instr, "mov%s\t%%|pc, %%|lr", conditional);
>>> + snprintf (instr, sizeof (instr), "mov%s\t%%|pc, %%|lr",
>>> + conditional);
>>> break;
>>> }
>>> @@ -22817,9 +22834,9 @@ arm_elf_asm_cdtor (rtx symbol, int
>>> priority, bool is_ctor)
>>> if (priority != DEFAULT_INIT_PRIORITY)
>>> {
>>> char buf[18];
>>> - sprintf (buf, "%s.%.5u",
>>> - is_ctor ? ".init_array" : ".fini_array",
>>> - priority);
>>> + snprintf (buf, sizeof (buf), "%s.%.5u",
>>> + is_ctor ? ".init_array" : ".fini_array",
>>> + priority);
>>> s = get_section (buf, SECTION_WRITE, NULL_TREE);
>>> }
>>> else if (is_ctor)
>>> @@ -27429,10 +27446,10 @@ arm_output_shift(rtx * operands, int
>>> set_flags)
>>> {
>>> if (val != -1)
>>> operands[2] = GEN_INT(val);
>>> - sprintf (pattern, "%s%%%c\t%%0, %%1, %%2", shift, c);
>>> + snprintf (pattern, sizeof (pattern), "%s%%%c\t%%0, %%1, %%2",
>>> shift, c);
>>> }
>>> else
>>> - sprintf (pattern, "mov%%%c\t%%0, %%1", c);
>>> + snprintf (pattern, sizeof (pattern), "mov%%%c\t%%0, %%1", c);
>>> output_asm_insn (pattern, operands);
>>> return "";
>>> @@ -27456,33 +27473,35 @@ arm_output_iwmmxt_shift_immediate (const
>>> char *insn_name, rtx *operands, bool wr
>>> {
>>> if (wror_or_wsra)
>>> {
>>> - sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, 32);
>>> - output_asm_insn (templ, operands);
>>> - if (opmode == DImode)
>>> - {
>>> - sprintf (templ, "%s\t%%0, %%0, #%d", insn_name, 32);
>>> + snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d", insn_name,
>>> 32);
>>> + output_asm_insn (templ, operands);
>>> + if (opmode == DImode)
>>> + {
>>> + snprintf (templ, sizeof (templ), "%s\t%%0, %%0, #%d",
>>> + insn_name, 32);
>>> output_asm_insn (templ, operands);
>>> - }
>>> + }
>>> }
>>> else
>>> {
>>> - /* The destination register will contain all zeros. */
>>> - sprintf (templ, "wzero\t%%0");
>>> - output_asm_insn (templ, operands);
>>> + /* The destination register will contain all zeros. */
>>> + snprintf (templ, sizeof (templ), "wzero\t%%0");
>>> + output_asm_insn (templ, operands);
>>> }
>>> return "";
>>> }
>>> if ((opmode == DImode) && (shift > 32))
>>> {
>>> - sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, 32);
>>> + snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d",
>>> insn_name, 32);
>>> output_asm_insn (templ, operands);
>>> - sprintf (templ, "%s\t%%0, %%0, #%d", insn_name, shift - 32);
>>> + snprintf (templ, sizeof (templ), "%s\t%%0, %%0, #%d",
>>> + insn_name, shift - 32);
>>> output_asm_insn (templ, operands);
>>> }
>>> else
>>> {
>>> - sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, shift);
>>> + snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d",
>>> insn_name, shift);
>>> output_asm_insn (templ, operands);
>>> }
>>> return "";
>>> @@ -27510,13 +27529,13 @@ arm_output_iwmmxt_tinsr (rtx *operands)
>>> switch (GET_MODE (operands[0]))
>>> {
>>> case V8QImode:
>>> - sprintf (templ, "tinsrb%%?\t%%0, %%2, #%d", i);
>>> + snprintf (templ, sizeof (templ), "tinsrb%%?\t%%0, %%2, #%d", i);
>>> break;
>>> case V4HImode:
>>> - sprintf (templ, "tinsrh%%?\t%%0, %%2, #%d", i);
>>> + snprintf (templ, sizeof (templ), "tinsrh%%?\t%%0, %%2, #%d", i);
>>> break;
>>> case V2SImode:
>>> - sprintf (templ, "tinsrw%%?\t%%0, %%2, #%d", i);
>>> + snprintf (templ, sizeof (templ), "tinsrw%%?\t%%0, %%2, #%d", i);
>>> break;
>>> default:
>>> gcc_unreachable ();
>>> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
>>> index
>>> e2fdfbb04621ee6f8603849be089e8bce624214d..54e6edfc4c053f685c8ca0cd3d1227015654aca1
>>> 100644
>>> --- a/gcc/config/arm/neon.md
>>> +++ b/gcc/config/arm/neon.md
>>> @@ -43,9 +43,10 @@ (define_insn "*neon_mov<mode>"
>>> gcc_assert (is_valid != 0);
>>> if (width == 0)
>>> - return "vmov.f32\t%P0, %1 @ <mode>";
>>> + return "vmov.f32\t%P0, %1 @ <mode>";
>>> else
>>> - sprintf (templ, "vmov.i%d\t%%P0, %%x1 @ <mode>", width);
>>> + snprintf (templ, sizeof (templ),
>>> + "vmov.i%d\t%%P0, %%x1 @ <mode>", width);
>>> return templ;
>>> }
>>> @@ -88,9 +89,10 @@ (define_insn "*neon_mov<mode>"
>>> gcc_assert (is_valid != 0);
>>> if (width == 0)
>>> - return "vmov.f32\t%q0, %1 @ <mode>";
>>> + return "vmov.f32\t%q0, %1 @ <mode>";
>>> else
>>> - sprintf (templ, "vmov.i%d\t%%q0, %%1 @ <mode>", width);
>>> + snprintf (templ, sizeof (templ),
>>> + "vmov.i%d\t%%q0, %%1 @ <mode>", width);
>>> return templ;
>>> }
>>> @@ -2346,12 +2348,13 @@ (define_insn "neon_vc<cmp_op><mode>_insn"
>>> && !flag_unsafe_math_optimizations)"
>>> {
>>> char pattern[100];
>>> - sprintf (pattern, "vc<cmp_op>.%s%%#<V_sz_elem>\t%%<V_reg>0,"
>>> - " %%<V_reg>1, %s",
>>> - GET_MODE_CLASS (<MODE>mode) == MODE_VECTOR_FLOAT
>>> - ? "f" : "<cmp_type>",
>>> - which_alternative == 0
>>> - ? "%<V_reg>2" : "#0");
>>> + snprintf (pattern, sizeof (pattern),
>>> + "vc<cmp_op>.%s%%#<V_sz_elem>\t%%<V_reg>0,"
>>> + " %%<V_reg>1, %s",
>>> + GET_MODE_CLASS (<MODE>mode) == MODE_VECTOR_FLOAT
>>> + ? "f" : "<cmp_type>",
>>> + which_alternative == 0
>>> + ? "%<V_reg>2" : "#0");
>>> output_asm_insn (pattern, operands);
>>> return "";
>>> }
>>> @@ -2370,10 +2373,11 @@ (define_insn
>>> "neon_vc<cmp_op_unsp><mode>_insn_unspec"
>>> "TARGET_NEON"
>>> {
>>> char pattern[100];
>>> - sprintf (pattern, "vc<cmp_op_unsp>.f%%#<V_sz_elem>\t%%<V_reg>0,"
>>> - " %%<V_reg>1, %s",
>>> - which_alternative == 0
>>> - ? "%<V_reg>2" : "#0");
>>> + snprintf (pattern, sizeof (pattern),
>>> + "vc<cmp_op_unsp>.f%%#<V_sz_elem>\t%%<V_reg>0,"
>>> + " %%<V_reg>1, %s",
>>> + which_alternative == 0
>>> + ? "%<V_reg>2" : "#0");
>>> output_asm_insn (pattern, operands);
>>> return "";
>>> }
>>>
>
