On 09/05/2018 10:57 AM, Michael Matz wrote:
Hi,
On Wed, 5 Sep 2018, Aldy Hernandez wrote:
No apologies needed. I welcome all masochists to join me in my personal hell
:).
;-)
How can this test and following code catch all problematic cases? Assume
a range of [253..257], truncating to 8 bits unsigned. The size of the
range is 5 (not 4 as the code above calculates), well representable in 8
bits. Nevertheless, the min of the new range isn't 253, nor is the max of
the new range 257. In fact the new range must be [0..255] (if you have no
multi ranges or anti-ranges). So whatever this function is supposed to
do (what btw?), it certainly does not convert a range.
Welcome to the wonderful world of anti ranges, where nothing is as it
seems.
Yes, as I said, to precisely capture the result of '[253..257] & 255' you
either need multi-ranges or an anti-range.
First you're not looking at what's currently in mainline. Richard
approved the first incantation of this code. But even so, I believe the
above is correct as well.
Well, maybe as part of a larger sequence of code, but the above code
doesn't even return [253..1], there's actually no conversion/change of the
inputs done at all. Yes, the current code actually does the truncation.
I guess I was triggered by the name range_convert that didn't actually do
any conversion. I now have no issue with the code in mainline anymore
(well, maybe except disliking functions with eight parameters).
If only someone had suggested a class to stuff those parameters in... ;-).
If desired, I could further document that the range returned may have
its contents swapped and that is why it needs canonizing. But hey, that
shit was already broken when I got here ;-).
If you follow the code in tree-vrp.c before my patch you will see that
the path is the same... we calculate a nonsensical range of [253, 1] and
then massage it into ~[2, 252] in set_and_canonicalize_value_range.
Yes, I saw that, I think I was sceptical about splitting off a part of the
code that wasn't self-sufficient.
I think the fact that it's so hard to get all this right, is a strong
argument for getting rid of anti ranges. Almost all the bugs or
oversights I've fixed over the past months in VRP have been related to
anti ranges.
Well, without anti-ranges you need multi-ranges (or at least two-ranges);
it's a tradeoff. If I'd have to choose a high-level interface and the
choice is between supporting two-ranges directly (as two parameters) or a
single range/anti-range I'd always take the latter (internally of course
it would be represented as a two-range to make arguing and implementing
stuff easier).
Yup, in the ssa-range work we have multi-ranges, and they're kept in one
class we can pass as a unit.
Aldy
