[question] Memory management with C bindings

[logantm2]

In the C bindings, the Output structs contain raw pointers to data arrays. How can we delete these arrays once we are done with them? I suspect this is causing a memory leak in some of our bigger problems.

[drobnyjt]

I'm looking into this now. Assuming I understand what's happening under the hood correctly, which might be a big assumption, C should have full ownership over the struct of pointers in Rust's borrow-checker parlance, but I don't know what manually freeing that memory would look like. I can play around with an example and get back to you with what I find out.

[drobnyjt]

It looks like the correct way to do it is to provide a function in the library that disposes of memory using Rust's manual memory management. If I just use free on the raw pointers in C, valgrind says the memory leak does disappear, but it also complains that the free() calls are invalid.

[drobnyjt]

@logantm2 I think I figured out what needs to happen. So, I was under the assumption that you would be able to free that memory from C since I transferred ownership, but it looks like that while it may work sometimes due to interoperability between C's allocator and Rust's standard allocator, it's not guaranteed to work and is unspecified behavior.

So, I'm working on adding deallocation functions to call from C that appropriately dispose of structs that contain raw pointers originally created in Rust. It will take a little time to get everything working, but for future reference they will look like this:

#[no_mangle]
pub extern "C" fn drop_output_tagged_bca(output: OutputTaggedBCA) {
    let length = output.len;

    let particles_layout = Layout::from_size_align(length, align_of::<[f64; 9]>()).unwrap();
    let weights_layout = Layout::from_size_align(length, align_of::<f64>()).unwrap();
    let tags_layout = Layout::from_size_align(length, align_of::<i32>()).unwrap();
    let incident_layout = Layout::from_size_align(length, align_of::<bool>()).unwrap();

    unsafe {
        dealloc(output.particles as *mut u8, particles_layout);
        dealloc(output.weights as *mut u8, weights_layout);
        dealloc(output.tags as *mut u8, tags_layout);
        dealloc(output.incident as *mut u8, incident_layout);
    };
}

And will be called like this:

output = compound_tagged_bca_list_c(input);
... do stuff with output ...
drop_output_tagged_bca(output);

This makes valgrind happy:

valgrind --leak-check=full ./a.out
==7151== Memcheck, a memory error detector
==7151== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==7151== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==7151== Command: ./a.out
==7151==
==7151== error calling PR_SET_PTRACER, vgdb might block
Particle 1 Z: 1
Particle 1 E [eV]: 0.772666
Particle 2 Z: 1
Particle 2 E [eV]: 846.228
Before rotation: 1, 0, 0
After rotation: 0.707107, -0.707106, 0
After rotation back: 1, -1.33227e-15, 0
==7151==
==7151== HEAP SUMMARY:
==7151==     in use at exit: 0 bytes in 0 blocks
==7151==   total heap usage: 4,135 allocs, 4,135 frees, 280,124 bytes allocated
==7151==
==7151== All heap blocks were freed -- no leaks are possible
==7151==
==7151== For lists of detected and suppressed errors, rerun with: -s
==7151== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[drobnyjt]

A neater way to do this would be to just implement Drop on the aforementioned structs and then I could just write a generic drop_rust_object function call that just takes ownership and automatically calls the implementation of Drop, but first things first.

[logantm2]

Ah, this explains why I was getting intermittent errors when trying to use free and delete[] on those pointers. This is perfect, should solve our issues!

[drobnyjt]

@logantm2 Everything seems to work, so try downloading the dev branch and adding drop_output_bca(output) or drop_output_tagged_bca(output) where relevant after use and check that everything is a-okay.

[logantm2]

Thanks again for looking into this. I'm just now getting around to putting this into hpic2. It looks like it's segfaulting on the drop_output_bca call in one of our tests. Note that I'm not using tagged output still. It looks to me like it's segfaulting when len is zero... is it possible that this is an edge case that could break the drop_output_bca call? If not, I can try to make a small reproducer.

[logantm2]

I'm also thinking about how to implement this in such a way that we use it where possible, but don't break installations when people have old versions of RustBCA and new versions of hPIC2. Is it feasible to add a macro or something similar in RustBCA.h to help us detect when the drop_output_bca functions are available? If not, it's not a huge deal, there are ways to detect it purely on our end, but they are a little hacky.

[drobnyjt]

Thanks again for looking into this. I'm just now getting around to putting this into hpic2. It looks like it's segfaulting on the drop_output_bca call in one of our tests. Note that I'm not using tagged output still. It looks to me like it's segfaulting when len is zero... is it possible that this is an edge case that could break the drop_output_bca call? If not, I can try to make a small reproducer.

Yeah, that's very possible - this is my first time interacting with Rust's deallocator, so I wouldn't be surprised if there's gotchas (especially since all the magic happens in an unsafe block). It would be trivial to add a check that the length is non-zero in the function itself; I can check with my little example as well.

I'm also thinking about how to implement this in such a way that we use it where possible, but don't break installations when people have old versions of RustBCA and new versions of hPIC2. Is it feasible to add a macro or something similar in RustBCA.h to help us detect when the drop_output_bca functions are available? If not, it's not a huge deal, there are ways to detect it purely on our end, but they are a little hacky.

I don't know of any way to do this; it looks like the way other people do it is by specifying version numbers in the header file and then using the preprocessor to selectively compile stuff. If I added something to the header file though it wouldn't help, since a user with an old version would have to download a new version to get the header file... If you can figure anything out for this issue, I'd be happy to try to implement it on my side. I'll look into it as well, but I'm not even sure what to search for, haha.

[drobnyjt]

I was able to reproduce the segfault; adding a check that length is nonzero seems to have fixed it. That change is on the dev branch now.

[logantm2]

It actually wasn't as hard as I expected to check if the function exists on our end, so I think we're good to go!