[PATCH] fix outstanding -Wformat-length failures (pr77735 et al.)

Jeff Law law@redhat.com
Wed Oct 19 22:02:00 GMT 2016


On 10/17/2016 03:59 PM, Martin Sebor wrote:
>
>
> On 10/17/2016 01:11 PM, Jeff Law wrote:
>> On 10/02/2016 02:10 PM, Martin Sebor wrote:
>>> The attached patch fixes a number of outstanding test failures
>>> and ILP32-related bugs in the gimple-ssa-sprintf pattch pointed
>>> out in bug 77676 and 77735).  The patch also fixes c_strlen to
>>> correctly handle wide strings (previously it accepted them but
>>> treated them as nul-terminated byte sequences), and adjusts the
>>> handling of "%a" to avoid assuming a specific number of decimal
>>> digits (this is likely a defect in C11 that I'm pursuing with
>>> WG14).
>>>
>>> Tested on powerpc64le, i386, and x86_64.
>>>
>>> There is one outstanding failure in the builtin-sprintf-warn-1.c
>>> test on powerpc64le that looks like it might be due to the
>>> printf_pointer_format target hook not having been set up entirely
>>> correctly.  I'll look into that separately, along with pr77819.
>>>
>>> Martin
>>>
>>> gcc-77735.diff
>>>
>>>
>>> PR middle-end/77735 - FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
>>>
>>> gcc/ChangeLog:
>>> 2016-10-02  Martin Sebor  <msebor@redhat.com>
>>>
>>>     PR middle-end/77735
>>>     * builtins.c (string_length): New function.
>>>     (c_strlen): Use string_length.  Correctly handle wide strings.
>>>     * gimple-ssa-sprintf.c (target_max_value, target_size_max): New
>>>     functions.
>>>     (target_int_max): Call target_max_value.
>>>     (format_result::knownrange): New data member.
>>>     (fmtresult::fmtresult): Define default constructor.
>>>     (format_integer): Use it and set format_result::knownrange.
>>>     Handle global constants.
>>>     (format_floating_max): Add third argument.
>>>     (format_floating): Recompute maximum value for %a for each argument.
>>>     (get_string_length): Use fmtresult default ctor.
>>>     (format_string): Set format_result::knownrange.
>>>     (format_directive): Check format_result::knownrange.
>>>     (add_bytes): Same.  Correct caret placement in diagnostics.
>>>     (pass_sprintf_length::compute_format_length): Set
>>>     format_result::knownrange.
>>>     (pass_sprintf_length::handle_gimple_call): Use target_size_max.
>>>
>>> gcc/testsuite/ChangeLog:
>>> 2016-10-02  Martin Sebor  <msebor@redhat.com>
>>>
>>>     PR middle-end/77735
>>>     * gcc.dg/tree-ssa/builtin-sprintf-2.c: Add test cases.
>>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Same.
>>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: Same.
>>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Adjust/relax.
>>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: Add test cases.
>>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-6.c: XFAIL for LP64 only.
>>>     * gcc.dg/tree-ssa/builtin-sprintf.c: Add test cases.
>>>
>>> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
>>> index 92f939e..410bbc3 100644
>>> --- a/gcc/gimple-ssa-sprintf.c
>>> +++ b/gcc/gimple-ssa-sprintf.c
>>> @@ -1281,9 +1330,9 @@ format_floating (const conversion_spec &spec,
>>> int width, int prec)
>>>      res.range.max = width;
>>>      }
>>>
>>> -  /* The argument is only considered bounded when the range of output
>>> -     bytes is exact.  */
>>> -  res.bounded = res.range.min == res.range.max;
>>> +  /* The argument is considered bounded when the output of a directive
>>> +     is fully specified and the range of output bytes is exact.  */
>>> +  // res.bounded &= res.range.min == res.range.max;
>> Did you mean to leave this commented out?
>
> I'm pretty sure I didn't mean to leave the code commented out like
> that.  I suspect I commented it out to confirm that it's not needed,
> and forgot to remove it after it passed my testing.  Let me remove
> it (and thanks for pointing it out!)
>
>> It looks like you're defining a constructor for "struct fmtresult".
>> Normally once we define a constructor we turn the underlying object into
>> a class.    It appears that the constructor leaves argmin, argmax,
>> knownrange, bounded & constant uninitialized?  Am I missing something
>> here?
>
> I added the default ctor to avoid having to explicitly clear the
> members.  The member() syntax means to default-initialize them by
> setting them to zero or null:
>
>  struct fmtresult
>  {
> +  fmtresult ()
> +  : argmin (), argmax (), knownrange (), bounded (), constant ()
> +  {
> +    range.min = range.max = HOST_WIDE_INT_MAX;
> +  }
> +
>
> Or did you have something else in mind?  (I want to leave the members
> public so they can conveniently accessed.)
Ah, thanks for clarifying.

>
>>
>>
>>> +      /* A plain '%c' directive.  Its ouput is excactly 1.  */
>> Typo for exactly.
>>
>
> Will fix.
>
> With that and with the comment above removed, would like me to post
> an updated patch or is it okay commit?
OK to commit after the fix in floating_format (assignment to 
res.bounded) and the commit fix.

jeff



More information about the Gcc-patches mailing list