Add `std::alloc` and `std::ptr` #1627

AlicanC · 2022-05-22T06:31:40Z

This adds two new libraries to sway-lib-std: alloc and ptr.

I needed these for my work on the Multicall script and just wanted to send them upstream. (Though it's not very usable ATM for me because of an issue.)

Adds alloc::alloc, alloc::realloc
Adds intrinsics::size_of_val, intrinsics::addr_of, intrinsics::copy, intrinsics::raw_eq
struct RawPointer for unsafely working with memory

sezna

This looks really good, will defer to the others though 😄

    boo: bool,
     uwu: u64,
     kek: bool,
     bur: u64

lol

nfurfaro

I think this looks good. I'd like to do some refactoring of other stuff to use this code!

otrho

This is great though I have a few thoughts:

Eventually I'd like a lot of this to be replaced with intrinsics, though that's true for most of the ASM blocks in the stdlib.
I'm a bit concerned about converting any old reference type to a pointer, and then assuming you can do what you like with it. Especially if you take the address of a literal which is still a reference type, but resides in the data section, it is essentially read only memory, and so if you try to copy() to it, the VM will panic. Maybe this isn't possible though, not sure.
If we were to get a little more serious we could store the size of allocations in the heap. This is what 'real' allocators do. It allows realloc() to work without needing pass the old size in. It also gives us free() although then we'd need to keep track of allocated and unallocated blocks for it to be useful. But it also would blur the distinction between pointer and buffer in this code -- buffer wouldn't necessarily be needed, and we could use Vec when it arrives for collections of values.

otrho · 2022-05-23T02:24:55Z

sway-lib-std/src/mem.sw

+        aloc size;
+        addi ptr hp i1;
+        ptr: u64


Why add 1 to $hp here?

$hp points to the free space on the heap and not to the allocated space. Heap grows from end to start so you need hp + 1 (and not hp - size) to get to the alloc'd space.

Will add a comment that explains this.

Are you sure? My impression from the spec is that the stack grows up, and so sp points to free space before you allocate. Therefore you copy sp before CFEI. Conversely, the heap grows down, so hp points to previously allocated memory before you allocate. You copy hp after you ALOC and it'll just point to your new buffer.

So I think the above should just be move ptr hp;. Adding a 1 really doesn't make sense either way. If hp was 0x1000 and you allocate 256 bytes with move r0 256; aloc r0 then hp will be 0xf00. Adding 1 to it makes it 0xf01, which is just discards one of the bytes you allocated and actually makes the buffer 1 byte smaller.

Maybe @adlerjohn can confirm or elaborate?

I wrote how I understood it in a comment:

/// Allocates zeroed memory on the heap /// /// In FuelVM, the heap begins at `VM_MAX_RAM - 1` and grows downward. /// Heap pointer `$hp` will always point to unallocated space. /// /// Initially the heap will look like this: /// ... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | /// $hp^ ^VM_MAX_RAM /// /// After allocating with `let ptr = alloc(8)`: /// ... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | /// $hp^ ^ptr ^VM_MAX_RAM /// /// After writing with `sw(ptr, u64::max())`: /// ... 00 00 00 00 00 00 00 00 FF FF FF FF FF FF FF FF | /// $hp^ ^ptr ^VM_MAX_RAM /// /// See: https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/main.md#vm-initialization /// See: https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/opcodes.md#aloc-allocate-memory pub fn alloc(size: u64) -> u64 { asm(size: size, ptr) { aloc size; // `$hp` points to unallocated space and heap grows downward so // our newly allocated space will be right after it addi ptr hp i1; ptr: u64 } }

sway-lib-std/src/mem.sw

AlicanC · 2022-05-23T20:51:22Z

@otrho thanks for the review!

My intention is to create an unsafe low level library to help w/ debugging and for building safe variants on later. std::mem is basically a collection of what I've been copying&pasting around for debugging script_data.

I've read the comments and made some research on what Rust has available and now I'm thinking:

Add std::alloc a la Rust's std::alloc:
- Add std::alloc::Global:
  - alloc = alloc_zeroed
  - dealloc = noop
- Move alloc fns from std::mem here and map them to std::alloc::Global.
Move non-alloc fns from std::mem to std::intrinsic:
- I see that Rust has std::intrinsics::copy and such so I guess that's suitable.
Add std::ptr a la Rust's std::ptr:
- Move Pointer here, but as RawPointer so we can later have Pointer<T>.
- Add size to it, and move appropriate size-based methods from Buffer here.
Remove std::mem:
- At this point there won't be anything left in std::mem but a lighter Buffer. I will still need it because Vec<u8> will pad its u8s to 8 bytes so it's not really a "vector of bytes", but it will be easy to implement Buffer where I need to use it since Pointer will be doing the boring work.

Comments appreciated!

otrho · 2022-05-24T02:27:33Z

Yep, cool, looks like a good plan! I think we'll be trying to remove the u8 is actually a u64 problem down the track but not in the short term.

otrho · 2022-05-25T05:36:01Z

sway-lib-std/src/alloc.sw

+/// Allocates zeroed memory on the heap
+/// 
+/// In FuelVM, the heap begins at `VM_MAX_RAM - 1` and grows downward.
+/// Heap pointer `$hp` will always point to unallocated space.
+/// 
+/// Initially the heap will look like this:
+/// ... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
+///                                               $hp^  ^VM_MAX_RAM
+/// 
+/// After allocating with `let ptr = alloc(8)`:
+/// ... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
+///                       $hp^  ^ptr                    ^VM_MAX_RAM
+/// 
+/// After writing with `sw(ptr, u64::max())`:
+/// ... 00 00 00 00 00 00 00 00 FF FF FF FF FF FF FF FF |
+///                       $hp^  ^ptr                    ^VM_MAX_RAM
+///
+/// See: https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/main.md#vm-initialization
+/// See: https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/opcodes.md#aloc-allocate-memory


OK, I believe the initialisation of the VM is broken, and $hp should be initialised to VM_MAX_RAM. Putting in hacks to account for the neglected last byte isn't the role of the stdlib.

Especially since if another part of the library used ALOC and didn't do the little $hp + 1 trick then there'd be conflict and potential corruption.

sway-lib-std/src/alloc.sw

AlicanC · 2022-05-25T06:31:40Z

Update:

std::mem is gone. Buffer was removed and the rest was moved to std::intrinsics and to the new std::alloc and std::ptr.

Added alloc::alloc, alloc::realloc:
- I did implement std::alloc::Global but couldn't call its methods from the free functions. Will attempt again later.
- I initially wanted to add alloc_zeroed and dealloc, but didn't. Should I?
Adds intrinsics::size_of_val, intrinsics::addr_of, intrinsics::copy, intrinsics::raw_eq.
Adds struct RawPointer for unsafely working with memory:
- Didn't add a len to it. It wasn't being used anywhere because of the unsafe nature of it.

AlicanC · 2022-05-25T06:52:49Z

To elaborate on the alloc situation:

Fully implementing Allocator would look like this:

trait Allocator {
    fn allocate(self, size: u64) -> u64;
    fn deallocate(self, ptr: u64, size: u64) -> u64;
    fn allocate_zeroed(self, size: u64) -> u64;
    fn grow(self, ptr: u64, old_size: u64, new_size: u64) -> u64;
    fn grow_zeroed(self, ptr: u64, old_size: u64, new_size: u64) -> u64;
    fn shrink(self, ptr: u64, old_size: u64, new_size: u64) -> u64;
}

But in reality we only have these:

trait Allocator {
    fn allocate_zeroed(self, size: u64) -> u64;
    fn grow_zeroed(self, ptr: u64, old_size: u64, new_size: u64) -> u64;
}

And in my latest commit we call them alloc and realloc.

I see three options:

Keep it as is: alloc and realloc
Rename them to allocate_zeroed, grow_zeroed
Implement full Allocator and start using deallocate/shrink/etc wherever makes sense even though they are noop.

otrho · 2022-05-25T07:09:11Z

At this stage we really don't need to go apeshit with the Allocator, IMO, this is a great start and will do for now. I'm happy except I still strongly feel the $hp + 1 hack needs to go.

adlerjohn · 2022-05-25T15:42:50Z

Converting to draft until FuelLabs/fuel-specs#322 is implemented and pushed through to fuel-core. Alternatively, merge as-is with the off-by-one, and fix later.

otrho · 2022-05-26T00:52:48Z

There's no reason not to make the + 1 fix now and merge. We don't have to wait for the VM fix. It'd just mean that until then the last byte in the heap will be ignored/unused.

adlerjohn · 2022-05-26T00:54:13Z

Hmmm. But the VM's memory is 0 to VM_MAX_RAM - 1 inclusive. I.e. VM_MAX_RAM isn't actually addressable. Also, the value of $hp can't be changed without changing the VM semantics.

otrho · 2022-05-26T01:01:52Z

$hp starts at 0x3ffffff. You allocate 0x10 bytes and now $hp is 0x3ffffef. Your 0x10 byte buffer is all there. No need to add 1 or anything since as far as I was aware we don't have alignment constraints.

After the VM fix $hp will start as 0x4000000, that's the only difference. What am I missing?

adlerjohn · 2022-05-26T01:07:21Z

Ah I got it, I was getting the direction mixed up. I though it was that the top byte of allocated mem would be outside the VM's memory, but it's instead that the top byte of the VM's memory wouldn't be allocatable. Right? In that case, that's fine.

AlicanC · 2022-05-26T03:37:39Z

There's no reason not to make the + 1 fix now and merge. We don't have to wait for the VM fix. It'd just mean that until then the last byte in the heap will be ignored/unused.

I need to return $hp + 1 because you can't write to $hp.

otrho · 2022-05-26T04:57:57Z

If you commit my suggestion above for alloc() and remove the comment, then it should be all fine and we can merge. There's nothing special about $hp other than it's the subject of the ALOC instruction and that it must remain larger than $sp...

AlicanC · 2022-05-26T06:40:56Z

I commited the suggestion, removed the comment and also removed +1s from the test and it's failing here:

sway/test/src/sdk-harness/test_projects/alloc/src/main.sw

Line 37 in ddd1480

sw(ptr, val);

  Compiled script "std_alloc_test".
  Bytecode size is 348 bytes.
    Finished test [unoptimized + debuginfo] target(s) in 0.20s
     Running test_projects/harness.rs (/Users/johncub/Repos/FuelLabs/sway/test/src/sdk-harness/target/debug/deps/integration_tests-b203013b97979b2e)

running 1 test
test alloc::alloc_implementation ... FAILED

failures:

---- alloc::alloc_implementation stdout ----
thread 'alloc::alloc_implementation' panicked at 'called `Result::unwrap()` on an `Err` value: ContractCallError("MemoryOverflow")', /Users/johncub/Repos/FuelLabs/sway/test/src/sdk-harness/test_helpers/src/lib.rs:31:47

This CI run should fail: https://github.com/FuelLabs/sway/runs/6605214966

otrho · 2022-05-26T07:06:34Z

Alright, FYI, @AlicanC and I hopped in a huddle and we're seeing weirdness regarding writing to the allocated buffer. I'm going to take a closer look at the VM implementation and see what's going on.

otrho · 2022-05-31T04:28:50Z

An update: I've made FuelLabs/fuel-specs#322 and FuelLabs/fuel-vm#137 because I believe the way we're using $hp and ALOC is not quite right.

But at the moment if we want to go ahead with this PR, we can use the original behaviour of expecting $hp to point to last byte in unallocated heap and using the $hp + 1 idiom for newly allocated buffers. We can switch it up if/when those issues are addressed.

AlicanC · 2022-05-31T17:01:26Z

It seems like some change broke the way I use generics. Will try to isolate and open an issue.

AlicanC · 2022-06-15T15:47:44Z

Resolved in #1979

AlicanC self-assigned this May 22, 2022

AlicanC added lib: std Standard library enhancement New feature or request labels May 22, 2022

AlicanC changed the title ~~feat: add std::mem~~ Add std::mem May 22, 2022

AlicanC requested review from adlerjohn, mohammadfawaz and nfurfaro May 22, 2022 07:07

AlicanC marked this pull request as ready for review May 22, 2022 07:07

adlerjohn requested a review from otrho May 22, 2022 12:06

sezna reviewed May 22, 2022

View reviewed changes

nfurfaro reviewed May 23, 2022

View reviewed changes

otrho suggested changes May 23, 2022

View reviewed changes

AlicanC force-pushed the jc/std-mem branch from afae1a7 to 89d09fe Compare May 25, 2022 05:29

AlicanC changed the title ~~Add std::mem~~ Add std::alloc and std::ptr May 25, 2022

otrho suggested changes May 25, 2022

View reviewed changes

AlicanC marked this pull request as draft May 25, 2022 05:48

AlicanC marked this pull request as ready for review May 25, 2022 06:31

AlicanC requested review from otrho and nfurfaro May 25, 2022 06:32

otrho mentioned this pull request May 25, 2022

$hp is initialised to incorrect value. FuelLabs/fuel-specs#322

Closed

adlerjohn marked this pull request as draft May 25, 2022 15:42

AlicanC mentioned this pull request May 27, 2022

Multiple contract calls in a single transaction FuelLabs/fuels-ts#216

Closed

AlicanC force-pushed the jc/std-mem branch from ddd1480 to 0ba9621 Compare May 31, 2022 15:29

feat: add std::alloc and std::ptr

ac264ab

AlicanC force-pushed the jc/std-mem branch from 0ba9621 to ac264ab Compare May 31, 2022 16:43

AlicanC mentioned this pull request Jun 14, 2022

Add std::alloc #1979

Merged

AlicanC closed this Jun 15, 2022

AlicanC deleted the jc/std-mem branch July 14, 2022 18:50

adlerjohn mentioned this pull request Dec 6, 2022

Possible logd memory access bug FuelLabs/fuel-vm#282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `std::alloc` and `std::ptr` #1627

Add `std::alloc` and `std::ptr` #1627

AlicanC commented May 22, 2022 •

edited

Loading

sezna left a comment

nfurfaro left a comment •

edited

Loading

otrho left a comment

otrho May 23, 2022

AlicanC May 23, 2022

AlicanC May 23, 2022

otrho May 24, 2022 •

edited

Loading

AlicanC May 25, 2022

AlicanC commented May 23, 2022

otrho commented May 24, 2022

otrho May 25, 2022

AlicanC commented May 25, 2022

AlicanC commented May 25, 2022

otrho commented May 25, 2022

adlerjohn commented May 25, 2022

otrho commented May 26, 2022 •

edited

Loading

adlerjohn commented May 26, 2022 •

edited

Loading

otrho commented May 26, 2022

adlerjohn commented May 26, 2022

AlicanC commented May 26, 2022

otrho commented May 26, 2022 •

edited

Loading

AlicanC commented May 26, 2022

otrho commented May 26, 2022

otrho commented May 31, 2022

AlicanC commented May 31, 2022

AlicanC commented Jun 15, 2022

Add std::alloc and std::ptr #1627

Add std::alloc and std::ptr #1627

Conversation

AlicanC commented May 22, 2022 • edited Loading

sezna left a comment

Choose a reason for hiding this comment

nfurfaro left a comment • edited Loading

Choose a reason for hiding this comment

otrho left a comment

Choose a reason for hiding this comment

otrho May 23, 2022

Choose a reason for hiding this comment

AlicanC May 23, 2022

Choose a reason for hiding this comment

AlicanC May 23, 2022

Choose a reason for hiding this comment

otrho May 24, 2022 • edited Loading

Choose a reason for hiding this comment

AlicanC May 25, 2022

Choose a reason for hiding this comment

AlicanC commented May 23, 2022

otrho commented May 24, 2022

otrho May 25, 2022

Choose a reason for hiding this comment

AlicanC commented May 25, 2022

AlicanC commented May 25, 2022

otrho commented May 25, 2022

adlerjohn commented May 25, 2022

otrho commented May 26, 2022 • edited Loading

adlerjohn commented May 26, 2022 • edited Loading

otrho commented May 26, 2022

adlerjohn commented May 26, 2022

AlicanC commented May 26, 2022

otrho commented May 26, 2022 • edited Loading

AlicanC commented May 26, 2022

otrho commented May 26, 2022

otrho commented May 31, 2022

AlicanC commented May 31, 2022

AlicanC commented Jun 15, 2022

Add `std::alloc` and `std::ptr` #1627

Add `std::alloc` and `std::ptr` #1627

AlicanC commented May 22, 2022 •

edited

Loading

nfurfaro left a comment •

edited

Loading

otrho May 24, 2022 •

edited

Loading

otrho commented May 26, 2022 •

edited

Loading

adlerjohn commented May 26, 2022 •

edited

Loading

otrho commented May 26, 2022 •

edited

Loading