Repository navigation
PtrExt::offset taking an isize argument is not ideal #22104
Description
PtrExt::offset taking an isize argument prevents being able to advance forward / back the full possible range of a pointer. A better option might be to have separate functions for forwards / backwards that take usize arguments.
Activity
[-]ptr::offset taking an isize argument is not ideal[/-][+]PtrExt::offset taking an isize argument is not ideal[/+]steveklabnik commentedon Feb 9, 2015
/cc @aturon
bluss commentedon Feb 9, 2015
It's a fundamental issue with LLVM, see #18726 and GEP docs
cc @gankro
Gankra commentedon Feb 9, 2015
With high probability we will add
addandsubmethods which take uints, because 99% of the time you have a uint, and 99% of the time you're doing an add. That said, they will just be sugar for casting to anint, because that's just what the ptr offset intrinsic in LLVM takes.It's not like anyone ever does a check for "the uint is too large" when they cast, so this would just be an ergonomic improvement. I suppose we could add some friendly debug asserts in there.
That said, this API taking a signed int is a soundness issue for us due to being able to allocate more than int::MAX contiguous bytes in some situations. There are a few options to address this, but all of them have drawbacks.
bluss commentedon Feb 9, 2015
Not sure we need those methods. I'll be the conservative side and say that hiding the problem doesn't help. Users of .offset need to be aware, and a cast to isize might help them with that.
carllerche commentedon Feb 9, 2015
Why is the intrinsic different than casting the ptr to usize, modifying it, and casting back to a ptr?
bluss commentedon Feb 9, 2015
That's this question in the GEP FAQ.
thestinger was the guy to make sure Rust used GEP so that llvm could optimize much better.
Thiez commentedon Feb 10, 2015
The maximum object size in Rust is equal to
std::isize::MAX, see here. It cannot be the case that you need to offset a pointer more than that, or you're probably invoking undefined behavior.Gankra commentedon Feb 10, 2015
@Thiez that specification was of course added in response to this problem, so it's a bit of a circular argument.
Thiez commentedon Feb 11, 2015
You couldn't do this in C either, because the difference between two pointers has type
ptrdiff_t, which is signed. I see we don't check for this when allocating memory, which should probably be considered a bug.aturon commentedon Feb 16, 2015
Nominating for 1.0-beta P-backcompat-libs. We need to resolve this issue.
pnkfelix commentedon Feb 19, 2015
assigning to self (to get answers to some questions I have)
pnkfelix commentedon Feb 19, 2015
Not 1.0 blocker based on current understanding of situation. P-high.
alexcrichton commentedon Jan 21, 2016
The libs team discussed this in triage yesterday and the conclusion was that this cannot be changed due to it being stable, and the situation is also intended due to the limitations imposed by LLVM.
Pointer offsets need to be able to go backwards (e.g. move around in an array), but it's undefined behavior to overflow, so the only defined way to do this is to have a signed offset.
alexcrichton commentedon Jan 21, 2016
I also just reread the docs and I think that they adequately reflect this.
Merge pull request rust-lang#22104 from ChayimFriedman2/reinit-cache