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

[release/8.0-preview4] Comment out support for mtag extension in libunwind #85431

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Apr 27, 2023

Fixes dotnet/source-build#3424

Customer Impact

Break in source build caused by recent upgrade of libunwind from libunwind upstream. The update brought it inline assembly code that does not compile with older compilers.

Comment out the offending code since it is not needed for our libunwind use case.

Testing

Local build

Risk

Low. The change is effectively reverting recent change.

@jkotas jkotas marked this pull request as ready for review April 27, 2023 00:33
@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 Apr 27, 2023
@ghost ghost assigned jkotas Apr 27, 2023
@jkotas jkotas changed the base branch from main to release/8.0-preview4 April 27, 2023 00:34
@jkotas jkotas force-pushed the arm-build-break branch 2 times, most recently from f6aa2ec to 831d52e Compare April 27, 2023 00:52
@ghost
Copy link

ghost commented Apr 27, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: jkotas
Assignees: jkotas
Labels:

area-Infrastructure-coreclr, needs-area-label

Milestone: -

@jkotas jkotas changed the title use .arch_extension memtag when building with GCC . Comment out support for mtag extension in libunwind Apr 27, 2023
@jkotas
Copy link
Member Author

jkotas commented Apr 27, 2023

cc @mthalman

@jkotas jkotas changed the title Comment out support for mtag extension in libunwind [release/8.0-preview4] Comment out support for mtag extension in libunwind Apr 27, 2023
@jkotas jkotas added Servicing-consider Issue for next servicing release review and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 27, 2023
@jkotas jkotas requested a review from VSadov April 27, 2023 01:07
@VSadov
Copy link
Member

VSadov commented Apr 27, 2023

Regarding:

Please reference the commit modifying external source in src/native/external/llvm-libunwind-version.txt.

I assume we will do a similar change in the main and then reference that commit?

@am11

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@am11
Copy link
Member

am11 commented Apr 27, 2023

I assume we will do a similar change in the main and then reference that commit?

We can reference the commit during the forwardport to main branch (makes it easier during the updates).

@jeffschwMSFT
Copy link
Member

@jkotas @VSadov this is ready to merge. Please take a look at the CI failures.

@VSadov
Copy link
Member

VSadov commented Apr 27, 2023

It looks like the PR needs rebasing

Works around build breaks with some compiler versions.

Fixes dotnet/source-build#3424
@jeffschwMSFT
Copy link
Member

@VSadov thanks for taking a look. To make Preview 4 we will need to merge today. Hopefully we will have a green ci by then, but we will need to monitor it.

@VSadov
Copy link
Member

VSadov commented Apr 27, 2023

@jeffschwMSFT I think the failures were unrelated to the change, since this is a build fix that is specific to nativeaot+arm64+linux, but the PR got behind the base branch and became unmergeable (according to GitHub).
I've rebased the PR.

I think once NativeAOT builds on linux/arm64, we will know that the fix works (there should be no other effects), but it would be better to see green where it can be green. I'll be watching.

@mthalman
Copy link
Member

FWIW, I've verified this fix in the pipeline that originally found this issue: Debian11_Offline_MsftSdk_arm64 (internal link). This build leg was previously failing (internal link) without this change.

@jkotas jkotas merged commit 2783ef8 into dotnet:release/8.0-preview4 Apr 27, 2023
@jkotas jkotas deleted the arm-build-break branch April 27, 2023 23:02
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants