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 for "ld.lld: error: undefined symbol: _Uaarch64_setcontext" when doing build for FreeBSD-arm64 using internal libunwind #110432

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sec
Copy link
Contributor

@sec sec commented Dec 5, 2024

Enable runtime build under FreeBSD-arm64 when using internal libunwind (atm. port version have bug that crash sdk randomly). Setting CLR_CMAKE_USE_SYSTEM_LIBUNWIND 0 produce working SDK. cc @am11 for the tip.

…doing build for arm64 using internal libunwind
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 5, 2024
@teo-tsirpanis teo-tsirpanis added the os-freebsd FreeBSD OS label Dec 5, 2024
@@ -96,7 +96,7 @@ elseif(CLR_CMAKE_TARGET_FREEBSD)
set(libunwind_la_SOURCES_arm_os arm/Gos-freebsd.c)
set(libunwind_la_SOURCES_arm_os_local arm/Los-freebsd.c)
set(libunwind_la_SOURCES_aarch64_os aarch64/Gos-freebsd.c)
set(libunwind_la_SOURCES_aarch64_os_local aarch64/Los-freebsd.c)
set(libunwind_la_SOURCES_aarch64_os_local aarch64/Los-freebsd.c aarch64/setcontext.S)
Copy link
Member

@am11 am11 Dec 5, 2024

Choose a reason for hiding this comment

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

We need both (only setcontext.S was missing)

Suggested change
set(libunwind_la_SOURCES_aarch64_os_local aarch64/Los-freebsd.c aarch64/setcontext.S)
set(libunwind_la_SOURCES_aarch64_os_local aarch64/Los-freebsd.c)
set(libunwind_aarch64_la_SOURCES_os aarch64/setcontext.S)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't what my change set(libunwind_la_SOURCES_aarch64_os_local aarch64/Los-freebsd.c aarch64/setcontext.S) is doing? Am I wrong, that second set will overwrite the previous one? So it's just ok to add aarch64/setcontext.S to the existing line with set?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, corrected the patch. Lets put them on separate lines for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New lines added.

Copy link
Member

Choose a reason for hiding this comment

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

I meant keep the existing style used in this file (one line added, zero removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm lost - there are few ways of doing set in this file. It's not possible to only add one line in here, as there's matching bracket on the line end when calling set, and this needs to go into the same variable which is included later?

Copy link
Member

Choose a reason for hiding this comment

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

The naming convention here is taken from libunwind, how they classify these files. Source are arch specific, others are OS specific and then few are machine independent (mi). Using the existing convention, that will become main...am11:runtime:patch-24.

Two lines because this is the first assembly file in aarch64 group. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, wasn't feeling good last days, now should be ok..

@am11 am11 added area-PAL-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 5, 2024
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Thank you!

@am11 am11 requested a review from janvorli December 6, 2024 13:05
@@ -229,7 +230,7 @@ set(libunwind_la_SOURCES_aarch64_common
# The list of files that go into libunwind:
set(libunwind_la_SOURCES_aarch64
${libunwind_la_SOURCES_aarch64_common}
${libunwind_la_SOURCES_aarch64_os_local}
${libunwind_la_SOURCES_aarch64_os_local} ${libunwind_aarch64_la_SOURCES_os}
Copy link
Member

Choose a reason for hiding this comment

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

Why is the aarch64/setcontext.S added only for FreeBSD and the getcontext.S for all targets? For other architectures, both are added for all targets.

Copy link
Member

Choose a reason for hiding this comment

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

@janvorli, getsetcontext is for x86 arch only and applies to Linux and FreeBSD OS alike.
FreeBSD aarch64 requirement was added in 1.8 libunwind/libunwind@032abaa. Linux aarch64 does not require it. I think we should make it clear by adding a column "OS" to the table in their readme https://github.com/libunwind/libunwind?tab=readme-ov-file#libc-requirements

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thank you for the details, that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member os-freebsd FreeBSD OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants