/rust

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 memory leak in P::filter_map #57667

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Reviewers

@eddyb eddyb

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
7 participants
@ishitatsuyuki @Mark-Simulacrum @bors @rust-timer @nnethercote @eddyb @rust-highfive
@ishitatsuyuki
Copy link
Member

ishitatsuyuki commented Jan 16, 2019

Probably this function isn't widely used, but anyway this wasn't working as intended.

r? @eddyb

Do not rollup if you want to see if max-rss change in perf.

@rust-highfive rust-highfive assigned eddyb Jan 16, 2019

@rust-highfive rust-highfive added the S-waiting-on-review label Jan 16, 2019

@ishitatsuyuki
Fix memory leak in P::filter_map
Loading status checks…
763392c

@ishitatsuyuki ishitatsuyuki force-pushed the ishitatsuyuki:p-leak branch from d8c773c to 763392c Jan 16, 2019

@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Jan 16, 2019

@bors try
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 16, 2019

⌛️ Trying commit 763392c with merge 4fffff5...

bors added a commit that referenced this pull request Jan 16, 2019

@bors
Auto merge of #57667 - ishitatsuyuki:p-leak, r=<try> 
Fix memory leak in P::filter_map

Probably this function isn't widely used, but anyway this wasn't working as intended.

r? @eddyb

Do not rollup if you want to see if max-rss change in perf.
Loading status checks…
4fffff5
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 16, 2019

☀️ Test successful - checks-travis
State: approved= try=True
@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Jan 16, 2019

@rust-timer build 4fffff5
@rust-timer

This comment has been minimized.

Sign in to view
Copy link

rust-timer commented Jan 16, 2019

Success: Queued 4fffff5 with parent e2f221c, comparison URL.
@rust-timer

This comment has been minimized.

Sign in to view
Copy link

rust-timer commented Jan 16, 2019

Finished benchmarking try commit 4fffff5
@ishitatsuyuki

This comment has been minimized.

Sign in to view
Copy link
Member Author

ishitatsuyuki commented Jan 21, 2019 •
edited

r? @nnethercote

Edit: couldn't assign as he/she isn't a rust-lang member, but anyway requesting a review based on git blame.
@nnethercote

This comment has been minimized.

Sign in to view
Copy link
Contributor

nnethercote commented Jan 21, 2019

Nice catch. How did you find it?

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 21, 2019

📌 Commit 763392c has been approved by nnethercote

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Jan 21, 2019

Centril added a commit to Centril/rust that referenced this pull request Jan 21, 2019

@Centril
Rollup merge of rust-lang#57667 - ishitatsuyuki:p-leak, r=nnethercote 
Fix memory leak in P::filter_map

Probably this function isn't widely used, but anyway this wasn't working as intended.

r? @eddyb

Do not rollup if you want to see if max-rss change in perf.
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
d29a1bc

@Centril Centril referenced this pull request Jan 21, 2019

Closed

Rollup of 8 pull requests #57823

bors added a commit that referenced this pull request Jan 22, 2019

@bors
Auto merge of #57823 - Centril:rollup, r=Centril 
Rollup of 8 pull requests

Successful merges:

 - #57294 (When using value after move, point at span of local)
 - #57552 (Default images)
 - #57667 (Fix memory leak in P::filter_map)
 - #57677 (const_eval: Predetermine the layout of all locals when pushing a stack frame)
 - #57730 (Merge visitors in AST validation)
 - #57791 (Add regression test for #54582)
 - #57795 (Use structured suggestion in stead of notes)
 - #57809 (Add powerpc64-unknown-freebsd)

Failed merges:

r? @ghost
Loading status checks…
4aeeaab
@ishitatsuyuki

This comment has been minimized.

Sign in to view
Copy link
Member Author

ishitatsuyuki commented Jan 22, 2019

Nothing special, I was just learning the AST internals and during the review of the source this came up.

Centril added a commit to Centril/rust that referenced this pull request Jan 22, 2019

@Centril
Rollup merge of rust-lang#57667 - ishitatsuyuki:p-leak, r=nnethercote 
Fix memory leak in P::filter_map

Probably this function isn't widely used, but anyway this wasn't working as intended.

r? @eddyb

Do not rollup if you want to see if max-rss change in perf.
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
7fb44e8

@Centril Centril referenced this pull request Jan 22, 2019

Merged

Rollup of 9 pull requests #57830

bors added a commit that referenced this pull request Jan 22, 2019

@bors
Auto merge of #57830 - Centril:rollup, r=Centril 
Rollup of 9 pull requests

Successful merges:

 - #57537 (Small perf improvement for fmt)
 - #57552 (Default images)
 - #57604 (Make `str` indexing generic on `SliceIndex`.)
 - #57667 (Fix memory leak in P::filter_map)
 - #57677 (const_eval: Predetermine the layout of all locals when pushing a stack frame)
 - #57791 (Add regression test for #54582)
 - #57798 (Corrected spelling inconsistency)
 - #57809 (Add powerpc64-unknown-freebsd)
 - #57813 (fix validation range printing when encountering undef)

Failed merges:

r? @ghost
Loading status checks…
ad30e9a

@bors bors merged commit 763392c into rust-lang:master Jan 22, 2019
1 check passed

1 check passed

homu Test successful
Details

@ishitatsuyuki ishitatsuyuki deleted the ishitatsuyuki:p-leak branch Jan 22, 2019

@eddyb

eddyb reviewed Jan 30, 2019
View changes

src/libsyntax/ptr.rs
@@ -101,6 +101,7 @@ impl<T: 'static> P<T> {
// Recreate self from the raw pointer.
Some(P { ptr: Box::from_raw(p) })
} else {
drop(Box::from_raw(p));

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Jan 30, 2019

Member

This is wrong, because of the ptr::read(p) call above - you need to deallocate without dropping the T value inside the Box.

ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Jan 31, 2019

@ishitatsuyuki
Fix fallout from rust-lang#57667
58ca5dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment