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

add MoveItems to RingBuf, fixes #19085 #19231

Merged
merged 1 commit into from
Nov 27, 2014
Merged

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Nov 23, 2014

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@Gankra
Copy link
Contributor Author

Gankra commented Nov 23, 2014

(still make checking)

Some(unsafe { mem::transmute(1u) })

unsafe {
Some(&mut *self.ptr.offset(tail as int))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, I removed this because I just noticed it wasn't necessary while I was in here. In the zero-sized case the ptr is non-null, offsetting it does nothing, so dereffing it should be A-ok. (also next_back just doesn't bother)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain about this change. offset lowers into an LLVM GetElementPtr with an inbounds annotation. According to this LLVM documentation,

With the inbounds keyword, the result value of the GEP is undefined if the address is outside the actual underlying allocated object and not the address one-past-the-end.

Since there is no underlying allocated object, this is a bit unclear, but it seems to imply that using an inbounds GEP on some random pointer produces undefined results.

This may be fine, as Rust never actually tries to dereference zero size pointers, and in another part of the same page discussing computing an address from null (analagous to what you are doing here), the docs say that

The underlying integer computation is sufficiently defined; null has a defined value — zero — and you can add whatever value you want to it.

However, it’s invalid to access (load from or store to) an LLVM-aware object with such a pointer. This includes GlobalVariables, Allocas, and objects pointed to by noalias pointers.

Unfortunately, that section is talking about plain GEP, without the inbounds marker. Even if LLVM currently doesn't do anything weird that would cause this to break, I think using offset here is technically invalid.

(Also, if this does turn out to be valid, this pattern of avoiding offsets of 0-size pointers is common, and all cases would be presumably simplified by using offset.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offsets like these are used fairly pervasively throughout slice and Vec. If this is wrong, we're standing on a very shaky foundation. I've asked about this before in IRC before and the general consensus is that it's safe to offset a junk zero-sized-pointer. I have an intuition to the effect of "this code gets completely stripped away in compilation, so offset is never actually called", but I might be wrong.

cc @thestinger @huonw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not already the case, Rust could define offset to always be valid on any pointer to a zero-sized type (e.g. if it's problematic for LLVM, the front-end can just detect these small types and output noop instead of the real GEPi).

I've asked on #llvm for additional input, but I think this is probably OK. (One can regard any place in memory as a valid instance of a zero-sized type.)

@ghost
Copy link

ghost commented Nov 23, 2014

Looks good to me.

@Gankra Gankra force-pushed the ringbuf-into-iter branch 2 times, most recently from 0d3d2db to 12b65d0 Compare November 23, 2014 17:25
@Gankra
Copy link
Contributor Author

Gankra commented Nov 23, 2014

Whoops, messed up my tests. Should be good now (still re-checking).

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 27, 2014
@bors bors merged commit 865c2db into rust-lang:master Nov 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants