On 08/11/2016 08:13 PM, Martin Sebor wrote:
Attached is an updated patch with changes addressing most of
the suggestions and comments I received.
The main changes/improvements are the following:
* New option, -fprintf-return-value, enables the sprintf return
value optimization. It's disabled by default in this version
but I'm hoping to enable in the final version of the patch.
I added it in this version mainly to be able to verify the
correctness of the pass by comparing the sprintf return value
computed by the pass to the expected result returned from
the same libc call. (Before I had to rely solely on results
hardcoded in the tests).
* With the -fdump-tree-printf-return-value the pass generates
simple dump output to indicate what has been optimized and
what hasn't.
* Floating point formatting relies on mpfr_snprintf to determine
the size of the output for known values. This lets the pass
avoid duplicating the tricky floating point math done by sprintf
and getting it wrong.
* New target hooks remove hardcoding target-specific assumptions
about libc implementation-specific details (%p format and printf
floating point rounding mode).
What's missing is integration with David's latest location range
changes. I plan to work on it next but this seemed like a good
stopping point to get feedback.
There also are still opportunities for improvements to the string
length computation (now via gimple-fold.c's get_maxval_strlen,
thanks to Jakub) to improve checking of %s directives. But since
that's a general-purpose function that's used outside this pass
it will be a separate patch.
My biggest concern with this iteration is the tight integration between
the optimization and warning. We generally avoid that kind of tight
integration such that enabling the warning does not affect the
optimization and vice-versa.
So ISTM you have to do the analysis if the optimization or warning has
been requested. Then you conditionalize whether or not the warnings are
emitted by their flag and the optimization based on its flag.
I understand you're going to have some further work to do because of
conflicts with David's patches. With that in mind, I'd suggest a bit of
carving things up so things can start moving forward.
Patch #1. All the fixes to static buffer sizes that were inspired by
your warning. These are all approved and can go in immediately.
Patch #2. Improvement to __builtin_object_size to handle
POINTER_PLUS_EXPR on arrays. This is something that stands on it own
and ought to be reviewable quickly and doesn't really belong in the
bowels of the warning/optimization patch you're developing.
Patch #3. Core infrastructure and possibly the warning. The reason I
say possibly the warning is they may be intertwined enough that
separating them makes more work than it saves. I think the warning bits
are largely ready to go and may just need twiddling due to conflicts
with David's work.
Patch #4. The optimizations you've got now which I'll want to take
another look at. Other than the overly tight integration with the
warning, I don't see anything inherently wrong, but I would like to take
another look at those once #1-#3 are done and dusted.
Patch #5 and beyond: Further optimization work.
+
+static void
+compute_format_length (const pass_sprintf_length::call_info &info,
+ format_result *res,
+ const char *cvtbeg,
+ size_t cvtlen,
+ size_t offset,
+ const conversion_spec &spec,
+ tree arg)
Needs function comment. Please don't bother lining up arguments like
that. Just one space between type and its name.
+/* Given a suitable result RES of a call to a formatted output function
+ described by INFO, substitute the result for the return value of
+ the call. The result is suitable if the number of bytes it represents
+ is known and exact. */
+
+static void
+try_substitute_return_value (gimple_stmt_iterator gsi,
+ const pass_sprintf_length::call_info &info,
+ const format_result &res)
Single space between the type and the variable name. I only happened to
see this one because I was going to make a comment about this function,
so please check elsewhere in your new code.
+{
+ tree lhs = gimple_get_lhs (info.callstmt);
+ if (lhs && res.bounded && res.number_chars < HOST_WIDE_INT_MAX)
+ {
+ /* Replace the left-hand side of the call with the constant
+ result of the formatting function minus 1 for the terminating
+ NUL which the functions' return value does not include. */
+ gimple_call_set_lhs (info.callstmt, NULL_TREE);
+ tree cst = build_int_cst (integer_type_node, res.number_chars - 1);
+ gimple *g = gimple_build_assign (lhs, cst);
+ gsi_insert_after (&gsi, g, GSI_NEW_STMT);
+ update_stmt (info.callstmt);
Can you verify that SSA_NAME_DEF_STMT (lhs) points to the new statement
rather than the gimple call statement after this transformation? Just
verifying in the debugger is sufficient. Probably the easiest way would
be to p debug_tree (lhs) -- that will print the defining statement as well.
+ else if (dump_file)
+ {
+ location_t callloc = gimple_location (info.callstmt);
+ fprintf (dump_file, "On line %i ", LOCATION_LINE (callloc));
+ print_generic_expr (dump_file, info.func, dump_flags);
+
+ const char *ign = lhs ? "" : " ignored";
+ if (res.number_chars >= HOST_WIDE_INT_MAX)
+ fprintf (dump_file, " return value in range [%lu, %lu]%s.\n",
+ (unsigned long)res.number_chars_min,
+ (unsigned long)res.number_chars_max, ign);
+ else
+ fprintf (dump_file, " return value %lu%s.\n",
+ (unsigned long)res.number_chars, ign);
+ }
Consider setting a range via set_range_info in the else clause.
jeff
