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

Fix unwinding #11

Merged
merged 2 commits into from Nov 20, 2023
Merged

Conversation

mschlumpp
Copy link
Member

libunwind has to unwind its own functions too when unwinding the complete stack. Without unwind tables this is impossible. Therefore, enable them for libunwind itself. Also remove the patch which just skipped those internal libunwind functions to work-around the missing unwind tables. This work-around caused a mismatch between the actual stack state and what the unwind tables of the user application expected.

@jviotti
Copy link

jviotti commented Nov 13, 2023

Thanks for the quick fix @mschlumpp . I confirm it works on Debian Bookworm!

@jviotti
Copy link

jviotti commented Nov 17, 2023

Is there anything pending before we can merge this? Would love to use it already, without having to point to the fork

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

This works fine @mschlumpp, thanks.

Please author the commit using the same email address that you use for the SOB (it's authored by <marco.schlumpp@gmail.com> now).
I would also add a bit more descriptive commit message, Fix unwinding is too general.

The patch just skipped internal libunwind functions to work-around the missing
unwind tables in libunwind itself. This work-around caused a mismatch between
the actual stack state and what the unwind tables of the user application
expected.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
libunwind has to unwind its own functions too when unwinding the
complete stack. Without unwind tables this is impossible. Therefore,
enable them for libunwind itself.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
@razvand razvand added the enhancement New feature or request label Nov 20, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good now, thanks.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

It's the second time we are removing this patch, hopefully this time for good! The changes are alright, thanks @mschlumpp!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand merged commit b587279 into unikraft:staging Nov 20, 2023
@mschlumpp mschlumpp deleted the mschlumpp/fix/unwinding-patch branch November 20, 2023 18:17
@jviotti
Copy link

jviotti commented Nov 21, 2023

Not that it needs to happen right now, but out of curiosity, how do changes to staging eventually migrate to the stable branch? What is the process?

@razvand
Copy link

razvand commented Nov 21, 2023

Not that it needs to happen right now, but out of curiosity, how do changes to staging eventually migrate to the stable branch? What is the process?

@jviotti , currently, when we do a release we switch things to stable, once we have tested everything. The goal would be that, in the not-so-far term, we would be able to have an automated transition of staging to stable, once we test everything.

In case of certain commits / PRs, we can do manual testing and then make their transition to stable. If you want that to happen to this PR, we can do it.

@jviotti
Copy link

jviotti commented Nov 21, 2023

Makes sense!

In case of certain commits / PRs, we can do manual testing and then make their transition to stable. If you want that to happen to this PR, we can do it.

No worries, staging is fine. I was just wondering what the process was

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants