Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Implement #[bytecode_instruction] #187

Closed
brson opened this issue Jun 7, 2023 · 6 comments
Closed

Implement #[bytecode_instruction] #187

brson opened this issue Jun 7, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@brson
Copy link
Collaborator

brson commented Jun 7, 2023

#[bytecode_instruction] is a built-in attribute for native functions. It is applied only to the natives in 0x1::vector as an optimization. Calls to these functions do not emit bytecode for a native call but instead emit dedicated bytecode for these specific native functions.

These don't seem to be represented consistently in stackless bytecode:

While calls to these instructions have dedicated bytecode in normal move, in stackless bytecode they do not - they are just regular call instructions; but also the move model does not tell us these functions are called when we are making declarations - they do not show up in get_called_functions etc.

So it looks like our step of declaring called functions needs to have a special case of looking through the bytecode for calls to these functions in order to declare them.

@brson brson added the enhancement New feature or request label Jun 7, 2023
@brson brson self-assigned this Jun 7, 2023
@brson
Copy link
Collaborator Author

brson commented Jun 7, 2023

move_compiler::naming::fake_natives is the code that deals with these special functions.

@nvjle
Copy link

nvjle commented Jun 7, 2023

move_compiler::naming::fake_natives is the code that deals with these special functions.

Yikes-- what an ugly wart. For the SBC path, it is pointless to lower them to specialized bytecode operations in the first place. What makes sense to me is to simply have the front-end ignore the directive for cfg Solana. If the SBC wants to turn them back into the calls they originally were, it should do so consistently. I'd actually call this a defect-- worse than a mere wart. It's all fixable in their code, but we're trying to avoid touching and/or fixing their problems.

@nvjle
Copy link

nvjle commented Jun 7, 2023

move_compiler::naming::fake_natives is the code that deals with these special functions.

Yikes-- what an ugly wart. For the SBC path, it is pointless to lower them to specialized bytecode operations in the first place. What makes sense to me is to simply have the front-end ignore the directive for cfg Solana. If the SBC wants to turn them back into the calls they originally were, it should do so consistently. I'd actually call this a defect-- worse than a mere wart. It's all fixable in their code, but we're trying to avoid touching and/or fixing their problems.

Following myself up-- it appears that the EVM translator also ignores them (move_compiler::to_bytecode::translate::module_call).

fn module_call(
    context: &mut Context,
    loc: Loc,
    code: &mut IR::BytecodeBlock,
    mident: ModuleIdent,
    fname: FunctionName,
    tys: Vec<H::BaseType>,
) {
    use IR::Bytecode_ as B;
    match fake_natives::resolve_builtin(&mident, &fname) {
        // TODO full evm support for vector bytecode instructions
        Some(mk_bytecode) if !cfg!(feature = "evm-backend") => {
            code.push(sp(loc, mk_bytecode(base_types(context, tys))))
        }
        _ => {
            let (m, n) = context.qualified_function_name(&mident, fname);
            code.push(sp(loc, B::Call(m, n, base_types(context, tys))))
        }
    }
}

@brson
Copy link
Collaborator Author

brson commented Jun 7, 2023

It would be easier to just turn this feature off in the compiler, though we would have to modify the compiler to do it, which I am reluctant to do - it would be more desirable to operate on standard move bytecode and not require any compiler modifications.

A flag to disable this feature seems like a reasonable thing to push upstream since standard move can work without these special vector bytecodes; though I am not confident upstream is engaged enough to consider such a patch.

The hacks needed to work around this in the llvm code generator are not too onerous. And maybe there are still apis I'm not seeing in the move model to figure out which of these fake natives have been used and declare them.

@nvjle
Copy link

nvjle commented Jun 7, 2023

It would be easier to just turn this feature off in the compiler, though we would have to modify the compiler to do it, which I am reluctant to do - it would be more desirable to operate on standard move bytecode and not require any compiler modifications.

A flag to disable this feature seems like a reasonable thing to push upstream since standard move can work without these special vector bytecodes; though I am not confident upstream is engaged enough to consider such a patch.

The hacks needed to work around this in the llvm code generator are not too onerous. And maybe there are still apis I'm not seeing in the move model to figure out which of these fake natives have been used and declare them.

Did you see my other comment above? Turning it off is simple and is a one-line change. The EVM compiler does the same thing-- guarding the builtin conversion with !cfg!(feature = "evm-backend"). See the code block in the comment above for resolve_builtin.

@nvjle
Copy link

nvjle commented Jun 16, 2023

Closing issue-- this has been fixed by #199.

@nvjle nvjle closed this as completed Jun 16, 2023
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

No branches or pull requests

2 participants