On 10/1/20 5:05 AM, Aldy Hernandez via Gcc-patches wrote:
Thanks for doing all this! There isn't anything I don't understand
in the sprintf changes so no questions from me (well, almost none).
Just some comments:
Thanks for your comments on the sprintf/strlen API conversion.
The current call statement is available in all functions that take
a directive argument, as dir->info.callstmt. There should be no need
to also add it as a new argument to the functions that now need it.
Fixed.
The change adds code along these lines in a bunch of places:
+ value_range vr;
+ if (!query->range_of_expr (vr, arg, stmt))
+ vr.set_varying (TREE_TYPE (arg));
I thought under the new Ranger APIs when a range couldn't be
determined it would be automatically set to the maximum for
the type. I like that and have been moving in that direction
with my code myself (rather than having an API fail, have it
set the max range and succeed).
I went through all the above idioms and noticed all are being used on
supported types (integers or pointers). So range_of_expr will always
return true. I've removed the if() and the set_varying.
Since that isn't so in this case, I think it would still be nice
if the added code could be written as if the range were set to
varying in this case and (ideally) reduced to just initialization:
value_range vr = some-function (query, stmt, arg);
some-function could be an inline helper defined just for the sprintf
pass (and maybe also strlen which also seems to use the same pattern),
or it could be a value_range AKA irange ctor, or it could be a member
of range_query, whatever is the most appropriate.
(If assigning/copying a value_range is thought to be too expensive,
declaring it first and then passing it to that helper to set it
would work too).
In strlen, is the removed comment no longer relevant? (I.e., does
the ranger solve the problem?)
- /* The range below may be "inaccurate" if a constant has been
- substituted earlier for VAL by this pass that hasn't been
- propagated through the CFG. This shoud be fixed by the new
- on-demand VRP if/when it becomes available (hopefully in
- GCC 11). */
It should.
I'm wondering about the comment added to get_range_strlen_dynamic
and other places:
+ // FIXME: Use range_query instead of global ranges.
Is that something you're planning to do in a followup or should
I remember to do it at some point?
I'm not planning on doing it. It's just a reminder that it would be
beneficial to do so.
Otherwise I have no concern with the changes.
It's not cleared whether Andrew approved all 3 parts of the patchset
or just the valuation part. I'll wait for his nod before committing
this chunk.
Aldy
I have no issue with it, so OK.
Just an observation that should be pointed out, I believe Aldy has all
the code for converting to a ranger, but we have not pursued that any
further yet since there is a regression due to our lack of equivalence
processing I think? That should be resolved in the coming month, but at
the moment is a holdback/concern for converting these passes... iirc.
Andrew
