Skip to content

Conversation

am11
Copy link
Member

@am11 am11 commented Apr 1, 2025

As discussed here: #72655 (comment)

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 1, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 1, 2025
@am11 am11 added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 1, 2025
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@am11 am11 requested a review from janvorli April 1, 2025 15:37
@am11
Copy link
Member Author

am11 commented Apr 2, 2025

@janvorli this is ready.


In our patches, we are doing stuff like

- template <typename A>
- int CompactUnwinder_arm64<A>::stepWithCompactEncoding(
+ template <typename A, typename R>
+ int CompactUnwinder_arm64<A, R>::stepWithCompactEncoding(

even though these are arch-specific, we pass our own R=Registers_REGDISPLAY to satisfy the static_asserts which check for A<=R match and fail upstream.

Not sure if it is possible at all to upstream the patches in a manner that distro provided llvm-libunwind can be used without needing to rebuild a different flavor for .NET, if we can determine it's not, we should just remove

<!-- TODO: llvm-libunwind -->
cc @tmds

@janvorli
Copy link
Member

janvorli commented Apr 2, 2025

This change was added relatively recently in in cb6fb60. I wonder if we can do it differently to minimize the changes upstream.

@am11
Copy link
Member Author

am11 commented Apr 2, 2025

It is probably possible to make C API contracts (in libunwind.h) and not directly modify the (internal) C++ APIs.

@janvorli
Copy link
Member

janvorli commented Apr 2, 2025

Not sure if it is possible at all to upstream the patches in a manner that distro provided llvm-libunwind can be used without needing to rebuild a different flavor for .NET

I am not sure either. Some things might be solved by not using REGDISPLAY inside of the libunwind at the cost of some copying of the context, but there were other changes made where we access internals of the libunwind for performance reasons, like dotnet/corert#5076. That is not something we could without using the full sources.

@am11
Copy link
Member Author

am11 commented Apr 2, 2025

I think it is a reasonable explanation. The portion of llvm-libunwind used in NativeAOT is an internal detail. In coreclr PAL, HP libunwind is strictly used via the public API, so the choice of using system libunwind in source-build is feasible.

BTW, llvm-libunwind lists 200 dwarf codes

(despite the name, it's for all DWARF versions) out of which they handle 54 unique cases (rest are grouped):

ilc objwriter has writer implementation in the directory where these ~530 codes live (could be reduced to the same number llvm-libunwind supports)

maybe with some effort, we can add DwarfReader as well to coin our own run-time unwinder to get rid of llvm-libunwind dependency; given we are half way there dealing with binary formats without any dependency.

@janvorli janvorli closed this Jun 2, 2025
@janvorli janvorli reopened this Jun 2, 2025
@janvorli
Copy link
Member

janvorli commented Jun 2, 2025

@am11 I am sorry, this got out of my radar. I have just closed / reopened the PR to recheck stuff.

@agocke
Copy link
Member

agocke commented Jul 15, 2025

@janvorli anything else?

@janvorli
Copy link
Member

@agocke I'm fine with it. The build analysis seems to be somehow confused, it shows errors that the list above doesn't show.

@agocke
Copy link
Member

agocke commented Jul 16, 2025

/ba-g failures from BA are stale

@agocke agocke merged commit 05e00fc into dotnet:main Jul 16, 2025
157 checks passed
@am11 am11 deleted the patch-30 branch July 16, 2025 23:17
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants