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.

Sign up for GitHub

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

Jump to bottom

refactor(wasm-instrument, wasm-gen): Make syscalls errors config and syscalls results processing more consistent #3554

Merged
merged 23 commits into from
Dec 12, 2023
Merged

refactor(wasm-instrument, wasm-gen): Make syscalls errors config and syscalls results processing more consistent #3554

merged 23 commits into from
Dec 12, 2023

Conversation

techraed
Copy link
Member

@techraed techraed commented Nov 30, 2023 •
edited
Loading

  • Make it impossible to set infallible syscalls to syscalls errors config
  • Introduce on type level those guarantees that are required for build_result_processing* functions.

@techraed techraed added A1-inprogress Issue is in progress or PR draft is not ready to be reviewed D5-tooling Helper tools and utilities labels Nov 30, 2023
@techraed techraed self-assigned this Nov 30, 2023
@techraed techraed added A0-pleasereview PR is ready to be reviewed by the team and removed A1-inprogress Issue is in progress or PR draft is not ready to be reviewed labels Dec 4, 2023
mertwole
mertwole requested changes Dec 4, 2023
View reviewed changes
utils/wasm-instrument/src/syscalls.rs Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
utils/wasm-gen/src/generator/syscalls/invocator.rs Show resolved Hide resolved
utils/wasm-gen/src/generator/syscalls/invocator.rs Outdated Show resolved Hide resolved
utils/wasm-gen/src/config/syscalls/process_errors.rs Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
@techraed techraed added A2-mergeoncegreen PR is ready to merge after CI passes and removed A0-pleasereview PR is ready to be reviewed by the team labels Dec 5, 2023
breathx
breathx approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Avoiding diving into details, seems ok for me
The few design questions:

  • don't we really can avoid duplicating variants like Ptr::Hash, Ptr::MutHash etc
  • are we okay with such huge enum nesting of arguments while building syscall and so on?

@techraed techraed merged commit 5b021ea into master Dec 12, 2023
11 checks passed
@techraed techraed deleted the st-refactor-error-config branch December 12, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StackOverflowExcept1on StackOverflowExcept1on StackOverflowExcept1on approved these changes

@breathx breathx breathx approved these changes

@mertwole mertwole mertwole approved these changes

@gshep gshep Awaiting requested review from gshep

@shamilsan shamilsan Awaiting requested review from shamilsan

Assignees

@techraed techraed

Labels
A2-mergeoncegreen PR is ready to merge after CI passes D5-tooling Helper tools and utilities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@techraed @mertwole @breathx @StackOverflowExcept1on