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

Add global export apis #3452

Merged
merged 2 commits into from
May 22, 2024

Conversation

bnason-nf
Copy link
Contributor

I'm very new to this code, so I'm not confident at all that this is the best way (or even a good way) to expose export globals, so please let me know if there is a better strategy or if I've just done it wrong. As a minor detail, I didn't see a good way in the AOT global_instantiate() to fill in the ref_type.

@lum1n0us
Copy link
Collaborator

lum1n0us commented May 21, 2024

the intention is for it to be writable from outside of WASM code.

Is it about to operate wasm export global in a host function? I may misunderstand the purpose. You used to mention it's about to go through all import/export and check and make sure all matched.

@wenyongh
Copy link
Contributor

@bnason-nf the method of this PR in the AOT mode will allocate memory for the global instances, which increases footprint and the binary size. How about we define the wasm_global_inst_t structure in wasm_export.h, and let developer provide the structure pointer in the API, so that runtime just fills the structure and doesn't need to allocate memory? Somewhat like wasm_import_t/wasm_export_t and wasm_runtime_get_import_type/wasm_runtime_get_export_type:

typedef struct wasm_global_inst_t {
    wasm_valkind_t kind;;
    bool is_mutable;
    void *global_data;
} wasm_global_inst_t;
    
WASM_RUNTIME_API_EXTERN unt32_t
wasm_runtime_get_global_inst_count(const wasm_module_inst_t module_inst);

WASM_RUNTIME_API_EXTERN void
wasm_runtime_get_global_inst(const wasm_module_inst_t module_inst, uint32_t global_index,
                             wasm_global_inst_t *global_inst);

I am not sure whether exposing global_data pointer in global instance is good, if yes, developer should ensure the security, e.g. don't write invalid data to it or overwrite it.

@bnason-nf bnason-nf force-pushed the add-global-export-apis branch 2 times, most recently from 9248227 to 32f2d14 Compare May 21, 2024 20:30
@bnason-nf
Copy link
Contributor Author

@wenyongh I implemented something very close to your idea, except that I kept the lookup by name instead of by index, since to me it feels more natural to use it that way from the calling application code.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM except minor comment.

core/iwasm/common/wasm_runtime_common.c Outdated Show resolved Hide resolved
core/iwasm/include/wasm_export.h Show resolved Hide resolved
core/iwasm/include/wasm_export.h Show resolved Hide resolved
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit c5ab862 into bytecodealliance:main May 22, 2024
377 checks passed
@bnason-nf bnason-nf deleted the add-global-export-apis branch June 3, 2024 22:45
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.

3 participants