Skip to content

Conversation

@jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Nov 22, 2025

fixes: #1047

Adds tests for exception handling but doesn't specifically test the logic for page faulting logic as described in (#698)

This pull request updates the exception context saving and restoring logic to properly ensure correct 16-byte stack alignment. It adds a test that demonstrates the failure and then does some additional validation.

simongdavies
simongdavies previously approved these changes Nov 26, 2025
Copy link
Contributor

@simongdavies simongdavies left a comment

Choose a reason for hiding this comment

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

LGTM

@danbugs danbugs requested a review from Copilot November 26, 2025 20:29
Copilot finished reviewing on behalf of danbugs November 26, 2025 20:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a critical stack alignment issue in the exception handler infrastructure (issue #1047). The changes ensure proper 16-byte stack alignment when saving and restoring CPU context during exception handling, which is required by SSE instructions. The PR adds comprehensive testing for exception handler installation, validation, and invocation, along with improved API documentation.

Key Changes

  • Stack alignment fix: Modified the Context struct to include padding and updated the context_save!/context_restore! macros to maintain 16-byte alignment by adjusting when padding and segment registers are pushed/popped
  • Enhanced API: Added ExceptionHandler type alias and new install_handler/uninstall_handler functions with better documentation and error handling
  • Comprehensive tests: Added test functions in the guest code to install/uninstall handlers, trigger exceptions, and validate register context manipulation, plus integration tests to verify the complete workflow

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/hyperlight_guest_bin/src/exceptions/interrupt_entry.rs Updated context save/restore macros to fix 16-byte alignment by reordering segment register handling and adding padding; added detailed stack layout documentation
src/hyperlight_guest_bin/src/exceptions/handler.rs Enhanced API with new ExceptionHandler type, InstallError enum, improved install_handler/uninstall_handler functions with comprehensive documentation; updated Context struct to include padding field and all 4 segment registers
src/tests/rust_guests/simpleguest/src/main.rs Added exception handler test functions including handler installation/uninstallation, INT3 triggering, and register context validation; however, contains bugs in register index usage
src/hyperlight_host/tests/integration_test.rs Added two integration tests for exception handler validation and invalid vector handling; contains bugs in vector number consistency and assertion messages
src/hyperlight_host/tests/common/mod.rs Added conditional GDB debugging support for rust guest tests
Justfile Parameterized build-and-move functions to accept target argument for more flexible builds
src/tests/rust_guests/callbackguest/Cargo.lock Removed obsolete lock file for non-existent guest

Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'm not very familiar with this code, will defer to others

@jsturtevant jsturtevant force-pushed the exception-alignment branch 2 times, most recently from e1fe588 to 1caa0c8 Compare November 26, 2025 22:19
ludfjig
ludfjig previously approved these changes Dec 2, 2025
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

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

LGTM. maybe @syntactically can have look too

@syntactically
Copy link
Member

In re: the main change with the stack alignment, you are absolutely correct: the old code was accidentally setting it up so that the stack was aligned at the beginning of the handler function, but the ABI requirement is actually that the stack be aligned before the call instruction that invokes the function, which is a different requirement on amd64 since call implicitly pushes a return address onto the stack. Let's merge that part as soon as possible.

The 16 byte alignment requirement for the stack is knowledge that I think is pretty widespread at the moment, but maybe let's add a citation to the ABI spec? I think the authoritative reference here is the x86-64 ELF System V psABI which can be found at https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf, Section 3.2.2.

I am not a huge fan of the new handler registration API---I don't think that what it abstracts is important, and I think that the important stuff that we do need to abstract in the near future is not covered here. I agree that the raw handler array export should go away, but I would like to see it replaced with some more generic and architecture-independent options for e.g. "let me respond to unmapped page accesses in this region", "let me respond to unpermitted accesses in this region", "let me know about any undefined/misaligned/FP faulting instructions", etc. The main reason that this is not there yet IIRC is that the virtual memory handlers probably need to work with the (types from the) upcoming architecture-independent virtual memory mapping abstractions. It seems to me like this changeset just means that fixing the exception handler exports is going to require two API breaks instead of one, with the intermediate API requiring downstream changes in hl-wasm an extra time and unlikely to unblock anyone else. Consequently, I am weakly against on that portion of the change.

syntactically
syntactically previously approved these changes Dec 2, 2025
@jsturtevant jsturtevant dismissed stale reviews from syntactically and ludfjig via f5265a0 December 2, 2025 21:29
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Dec 2, 2025

Added the reference and removed the handler API code since we have major changes make that more platform independent.

syntactically
syntactically previously approved these changes Dec 2, 2025
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bugfix For PRs that fix bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception handler with instruction that requires 16btye alignement causes GP fault

4 participants