[llvm] r203273 - [ADT] Update PointerIntPair to handle pointer types with more than 31 bits free.

[David Blaikie]

On Fri, Mar 7, 2014 at 1:07 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> On Mar 7, 2014, at 12:46 , David Blaikie <dblaikie at gmail.com> wrote:
>
> On Fri, Mar 7, 2014 at 11:57 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>
> On Mar 7, 2014, at 11:32, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Fri, Mar 7, 2014 at 11:19 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> Author: jrose
> Date: Fri Mar  7 13:19:56 2014
> New Revision: 203273
>
> URL: http://llvm.org/viewvc/llvm-project?rev=203273&view=rev
> Log:
> [ADT] Update PointerIntPair to handle pointer types with more than 31 bits
> free.
>
> Previously, the assertions in PointerIntPair would try to calculate the
> value
> (1 << NumLowBitsAvailable); the inferred type here is 'int', so if there
> were
> more than 31 bits available we'd get a shift overflow.
>
>
> I'm guessing this was found by the static analyzer? (how/why? the
> "bits free" would be an template parameter, not a runtime value - and
> I assume we never even approach 32 bits free... seems sort of like a
> false positive?)
>
> Or do we really have such a pointer somewhere that's aligned to 4 GB?
>
>
> I bet the analyzer could find it if such a path occurred, but the real
> answer is much more mundane: I've been working on an internal tool that
> actually has a Fixnum class like the one in the test case, which was
> originally just being used in a PointerUnion (alongside a real pointer). I
> tried using the same Fixnum<31> with a PointerIntPair, and Clang gave me a
> warning (-Wshift-op-overflow, I think).
>
>
> Still a little confused - what kind of pointer were you going to fit
> in the PointerIntPair? (& if you couldn't fit a pointer there, why not
> just use the int directly and skip the pair)
>
>
> It's actually
>
> using Offset = Fixnum<31>;
> using OffsetOrRecord = PointerUnion<Offset, Record *>;
>
> // added today
> using OffsetAndShouldForce = PointerIntPair<Offset, 1, bool>;
>
> On a 64-bit system this is the same amount of space as std::pair<Offset,
> bool>, but on a 32-bit system it fits in 32 bits. *shrug* I guess you're
> right that I should just bit-mangle it myself.

Oh, OK - so you're putting this non-pointer thing as the pointer part
of the pointer-int-pair. Fair enough... hadn't even really occurred to
me. If PointerIntPair works for non-pointers, perhaps we should just
rename it (but I realize that's invasive) - but that seems OK to make
the tool more general to handle those cases if it comes out nicely.

(maybe rename it & provide a template alias for the old name... but
also probably not worth worrying about)