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 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). Ciao, Michael.
