This is the mail archive of the
mailing list for the GCC project.
Re: VRP: rewrite the division code (to handle corner cases including 0)
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Aldy Hernandez <aldyh at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 21 Aug 2018 11:46:12 +0200
- Subject: Re: VRP: rewrite the division code (to handle corner cases including 0)
- References: <email@example.com>
On Wed, Aug 15, 2018 at 3:33 AM Aldy Hernandez <firstname.lastname@example.org> wrote:
> In auditing the *_DIV_EXPR code I noticed that we were really botching
> some divisions where the divisor included 0.
> Particularly interesting was that we were botching something as simple
> as dividing by [0,0]. We were also incorrectly calculating things like
> [-2,-2] / [0, 5555], where we should have removed the 0 from the divisor.
> Also, the symbolic special casing could be handled by just treating
> symbolic ranges as [-MIN, +MAX] and letting the common code handle then.
> Similarly for anti ranges, which actually never happen except for the
> constant case, since they've been normalized earlier.
Note we also have "mixed" symbolic (anti-)ranges like [0, a]. I don't think
we handled those before but extract_range_to_wide_ints may be improved
to handle them. Likewise ranges_from_anti_range, ~[0, a] -> [-INF,
-1] u [a+1, +INF]
though not sure if that helps any case in practice.
> All in all, it was much easier to normalize all the symbolic ranges and
> treat everything generically by performing the division in two chunks...
> the negative numbers and the (non-zero) positive numbers. And finally,
> unioning the results. This makes everything much simpler to read with
> minimal special casing.
Yeah, nice work. Few comments:
+ TYPE_OVERFLOW_UNDEFINED (expr_type),
+ TYPE_OVERFLOW_WRAPS (expr_type),
we no longer have the third state !UNDEFINED && !WRAPS so I suggest
to eliminate one for the other (just pass TYPE_OVERFLOW_UNDEFINED).
+ /* If we're definitely dividing by zero, there's nothing to do. */
+ if (wide_int_range_zero_p (divisor_min, divisor_max, prec))
+ return false;
I know we didn't do this before but for x / 0 we should compute UNDEFINED
as range. With the current interfacing this special case would require handling
in the non-wide-int caller.
> Finally, my apologies for including a tiny change to the
> POINTER_PLUS_EXPR handling code as well. It came about the same set of
> auditing tests.
Bah, please split up things here ;) I've done a related change there
> It turns out we can handle POINTER_PLUS_EXPR(~[0,0], [X,Y]) without
> bailing as VR_VARYING in extract_range_from_binary_expr_1. In doing so,
> I also noticed that ~[0,0] is not the only non-null. We could also have
> ~[0,2] and still know that the pointer is not zero. I have adjusted
> range_is_nonnull accordingly.
But there are other consumers and it would have been better to
change range_includes_zero_p to do the natural thing (get a VR) and
then remove range_is_nonnull as redundant if possible.
> (Yes, we can get something like ~[0,2] for a pointer for things like the
> following in libgcc:
> if (segment_arg == (void *) (uintptr_type) 1)
> else if (segment_arg == (void *) (uintptr_type) 2)
> return NULL;
> else if (segment_arg != NULL)
> segment = (struct stack_segment *) segment_arg;
> BTW, I am still not happy with the entire interface to wide-int-range.*,
> and have another pending patchset that will simplify things even
> further. I think everyone will be pleased ;-).
> OK pending another round of tests?
The division related changes are OK, please split out & improve the nonnull
parts (and the POINTER_PLUS_EXPR check is already in as variant).