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

update kernel and add missing functions #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mhmd-azeez
Copy link
Collaborator

Fixes #7

@@ -2,6 +2,7 @@ import { toByteArray } from "base64-js";
import { initialize } from "esbuild";
import { sha256 } from "js-sha256";

// @ts-ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why I am getting a "No base constructor has the specified number of type arguments.ts(2508)" error

Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm, that doesn't seem to fix it. To be clear, it builds fine, the only one complaining is the linter

@jrandall
Copy link

jrandall commented Jan 8, 2025

I can confirm that this works with the most recent released version of c-pdk (1.1.0). Thanks!

@jrandall
Copy link

jrandall commented Jan 8, 2025

I've just tried to actually exercise some of the logging and am running into some issues with that:

ERROR:  TypeError: Cannot read properties of null (reading 'logLevelToNumber')

dist/index.sql Outdated Show resolved Hide resolved
dist/index.sql Outdated Show resolved Hide resolved
dist/index.sql Outdated Show resolved Hide resolved
dist/index.sql Outdated Show resolved Hide resolved
dist/index.sql Outdated Show resolved Hide resolved
Copy link

@jrandall jrandall left a comment

Choose a reason for hiding this comment

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

I am seeing some issues with this version using the c-pdk higher-level input loading function extism_load_input().

As far as I can tell, this is because extism_input_length() != extism_length(extism_input_offset()) and it is relying on this being the case.

These are defined in c-pdk to use wasm imports:

  • extism_input_length -> input_length
  • extism_length -> length_unsafe
  • extism_input_offset -> input_offset

In a particular invocation of my plugin with the updated pg_extism on this branch, I am getting:

  • extism_input_length(): 13
  • extism_input_offset(): 0
  • extism_length(extism_input_offset()): 0

@jrandall
Copy link

jrandall commented Jan 8, 2025

Ah-ha! I think this may be because of a special case in the extism wasm runtime length_unsafe function, which appears to always returns a length of 0 for an offset of 0: https://github.com/extism/extism/blob/98800fe8a0f1154a58530b9eb04b269f25a7ff59/kernel/src/lib.rs#L397-L399

So I guess an offset of 0 is not supposed to be a valid value?

@jrandall
Copy link

jrandall commented Jan 8, 2025

I've just tried to actually exercise some of the logging and am running into some issues with that:

ERROR:  TypeError: Cannot read properties of null (reading 'logLevelToNumber')

I believe my suggested fixes (e.g. #8 (review)) will address this issue.

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Jan 8, 2025

I am seeing some issues with this version using the c-pdk higher-level input loading function extism_load_input().

As far as I can tell, this is because extism_input_length() != extism_length(extism_input_offset()) and it is relying on this being the case.

These are defined in c-pdk to use wasm imports:

  • extism_input_length -> input_length
  • extism_length -> length_unsafe
  • extism_input_offset -> input_offset

In a particular invocation of my plugin with the updated pg_extism on this branch, I am getting:

  • extism_input_length(): 13
  • extism_input_offset(): 0
  • extism_length(extism_input_offset()): 0

Ah-ha! I think this may be because of a special case in the extism wasm runtime length_unsafe function, which appears to always returns a length of 0 for an offset of 0: https://github.com/extism/extism/blob/98800fe8a0f1154a58530b9eb04b269f25a7ff59/kernel/src/lib.rs#L397-L399

So I guess an offset of 0 is not supposed to be a valid value?

Interesting, we probably should fix the c-pdk to remove that check, @zshipko what do you think?

@jrandall
Copy link

jrandall commented Jan 8, 2025

I am seeing some issues with this version using the c-pdk higher-level input loading function extism_load_input().
As far as I can tell, this is because extism_input_length() != extism_length(extism_input_offset()) and it is relying on this being the case.
These are defined in c-pdk to use wasm imports:

  • extism_input_length -> input_length
  • extism_length -> length_unsafe
  • extism_input_offset -> input_offset

In a particular invocation of my plugin with the updated pg_extism on this branch, I am getting:

  • extism_input_length(): 13
  • extism_input_offset(): 0
  • extism_length(extism_input_offset()): 0

Ah-ha! I think this may be because of a special case in the extism wasm runtime length_unsafe function, which appears to always returns a length of 0 for an offset of 0: https://github.com/extism/extism/blob/98800fe8a0f1154a58530b9eb04b269f25a7ff59/kernel/src/lib.rs#L397-L399
So I guess an offset of 0 is not supposed to be a valid value?

Interesting, we probably should fix the c-pdk to remove that check, @zshipko what do you think?

To be clear, the check is being done in the core extism runtime, not in c-pdk. It looks like the idea is that MemoryRoot is initialized with input_offset set to 0, and it seems reasonable to report an input_length of 0 if the input is never set.

It seems like the issue here may be that pg_extism never calls input_set to set the input?

@mhmd-azeez
Copy link
Collaborator Author

@jrandall oh yeah! pg_extism is not yet using the newer input functions and handles it outside the kernel. Will fix that!

@mhmd-azeez
Copy link
Collaborator Author

@jrandall thank you very much for pointing out the issue, it should be good now!

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

Successfully merging this pull request may close these issues.

Plugins built with newer PDKs don't work with pg_extism
3 participants