On 01/09/2017 09:18 AM, Martin Sebor wrote:
On 01/08/2017 09:44 PM, Jeff Law wrote:
As much as I hate to say it, I think we need to find a way to break this
down into something more manageable. It's just impossible to see the
structure of what you're doing with the implementation changes mixed in
with what appear to be refactoring changes.
Is there any way we can start to break this down into something more
manageable?
I'm sorry it's too big. I actually did start out by doing in stages
and and have a smaller initial patch that the rest of it builds on.
I unfortunately forgot that I had done that when I posted the bigger
patch and then left for my trip.
Attached are the two revisions. The description of the major changes
in each of the two is below. The patches are bigger in part because
of the name change (conversion_spec to directive and spec to dir),
but also because the range work provided an opportunity to factor
out and consolidate a good amount of code. IMO, the end result is
quite a bit cleaner than before and should be easier to follow.
I'm not sure how difficult or time consuming it would be to split
up the second of the two patches if it's still too big. I suspect
it would take a few days.
gcc-78703-1.diff
Extends struct conversion_spec to struct directive so that ordinary
format characters can be handled uniformly with other directives
(those that start with a '%') and adds format_plain to format them.
It also adds parse_directive and moves directive parsing from
pass_sprintf_length::compute_format_length to it.
It makes no changes to the warning or optimization logic and so
the test updates only to reflect the uniform directive handling.
gcc-78703-2.diff
Contains the bulk of the rest of the changes. I.e., it replaces
the superfluous exact byte counter with the likely counter and
adds the unlikely counter.
It replaces the get_width_and_precision function with get_int_range
to get ranges for width and precision ranges and adds
fmtresult::adjust_for_width_or_precision to handle with and precision
ranges.
It adds tge should_warn_p function and moves all logic to determine
whether or not to issue a warning at a given point in the processing
of a format string to it.
It adds the maybe_warn function and ymoves all the format directive
overflow and truncation warnings into it.
It removes the add_bytes function (its fully superseded by
format_directive.
Finally, this version also enhances the debugging output to include
width and precision ranges.
Martin
gcc-78703-1.diff
commit 366503058ae7fa8ff689b258f728010d942e0354
Author: Martin Sebor <mse...@redhat.com>
Date: Fri Dec 16 16:13:10 2016 -0700
Rename struct conversion_spec with struct directive.
Add format_plain to format plain format substrings.
Change format_directive to handle plain format substrings
and to print called function name in diagnostics
Add parse_directive.
Simplify pass_sprintf_length::compute_format_length and
call parse_directive.
Adjust tests.
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index a91dcb8..22ef86b 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -1838,32 +1848,42 @@ format_string (const conversion_spec &spec, tree arg)
return res;
}
+
+static fmtresult
+format_plain (const directive &spec, tree)
Needs a trivial function comment.
else if (navail < fmtres.range.max
- && (((spec.specifier == 's'
+ && (((dir.specifier == 's'
&& fmtres.range.max < HOST_WIDE_INT_MAX)
- /* && (spec.precision || spec.star_precision) */)
+ /* && (dir.precision || dir.star_precision) */)
|| 1 < warn_format_length))
Any good reason to keep the commented out code here? If not, let's
remove it.
With an updated changelog the -1 patch is OK. It was a *lot* easier to
see what you were doing here.
Jeff
