Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Host Functions compatible with PHP 7 #22

Closed
mhmd-azeez opened this issue Jul 1, 2024 · 3 comments
Closed

Make Host Functions compatible with PHP 7 #22

mhmd-azeez opened this issue Jul 1, 2024 · 3 comments
Assignees

Comments

@mhmd-azeez
Copy link
Contributor

When running the tests with PHPUnit 9 and PHP 7.4, this is one of our community members got:

There were 3 errors:

1) PluginTest::testHostFunctions
Object of class FFI\CData could not be converted to int

/app/lib/vendor/extism/extism/src/LibExtism.php:167
/app/lib/vendor/extism/extism/src/LibExtism.php:140
/app/lib/vendor/extism/extism/src/HostFunction.php:110
/app/lib/vendor/extism/extism/tests/PluginTest.php:84
phpvfscomposer:///app/lib/vendor/extism/extism/vendor/phpunit/phpunit/phpunit:106

2) PluginTest::testHostFunctionManual
Object of class FFI\CData could not be converted to int

/app/lib/vendor/extism/extism/src/LibExtism.php:167
/app/lib/vendor/extism/extism/src/LibExtism.php:140
/app/lib/vendor/extism/extism/src/HostFunction.php:110
/app/lib/vendor/extism/extism/tests/PluginTest.php:107
phpvfscomposer:///app/lib/vendor/extism/extism/vendor/phpunit/phpunit/phpunit:106

3) PluginTest::testHostFunctionNamespace
Object of class FFI\CData could not be converted to int

/app/lib/vendor/extism/extism/src/LibExtism.php:167
/app/lib/vendor/extism/extism/src/LibExtism.php:140
/app/lib/vendor/extism/extism/src/HostFunction.php:110
/app/lib/vendor/extism/extism/tests/PluginTest.php:129
phpvfscomposer:///app/lib/vendor/extism/extism/vendor/phpunit/phpunit/phpunit:106
@mhmd-azeez mhmd-azeez self-assigned this Jul 1, 2024
@mhmd-azeez mhmd-azeez changed the title Make HostFunctions compatible with PHP 7 Make Host Functions compatible with PHP 7 Jul 1, 2024
@oatmael
Copy link
Contributor

oatmael commented Jul 3, 2024

So I've poked at this a little bit, from what I can tell the following function is the culprit here:
https://github.com/extism/php-sdk/blob/main/src/LibExtism.php#L157

function toCArray(array $array, string $type): ?FFI\CData
{
    if (count($array) == 0) {
        return $this->ffi->new($type . "*");
    }

    $cArray = $this->ffi->new($type . "[" . count($array) . "]");
    for ($i = 0; $i < count($array); $i++) {            
        $cArray[$i] = $array[$i];
    }

    return $cArray;
}

But more specifically line 165, it's not exactly well documented from what I can tell, but PHP 7.4's FFI doesn't allow for assigning CData to structs or fields (see https://www.php.net/manual/en/class.ffi-cdata.php#Changelog).

Where this applies to host functions is when converting the handle to CData when mapping the inputs / outputs here: https://github.com/extism/php-sdk/blob/main/src/LibExtism.php#L136

function extism_function_new(string $name, array $inputTypes, array $outputTypes, callable $callback, $userData, $freeUserData) : \FFI\CData
{
    $inputs = $this->toCArray($inputTypes, "ExtismValType");
    $outputs = $this->toCArray($outputTypes, "ExtismValType");

    $handle = $this->ffi->extism_function_new($name, $inputs, count($inputTypes), $outputs, count($outputTypes), $callback, $userData, $freeUserData);

    return $handle;
}

Since the problematic line in toCArray is line 165, host functions actually do work if you define them as having no inputs or outputs:

$a = 0;
$increment_a = new HostFunction('increment_a', [], [], 
  function () use (&$a) {
    $a++;
  }
);

...
echo $a;

I'll do some more digging into if there's an effective workaround for the CData assignment issue, but hopefully this gives better visibility on where the issue actually lies 😄

@oatmael
Copy link
Contributor

oatmael commented Jul 3, 2024

I believe I have a fix.
There seems to be an issue with specifically the ExtismValType enum casting that happens in the HostFunction.php.

Changing the following block from this:

$inputs = [];

for ($i = 0; $i < count($inputTypes); $i++) {
    $inputs[$i] = $this->lib->ffi->cast("ExtismValType", $inputTypes[$i]);
}

$outputs = [];

for ($i = 0; $i < count($outputTypes); $i++) {
    $outputs[$i] = $this->lib->ffi->cast("ExtismValType", $outputTypes[$i]);
}

To this:

public const VAL_TYPE_MAP = [
    0 => 'I32',
    1 => 'I64',
    2 => 'F32',
    3 => 'F64',
    4 => 'V128',
    5 => 'FUNC_REF',
    6 => 'EXTERN_REF',
];
$inputs = [];

for ($i = 0; $i < count($inputTypes); $i++) {
    $enum = ExtismValType::VAL_TYPE_MAP[$inputTypes[$i]];
    $inputs[$i] = $this->lib->ffi->$enum;
}

$outputs = [];

for ($i = 0; $i < count($outputTypes); $i++) {
    $enum = ExtismValType::VAL_TYPE_MAP[$outputTypes[$i]];
    $outputs[$i] = $this->lib->ffi->$enum;
}

Allows the host functions to work as expected :)

mhmd-azeez pushed a commit that referenced this issue Jul 3, 2024
Fixes issue #22 that causes host
functions with arguments to not work on PHP 7.4.
See issue for context.
@mhmd-azeez
Copy link
Contributor Author

This is fixed in #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants