> 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
