Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PtrExt::offset taking an isize argument is not ideal #22104

Closed
carllerche opened this Issue Feb 9, 2015 · 14 comments

Comments

Projects
None yet
9 participants
@carllerche
Copy link
Member

carllerche commented Feb 9, 2015

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.

@carllerche carllerche changed the title ptr::offset taking an isize argument is not ideal PtrExt::offset taking an isize argument is not ideal Feb 9, 2015

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Feb 9, 2015

/cc @aturon

@steveklabnik steveklabnik added the A-libs label Feb 9, 2015

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Feb 9, 2015

It's a fundamental issue with LLVM, see #18726 and GEP docs

cc @Gankro

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented Feb 9, 2015

With high probability we will add add and sub methods 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 an int, 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

This comment has been minimized.

Copy link
Contributor

bluss commented 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

This comment has been minimized.

Copy link
Member Author

carllerche commented Feb 9, 2015

Why is the intrinsic different than casting the ptr to usize, modifying it, and casting back to a ptr?

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented 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

This comment has been minimized.

Copy link
Contributor

Thiez commented 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.

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented 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

This comment has been minimized.

Copy link
Contributor

Thiez commented 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

This comment has been minimized.

Copy link
Member

aturon commented Feb 16, 2015

Nominating for 1.0-beta P-backcompat-libs. We need to resolve this issue.

@aturon aturon added the I-nominated label Feb 16, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 19, 2015

assigning to self (to get answers to some questions I have)

@pnkfelix pnkfelix self-assigned this Feb 19, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 19, 2015

Not 1.0 blocker based on current understanding of situation. P-high.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented 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

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 21, 2016

I also just reread the docs and I think that they adequately reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.