On 26 January 2017 at 00:31, Jeff Law <l...@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 <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
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
