[PATCH 4/5] distinguish likely and unlikely results (PR 78703)

[Christophe Lyon]

On 26 January 2017 at 00:31, Jeff Law <law@redhat.com> wrote:
> On 01/22/2017 04:53 PM, Martin Sebor wrote:
>>
>> The attached patch adds the concept of likely and unlikely results
>> of formatted functions to improve the quality of diagnostics (reduce
>> false positives and negatives) while at the same time allowing for
>> rare cases such as a multibyte decimal point in floating point output
>> or excessive width or precision bounds (when width and precision range
>> support is added, in the final patch of the series).  To this end,
>> this patch makes the following changes:
>>
>> 1) Eliminate the format_result exact byte counter (since its value
>> can be determined from the other counters) and introduce the likely
>> and unlikely counters.  The likely counter is considered (along
>> with the min and max counters) when deciding whether a directive
>> that may write too many bytes should be diagnosed.  The unlikely
>> counter is used as the worst case scenario by the return value
>> optimization but not to trigger diagnostics.
>>
>> 2) Eliminate the format_result::bounded flag since its value can
>> be derived from the counters.  This simplifies the warning logic.
>>
>> 3) Introduce the fmtresult::adjust_for_width_or_precision() function
>> and factor code that handled width and precision in individual type-
>> specific format_xxx handlers out of those handlers and into it.
>> This reduce code duplication, avoiding subtle inconsistencies.
>>
>> 4) Introduce the type_max_digits() function to compute the likely
>> amount of output for integer directives with unknown (and thus
>> possibly unlimited) precision and width.  This reduces the rate
>> of false positives.
>>
>> 5) Consolidate the min_bytes_remaining() function into
>> bytes_remaining() to handle all counters consistently.  This again
>> reduces code duplication and subtle inconsistencies.
>>
>> 6) Complete the consolidation of handling sequences of plain format
>> characters with format specifications (that start with %) and remove
>> the add_bytes function.
>>
>> 7) Introduce the should_warn_p() function and move the logic
>> to determine whether the result of a format directive should be
>> diagnosed out of format_directive and into it.
>>
>> 8) Update diagnostics to make use of the new counters.
>>
>> gcc-78703-4.diff
>>
>>
>> commit c9a95d19eb307b7df06c1285325b23746ddbc738
>> Author: Martin Sebor <msebor@redhat.com>
>> Date:   Sun Jan 22 11:48:13 2017 -0700
>>
>>     2017-01-22  Martin Sebor  <msebor@redhat.com>
>>
>>         * gimple-ssa-sprintf.c (struct result_range): Add likely and
>>         unlikely counters.
>>         (struct format_result): Replace number_chars, number_chars_min,
>>         and number_chars_max with a single member of struct result_range.
>>         Remove bounded.
>>         (format_result::operator+=): Adjust.
>>         (struct fmtresult): Remove bounded.  Handle likely and unlikely
>>         counters.
>>         (fmtresult::adjust_for_width_or_precision): New function.
>>         (fmtresult:type_max_digits): New function.
>>         (bytes_remaining): Handle likely and unlikely counters.
>>         (min_bytes_remaining): Remove.
>>         (format_percent): Simplify.
>>         (format_integer, format_floating): Set likely and unlikely
>> counters.
>>         (get_string_length, format_character, format_string): Same.
>>         (format_plain, should_warn_p): New function.
>>         (maybe_warn): Call should_warn_p.  Update diagnostic messages
>>         and handle those for all directives, including plain strings.
>>         (format_directive): Handle likely and unlikely counters.
>>         Remove unnecessary quoting from diagnostics.  Add an informational
>>         note.
>>         (add_bytes): Remove.
>>         (pass_sprintf_length::compute_format_length): Simplify.
>>         (try_substitute_return_value): Handle likely and unlikely
>> counters.
>>
>>     gcc/testsuite/
>>         * gcc.dg/tree-ssa/builtin-snprintf-warn-2.c: Adjust.
>>         * gcc.dg/tree-ssa/builtin-sprintf-2.c: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf-5.c: Same.
>>         * 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: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf-warn-6.c: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf-warn-9.c: Same.
>>         * gcc.dg/tree-ssa/builtin-sprintf.c: Same.
>
> So I see the introduction of many
>
> if (const OP object) expressions
>
> Can you please fix those as an independent patch after #4 and #5 are
> installed on the trunk?  Consider that patch pre-approved, but please post
> it here for the historical record.
>
> I think a regexp of paren followed by a constant would probably take you to
> them pretty quickly.
>
> I'm going to trust that the actual computations, particularly in format_*
> are correct.  You know this stuff far better than I.
>
> I probably would have tried to break this down further, but in the interests
> if getting this wrapped up, I've just slogged my way through it.    For the
> future, I would have tried to break each format_XXX patch out.
> Should_warn_p and the simplifications it enabled would have been another
> patch, etc.
>
>
> OK for the trunk.
>
> jeff


Hi,

With this patch all my builds for aarch64/arm failed:
/gcc/gimple-ssa-sprintf.c: In function
‘<unnamed>::fmtresult<unnamed>::format_floating(const<unnamed>::direc
tive&, tree_node*)’:
/gcc/gimple-ssa-sprintf.c:1643: error: ‘XFmode’ was not declared in this scope

Is this fixed later in the series?

Christophe