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

leaked memory in rust main loop of ARM #6231

Closed
yichoi opened this issue May 4, 2013 · 10 comments
Closed

leaked memory in rust main loop of ARM #6231

yichoi opened this issue May 4, 2013 · 10 comments
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows O-android Operating system: Android

Comments

@yichoi
Copy link
Contributor

yichoi commented May 4, 2013

most of ARM test cases ended with segmentation fault

leaked memory in rust main loop (n objects)
/home/yichoi/rust_work/src/rt/memory_region.cpp:189: memory_region::~memory_region(): assertion "false" failed
segmentation fault

@ILyoan
Copy link
Contributor

ILyoan commented May 6, 2013

It looks to me that some managed boxes allocated in new task remain unreclaimed.

@ILyoan
Copy link
Contributor

ILyoan commented May 8, 2013

Looks like spawning new task yields this error.
But there are some test cases ended with leaked memory without spawning child task. cycle-collection.rs is one of those tests.
Here is the list of the test cases failed with this error.

@pcwalton
Copy link
Contributor

pcwalton commented May 9, 2013

I suspect task local data is not being cleaned up properly.

@brson
Copy link
Contributor

brson commented May 9, 2013

I'll take a look into this sometime.

@brson
Copy link
Contributor

brson commented May 14, 2013

I've confirmed that at least some of the leaks are from TLS (task-local storage)

@brson
Copy link
Contributor

brson commented May 14, 2013

This is a combination of at least two problems:

  1. TLS is leaking everything. cleanup_local_map is not succeeding at freeing boxes in TLS
  2. The box annihilator is not working. This can be shown by creating a simple test case that creates a GC box cycle. The box annihilator should free the cycle on task death and this isn't happening.

The first bug is only evident because of the latter.

@brson
Copy link
Contributor

brson commented May 15, 2013

Here's a reduced test case that shows the problem with TLS.

// -*- rust -*-
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use core::libc::c_void;
use core::libc;
use core::cast;

pub type rust_task = c_void;

pub type LocalDataKey<'self,T> = &'self fn(v: @T);

pub trait LocalData { }
impl<T: 'static> LocalData for @T { }

impl Eq for @LocalData {
    fn eq(&self, other: &@LocalData) -> bool {
        unsafe {
            let ptr_a: &(uint, uint) = cast::transmute(self);
            let ptr_b: &(uint, uint) = cast::transmute(other);
            return ptr_a == ptr_b;
        }
    }
    fn ne(&self, other: &@LocalData) -> bool { !(*self).eq(other) }
}

type TaskLocalElement = (*libc::c_void, *libc::c_void, @LocalData);
// Has to be a pointer at outermost layer; the foreign call returns void *.
type TaskLocalMap = @mut ~[Option<TaskLocalElement>];

fn cleanup_task_local_map(map_ptr: *libc::c_void) {
    unsafe {
        ::io::println("cleaning up task local map");
        assert!(!map_ptr.is_null());
        // Get and keep the single reference that was created at the
        // beginning.
        let map: TaskLocalMap = cast::transmute(map_ptr);
        *map == ~[];
        // All local_data will be destroyed along with the map.
    }
}

unsafe fn get_task_local_map(task: *rust_task) -> TaskLocalMap {

    extern fn cleanup_task_local_map_extern_cb(map_ptr: *libc::c_void) {
        cleanup_task_local_map(map_ptr);
    }

    // Relies on the runtime initialising the pointer to null.
    // Note: The map's box lives in TLS invisibly referenced once. Each time
    // we retrieve it for get/set, we make another reference, which get/set
    // drop when they finish. No "re-storing after modifying" is needed.
    let map_ptr = rust_get_task_local_data(task);
    if map_ptr.is_null() {
        let map: TaskLocalMap = @mut ~[];
        rust_set_task_local_data(task, cast::transmute(map));
        rust_task_local_data_atexit(task, cleanup_task_local_map_extern_cb);
        // Also need to reference it an extra time to keep it for now.
        let nonmut = cast::transmute::<TaskLocalMap,
                                       @~[Option<TaskLocalElement>]>(map);
        cast::bump_box_refcount(nonmut);
        map
    } else {
        let map = cast::transmute(map_ptr);
        let nonmut = cast::transmute::<TaskLocalMap,
                                       @~[Option<TaskLocalElement>]>(map);
        cast::bump_box_refcount(nonmut);
        map
    }
}

extern {
    #[rust_stack]
    pub fn rust_get_task() -> *rust_task;
    #[rust_stack]
    pub fn rust_get_task_local_data(task: *rust_task) -> *libc::c_void;
    #[rust_stack]
    pub fn rust_set_task_local_data(task: *rust_task, map: *libc::c_void);
    #[rust_stack]
    pub fn rust_task_local_data_atexit(task: *rust_task, cleanup_fn: *u8);
}

pub fn main() {
    unsafe {
        get_task_local_map(rust_get_task());
    }
}

brson added a commit to brson/rust that referenced this issue May 15, 2013
@brson
Copy link
Contributor

brson commented May 15, 2013

This pull request fixes the main problem: #6493

For some reason I'm still seeing the test runner report some of the spawning tests as failures even though they aren't leaking and appear to be running successfully, but I'm assuming that's a different problem.

@brson
Copy link
Contributor

brson commented May 15, 2013

Opened #6494 about the box annihilator.

bors added a commit that referenced this issue May 18, 2013
It uses the private field of TCB head to store stack limit. I tested on my Raspberry PI. A simple hello world program ran without any problem. However, for a more complex program, it segfaulted as #6231.
bors added a commit that referenced this issue May 24, 2013
@ILyoan
Copy link
Contributor

ILyoan commented May 24, 2013

This is fixed. Closing.

@ILyoan ILyoan closed this as completed May 24, 2013
flip1995 pushed a commit to flip1995/rust that referenced this issue May 20, 2021
…ednet

`while_let_on_iterator` Improvements

fixes: rust-lang#6491
fixes: rust-lang#6231
fixes: rust-lang#5844
fixes: rust-lang#1924
fixes: rust-lang#1033

The check for whether a field can be borrowed should probably be moved to utils at some point, but it would require some cleanup work and knowing what parts can actually be shared.

changelog: Suggest `&mut iter` when the iterator is used after the loop.
changelog: Suggest `&mut iter` when the iterator is a field in a struct.
changelog: Don't lint when the iterator is a field in a struct, and the struct is used in the loop.
changelog: Lint when the loop is nested in another loop, but suggest `&mut iter` unless the iterator is from a local declared inside the loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows O-android Operating system: Android
Projects
None yet
Development

No branches or pull requests

4 participants