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

Remove druntime's libunwind dependency #4691

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

JohanEngelen
Copy link
Member

No description provided.

@JohanEngelen
Copy link
Member Author

FYI: this is one of the stumbling blocks when trying to statically link with musl libc

@@ -2,6 +2,7 @@
// REQUIRED_ARGS(linux freebsd dragonflybsd): -L-export-dynamic
// LDC (required for Win32 and -O): REQUIRED_ARGS(windows32mscoff): -link-defaultlib-debug
// LDC (FreeBSD's libexecinfo apparently doesn't like elided frame pointers): REQUIRED_ARGS(freebsd): -link-defaultlib-debug -frame-pointer=all
// LDC (our _UnwindBacktrace implementation - for musl libc - requires this): REQUIRED_ARGS(linux): -link-defaultlib-debug
Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be worthwhile to figure out what is causing the need for -link-defaultlib-debug on all these platforms. Perhaps we can solve this by preventing optimization (with magic UDA) of specific functions in druntime (or remove some other delta between optimized vs debug builds of specific functions).

Copy link
Member Author

Choose a reason for hiding this comment

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

(make new issue for that?)

Copy link
Member

Choose a reason for hiding this comment

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

Win32 backtraces are a catastrophe AFAIK, but I couldn't care less. For FreeBSD, it's the elided frame pointers as per the comment. For musl, no idea, but NOT testing the release variant for every Linux distro would IMO be a test regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah...
add the complexity of musl-specific parameters?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, that doesn't sound sexy at least. In the short term, I'd probably remove this test file for musl CI and file an issue, unless you wanna dig into the problem and try to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was already digging into it but it will cost more time than simple thing I tried. So let's remove it from CI, and file a separate issue to be solved in time.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed it by running the tests manually for now

@kinke
Copy link
Member

kinke commented Jul 3, 2024

Please let me know if you wanna include this in v1.39, then I'll wait with the release.

@JohanEngelen
Copy link
Member Author

Please let me know if you wanna include this in v1.39, then I'll wait with the release.

It would actually really be nice to get this in yes (minus the test change --> remove tests from musl CI for the time being).

@JohanEngelen JohanEngelen merged commit 7ad1a98 into ldc-developers:master Jul 3, 2024
20 checks passed
@JohanEngelen JohanEngelen deleted the rm_libunwind_dep branch July 3, 2024 21:43
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.

2 participants