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

Fix TypedArena returning wrong pointers for recursive allocations #67003

Merged
merged 1 commit into from Dec 9, 2019

Conversation

@cjgillot
Copy link
Contributor

cjgillot commented Dec 3, 2019

Closes #67001

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 3, 2019

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 3, 2019

Yeah, the old code seems pretty clearly "just wrong" -- it doesn't even check if there are remaining elements in the iterator; size_hint can lie and it looks like we were previously just leaving elements in the iterator with no checks at all. Hopefully nothing was returning false-positive Some for the upper bound on size_hint...

@bors try @rust-timer queue -- let's try to get a sense of whether the slowpath is actually slower, to see if it's worth looking at optimizing this piece of code. cc @nnethercote

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 3, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 3, 2019

⌛️ Trying commit d84eb50 with merge f0ced29...

bors added a commit that referenced this pull request Dec 3, 2019
Fix TypedArena returning wrong pointers for recursive allocations

Closes #67001
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

☀️ Try build successful - checks-azure
Build commit: f0ced29 (f0ced29c268226d9dd0cb2e3bf61645a179c68f4)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 4, 2019

Queued f0ced29 with parent 7afe6d9, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 4, 2019

Finished benchmarking try commit f0ced29, comparison URL.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 4, 2019

~2.5% regression on one benchmark. Given that this is plausibly a correctness fix, I'm going to say that's fine, particularly since we've improved that benchmark ~14% in the last month (so we're losing some wins, but not regressing too much overall). Hopefully we can find a way to recover some of the performance over time.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

📌 Commit d84eb50 has been approved by Mark-Simulacrum

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 5, 2019

cc @SimonSapin : can you double check if crates.io typed_arena copied over this same code path when it was branched from the compiler's arena crate?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 5, 2019

Sorry, I haven’t touched that crate in years and to be honest I’m not that interested in maintaining it. A few other people have write access on GitHub and crates.io. Feel free to look at the code and/or file an issue on that repo.

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 6, 2019

I guess we'd need panic=abort to restore this optimization since we could then allocate the entire slice at once and then write into it.

@Mark-Simulacrum size_hint is not allowed to lie, but it's not UB for it to do so. It should really be called size_bounds.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 6, 2019

Whether you have panic=abort or not, you still need to be careful around updating the pointer -- in particular, if the next method also calls into the arena, you'd potentially run into trouble if that writes into the space allocated for the iterator space.

Hm, I'm not sure what you mean by "is not allowed to lie but it is not UB for it to do so." To my understanding, size_hint can of course be incorrect (i.e., buggy) though of course we should fix such bugs when we find them. We can never rely on it to be correct though from a soundness perspective, and in the compiler, that's more so than just memory correctness - leaving elements of the iterator unconsumed when interning can mean that we're forgetting bounds on types, etc., which is a potential cause for soundness holes. I personally expect that with the right logic, we could restore the implementation which pre-reserves space for the iterator's elements, though I don't have the time to work on that implementation myself.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 6, 2019

Would TrustedLen help here?

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 6, 2019

@SimonSapin No it would not.

The problem occurs if you panic when calling next on the iterator. We need a way to ensure that we free the objects that have already been moved into the arena and not any with uninitialized memory.

Centril added a commit to Centril/rust that referenced this pull request Dec 7, 2019
Fix TypedArena returning wrong pointers for recursive allocations

Closes rust-lang#67001
bors added a commit that referenced this pull request Dec 7, 2019
Rollup of 7 pull requests

Successful merges:

 - #66730 (remove dependency from libhermit)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66927 (Miri core engine: use throw_ub instead of throw_panic)
 - #66984 (Move clean types into their own file)
 - #66991 (Cleanup BodyCache)
 - #67003 (Fix TypedArena returning wrong pointers for recursive allocations)
 - #67004 (Do not ICE on async fn with non-Copy infered type arg)

Failed merges:

r? @ghost
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 7, 2019

Panic in next is also a concern for example in Vec::extend, so this isn’t entirely new territory. What is (relatively) new is mutation through &self, so next could call something that inserts other things in the arena during iteration.

What if this method made or took a local TypedArenaChunk that is not in self.chunks, ensuring it has exclusive access to it, and only added it (back) to self.chunks after the end of iteration?

Centril added a commit to Centril/rust that referenced this pull request Dec 8, 2019
Fix TypedArena returning wrong pointers for recursive allocations

Closes rust-lang#67001
bors added a commit that referenced this pull request Dec 8, 2019
Rollup of 6 pull requests

Successful merges:

 - #66892 (Format libcore with rustfmt (including tests and benches))
 - #67003 (Fix TypedArena returning wrong pointers for recursive allocations)
 - #67004 (Do not ICE on async fn with non-Copy infered type arg)
 - #67016 (In which we implement illegal subset relations errors using Polonius)
 - #67031 (Update tokio crates to latest versions)
 - #67138 (Simplify `Layout::extend_packed`)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

⌛️ Testing commit d84eb50 with merge 2b0e6d2...

bors added a commit that referenced this pull request Dec 9, 2019
Fix TypedArena returning wrong pointers for recursive allocations

Closes #67001
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 2b0e6d2 to master...

@bors bors added the merged-by-bors label Dec 9, 2019
@bors bors merged commit d84eb50 into rust-lang:master Dec 9, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191203.75 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@cjgillot cjgillot deleted the cjgillot:corrida branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.