On Wed, Aug 15, 2018 at 3:33 AM Aldy Hernandez <al...@redhat.com> wrote: > > Howdy! > > 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 yesterday... > > 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). Richard. > > Aldy
