Skip to content

Bring back panic info to abort #489

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

Merged
merged 3 commits into from
May 14, 2025

Conversation

danbugs
Copy link
Contributor

@danbugs danbugs commented May 13, 2025

In #474, I removed the guest panic context region. With that, aborting was split into two steps:
(1) printing panic info w/ debug_print, and
(2) exiting w/ abort and codes.

This PR addresses feedback I got in #474 to join the operations by buffering string output when aborting.

This PR also cleans up our manual chunking logic w/ outb and leverages out8, out16, and out32 to minimize VM exits. Abort detects it should finishing buffering by a terminator code (0xFF).

Edited tests to check for panic message in aborts as they were prior to #474 merged.

This PR can be reviewed commit by commit. I also made some other minor changes like:

  • removing some legacy unused structs, and
  • adding a feature flag to logging some of our build metadata, which uses a git2 dependency that is not available on every target.

@danbugs danbugs added the kind/refactor For PRs that restructure or remove code without adding new functionality. label May 13, 2025
@danbugs danbugs force-pushed the buffer-outb-output branch from 0dc0bbf to f96c34f Compare May 13, 2025 19:35
@danbugs danbugs changed the title Brought back panic info to abort Bring back panic info to abort May 13, 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.

Looks reasonable but have some questions. Also I'm not sure thread_local is right for this. What happens when two vms are running on the same thread (not currenly possible but will be very soon)?

@danbugs danbugs force-pushed the buffer-outb-output branch 5 times, most recently from 8729e15 to 6dec279 Compare May 13, 2025 23:53
@danbugs
Copy link
Contributor Author

danbugs commented May 13, 2025

Looks reasonable but have some questions. Also I'm not sure thread_local is right for this. What happens when two vms are running on the same thread (not currenly possible but will be very soon)?

Removed thread_local buffer and moved buffer to MemMgrWrapper so that it is exclusive to a sandbox and not to a thread.

@danbugs danbugs force-pushed the buffer-outb-output branch from ce71264 to 88df03a Compare May 14, 2025 00:02
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, but old file is added back

@danbugs
Copy link
Contributor Author

danbugs commented May 14, 2025

If the 0xFF is unconditionally added by abort_with_code, then wouldn't adding it here add an additional exit, not reduce?

No. &[0, 0xFF] is a single exit. Adding this beforehand, will cause the guest to exit earlier and not get back to do the other outb.

@danbugs danbugs force-pushed the buffer-outb-output branch from 88df03a to 6f4124a Compare May 14, 2025 17:18
@danbugs danbugs dismissed ludfjig’s stale review May 14, 2025 17:20

Fixed rebase

Copy link
Contributor

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

I didn't take a super detailed look at this yet, but I'm somewhat confused by the approach.

danbugs added 2 commits May 14, 2025 19:01
HostFunctionDefinitions and GuestErrorData were unused.

Signed-off-by: danbugs <[email protected]>
…details

git2 uses deep C bindings to libgit2 that might not be available on every target, we should allow disabling logging
build details.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the buffer-outb-output branch from 6f4124a to 893108d Compare May 14, 2025 19:02
…panic info to abort

In hyperlight-dev#474, I removed the guest panic context region. With that, aborting was split into two steps:
(1) printing panic info w/ debug_print, and
(2) exiting w/ abort and codes.

This PR addresses feedback I got in hyperlight-dev#474 to join the operations by buffering string output when
aborting.

This PR also cleans up our manual chunking logic w/ outb and leverages out32 w/ length prefixing to
minimize VM exits. Abort detects it should finishing buffering by a terminator code (0xFF).

Edited tests to check for panic message in aborts as they were prior to hyperlight-dev#474 merged.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the buffer-outb-output branch from 893108d to 23dcf47 Compare May 14, 2025 19:16
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.

I agree this can be improved, but its on the unhappy path, lets add an issue for the improvements that Lucy suggested and get this done after you make the other changes, lets make sure if gets in v0.5.0 and mark the issue as such

@danbugs danbugs merged commit 23dcf47 into hyperlight-dev:main May 14, 2025
61 of 71 checks passed
@danbugs danbugs deleted the buffer-outb-output branch May 14, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor For PRs that restructure or remove code without adding new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants