On Tue, Nov 8, 2016 at 5:03 AM, Martin Sebor <mse...@gmail.com> wrote:
> It's taken me longer than I expected to finally get back to this
> project. Sorry about the delay.
>
> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01110.html
>
> Attached is an updated patch with this enhancement and reflecting
> you previous comment.
>
> Besides running the GCC test suite I tested the patch by building
> Binutils and the Linux kernel. It found one stpcpy-related overflow
> in Binutils that I'm looking into and reduced by one the number of
> problems reported by the -Wformat-length option in the kernel (I
> haven't yet checked which one it eliminated).
>
> Although I'm not done investigating the Binutils problem I'm posting
> the patch for review now to allow for comments before stage 1 ends.
@@ -158,14 +170,149 @@ compute_object_offset (const_tree expr, const_tree var)
return size_binop (code, base, off);
}
+static bool
+operand_unsigned_p (tree op)
+{
+ if (TREE_CODE (op) == SSA_NAME)
new functions need a comment. But maybe you want to use tree_expr_nonnegative_p
to also allow signed but known positive ones?
+/* Fill the 2-element OFFRANGE array with the range of values OFF
+ is known to be in. Postcodition: OFFRANGE[0] <= OFFRANGE[1]. */
+
+static bool
+get_offset_range (tree off, HOST_WIDE_INT offrange[2])
+{
+ STRIP_NOPS (off);
why strip nops (even sign changes!) here? Why below convert things
via to_uhwi when offrange is of type signed HOST_WIDE_INT[2]?
+ gimple *def = SSA_NAME_DEF_STMT (off);
+ if (is_gimple_assign (def))
+ {
+ tree_code code = gimple_assign_rhs_code (def);
+ if (code == PLUS_EXPR)
+ {
+ /* Handle offset in the form VAR + CST where VAR's type
+ is unsigned so the offset must be the greater of
+ OFFRANGE[0] and CST. This assumes the PLUS_EXPR
+ is in a canonical form with CST second. */
+ tree rhs2 = gimple_assign_rhs2 (def);
err, what? What about overflow? Aren't you just trying to decompose
'off' into a variable and a constant part here and somehow extracting a
range for the variable part? So why not just do that?
+ else if (range_type == VR_ANTI_RANGE)
+ {
+ offrange[0] = max.to_uhwi () + 1;
+ offrange[1] = min.to_uhwi () - 1;
+ return true;
+ }
first of all, how do you know it fits uhwi? Second, from ~[5, 9] you get
[10, 4] !? That looks bogus (and contrary to the function comment
postcondition)
+ else if (range_type == VR_VARYING)
+ {
+ gimple *def = SSA_NAME_DEF_STMT (off);
+ if (is_gimple_assign (def))
+ {
+ tree_code code = gimple_assign_rhs_code (def);
+ if (code == NOP_EXPR)
+ {
please trust range-info instead of doing your own little VRP here.
VR_VARYING -> return false.
stopping review here noting that other parts of the compiler
split constant parts from variable parts and it looks like this is
what you want to do here? That is, enhance
static vec<unsigned HOST_WIDE_INT> object_sizes[4];
and cache a SSA-NAME, constant offset pair in addition? Or just a range
(range of that SSA name + offset), if that's good enough -- wait, that's
what you get from get_range_info!
So I'm not sure where the whole complication in the patch comes from...
Possibly from the fact that VRP on pointers is limited and thus &a[i] + 5
doesn't get you a range for i + 5?
Richard.
> Martin
>
> PS The tests added in the patch (but nothing else) depend on
> the changes in the patch for c/53562:
>
> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00483.html
>
