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 <mse...@redhat.com>
Date: Sun Jan 22 11:48:13 2017 -0700
2017-01-22 Martin Sebor <mse...@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
