BlockstreamResearch / simplicity

Simplicity is a blockchain programming language designed as an alternative to Bitcoin script.
MIT License
305 stars 45 forks source link

Simplicity alloc #222

Closed uncomputable closed 8 months ago

uncomputable commented 9 months ago

Simplicity (de)allocation macros in the style of simplicity_assert.h. These macros can be overwritten to use Rust functions that are injected into C via FFI.

Environment allocation functions that return the number of allocated bytes.

roconnor-blockstream commented 9 months ago

In C, malloc always aligns allocation with respect to alignof(max_align_t). Should we just use that everywhere?

apoelstra commented 9 months ago

I think we should. It would de-noise the C code and remove risk of copy/paste bugs that the compiler won't help us with.

We would need to export the size/align of max_align_t though, just like we do with a bunch of other C types, so that the Rust side can sanity-check its own values.

uncomputable commented 9 months ago

Thanks for pointing this out. I removed the alignment parameter from the function signatures.

apoelstra commented 9 months ago

LGTM. For what it's worth in rust-secp we just allocate extra memory and store the size in there so that we can extract it to deallocate. This is what C malloc (probably) does internally and makes life simpler for the programmer.

But I could go either way on this. It depends how much API complexity @roconnor-blockstream is willing to suffer in the C code.

roconnor-blockstream commented 9 months ago

just allocate extra memory and store the size in there so that we can extract it to deallocate.

I think this would be better.

roconnor-blockstream commented 9 months ago

Be wary of alignment issues though.

roconnor-blockstream commented 9 months ago

This seems fine to me. Can you provide a cross reference in the PR comments here to where/how you are overriding this in Rust, or wherever you are doing that?

uncomputable commented 9 months ago

Allocation is done in Rust in this PR: https://github.com/BlockstreamResearch/rust-simplicity/pull/205

uncomputable commented 8 months ago

Rebased and compressed the macro definitions.

roconnor-blockstream commented 8 months ago

@apoelstra I'm not sure if we should go ahead and replace all instances of malloc and free, or just these ones. Do you have any thoughts? Having a mix of simplicity_malloc and malloc seems potentially dangerous ... maybe?

uncomputable commented 8 months ago

The original signature of simplicity_alloc was different from malloc, so only some files were changed. Now that the signatures are the same, it should be easy to use simplicity_alloc everywhere.

apoelstra commented 8 months ago

@apoelstra I'm not sure if we should go ahead and replace all instances of malloc and free, or just these ones. Do you have any thoughts? Having a mix of simplicity_malloc and malloc seems potentially dangerous ... maybe?

I think we should replace them all.

I think it's safe to mix in our current setup, where if you try to call the norrmal malloc in WASM things just won't compile, and otherwise they're the same. But this feels a bit fragile.

uncomputable commented 8 months ago

I replaced all occurrences of malloc and of free.

I kept calloc in test.c because it is independent. For the record, we can define simplicity_malloc as NULL and make check succeeds.

roconnor-blockstream commented 8 months ago

Okay so I didn't think about the fact that we use calloc.

I can see two choices, either we add calloc to the simplicity_ allocation interface, or we carefully divide our uses of memory allocation into internal and external uses?

But if our goal is to make the entire simplicity execution library (excluding test.c) as optionally independent from stdlib.h then maybe our only choice to to add calloc to the interface.

roconnor-blockstream commented 8 months ago

LGTM. Can you squash the commits so we don't have any commits with unmatched simplicity_free *alloc floating around?

roconnor-blockstream commented 8 months ago

@apoelstra when you get a chance can you rerun CI?