Skip to content
This repository was archived by the owner on Mar 29, 2022. It is now read-only.

Adding an `is_reentrant() function #34

Draft
wants to merge 45 commits into
base: master
Choose a base branch
from
Draft

Conversation

nfurfaro
Copy link
Contributor

@nfurfaro nfurfaro commented Feb 3, 2022

This first draft is aimed at addressing issue FuelLabs/sway#1069.
The control flow could be improved with match statements for enums when they land.

As always, the asm blocks will need careful review.
In particular, the distinction between when I'm returning a value from an asm block vs returning an offset to a value.

ref: https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/main.md#semantics

@nfurfaro nfurfaro added the enhancement New feature or request label Feb 3, 2022
@nfurfaro nfurfaro self-assigned this Feb 3, 2022
@nfurfaro nfurfaro marked this pull request as draft February 3, 2022 17:43
@nfurfaro nfurfaro changed the title Furnic/reentrancy guard Adding an `is_reentrant() function Feb 3, 2022
@nfurfaro nfurfaro requested review from sezna and otrho February 3, 2022 22:30
} else {
internal = !caller_is_external();
call_frame_pointer = saved_registers_pointer + 48;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this is better. 👍

The call_frame_pointer value might not be quite right on line 31, I think you need to dereference it. i.e., at line 31 it's still a pointer to a pointer.

I think get_current_frame_pointer() and get_previous_frame_pointer(current_fp) functions would be good here, to keep it fairly readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@otrho I have a few questions here.

  1. RE: dereferencing. I'm sure it's simple (and obvious after the fact) but how exactly do I go about dereferencing the value on line 31?
    I understand the concept(eg. by using the * operator in Rust ) but it's not clear to me how to do this in an asm block. On that note, as we have no docs on the usage of assemble, what would be some recommended reading on something similar? (ie: x86 assembly language? or something else...)

  2. RE: aget_current_frame_pointer() function
    I have this assignment already on line 16:

let mut call_frame_pointer = asm() {
        fp: u64
    };

My understanding is that $fp would always point to the call frame on top of the stack, so there should be no need to access this location more than once (ie: no need for a get_current_frame_pointer() function. I might have misunderstood though. (also, we do have a contract_id() function in the stdlib which returns $fp already...)

  1. RE: a get_previous_frame_pointer(current_fp) function
    I do have 2 functions, which can be used together to basically achieve this.
    fn get_saved_regs_pointer(frame_ptr: u64) & fn get_previous_contract_id(saved_regs_ptr: u64)
    should get us to the start of the previous call frame. I'm trying to use them to recover the ContractId stored at that location, so again, getting crystal clear on the differences between returning values and returning pointers(and when//how to do each!) is key for me to put this together correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Dereferencing would need to be done with an lw, which can now be done with ASM blocks... not the most elegant solution, but until we actually have reference types I think this is how we do it.
  2. Yeah, you'd get the current fp once, and subsequent fps are fetched from the saved registers array, until it's zero which indicates an external call and the bottom of the call stack. I think that's right. contract_id() actually reads the first field at fp out though, because the compiler adds that code -- I mentioned that recently somewhere on Slack. Rather than returning fp itself. But the code is deceptive... if the type of contract_id() was u64 with the same ASM block, it would read and return fp.
  3. get_previous_contract_id() would be unhelpful again, as I assume it's similar to contract_id() and has a return type of b256.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@otrho I've (hopefully) addressed your comments.
I opened a couple of new issues regarding the creation of some modules to help with accessing registers and opcodes via wrapper functions as well.
I'll work to add tests to this as well once the test harness is merged.

@nfurfaro nfurfaro marked this pull request as ready for review February 14, 2022 20:19
@nfurfaro nfurfaro marked this pull request as draft February 14, 2022 20:21
~ContractId::from(asm(res, ptr: previous_frame_ptr) {
mcpi res ptr i32;
res: b256
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is still confusing, I'm having to stare hard at this stuff too, to work it out, but that mcpi is not going to work, as it expects the first arg to point to an allocated buffer.

So to get the previous contract ID you want to return the previous fp as a b256 and let the compiler do the copying, if required.

Maybe: asm(res, ptr: previous_frame_ptr) { move res ptr; res: b256 }
But even that move probably isn't required. Just 'casting' the previous fp to b256 is what we want to do.

Copy link
Contributor Author

@nfurfaro nfurfaro Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try the simpler version of the suggested change first.
I'm working now to add tests for this !

Copy link
Contributor

@otrho otrho Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious now if

asm(ptr: previous_frame_ptr) {
    ptr: b256
}

would just work. It's pretty much casting the fp value to a b256 pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will...after all, that's what we do in std::context::contract_id.
I'm hopeful that I'll have working tests for this today.

}

// get a pointer to the current call frame
fn get_current_frame_pointer() -> u64 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move helper functions to call_frames module.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants