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

Add armv8l virtualized 32-bit ARM core detection #66477

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

ayakael
Copy link
Contributor

@ayakael ayakael commented Mar 10, 2022

Expected behavior

Runtime should be able to build within 32-bit userspace on 64-bit ARM cores.

Actual behavior

Per dotnet/source-build#2781, runtime does not know to parse as arm when output of uname -m is armv8l.

Proposed modifications

This pull request modifies existing logics to parse armv8l in the same way as armv7l.

Varia

Parallel merge request on Alpine's side
Arm support on source-build at dotnet/installer#13378

Made as part of Alpine Linux dotnet6 packaging project, see dotnet/source-build#2782

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 10, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Mar 10, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Expected behavior

Runtime should be able to build within 32-bit userspace on 64-bit ARM cores.

Actual behavior

Per dotnet/source-build#2781, runtime does not know to parse as arm when output of uname -m is armv8l.

Proposed modifications

This pull request modifies existing logics to parse armv8l in the same way as armv7l.

Varia

Parallel merge request on Alpine's side
Arm support on source-build at dotnet/installer#13378

Made as part of Alpine Linux dotnet6 packaging project, see dotnet/source-build#2782

Author: ayakael
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

This looks okay to me. My only concern is that someone could build on an ARMv8 system and it would fail to run on an ARMv7 system. Is that something that can occur with this change?

@directhex
Copy link
Contributor

@jkoritzinsky I don't believe that can occur here. As it happens I already filed an issue similar to this in 2019. dotnet/arcade#4527

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

@directhex has more experience in this area and says there's no issue, so I'm okay with the change

@ayakael
Copy link
Contributor Author

ayakael commented Mar 10, 2022

@directhex Should I change the logics to cover arm*vl like in your PR?

@ayakael ayakael changed the title Add armv8l virtualized 32-bit ARM core derection Add armv8l virtualized 32-bit ARM core detection Mar 10, 2022
@akoeplinger
Copy link
Member

Yeah I think that makes sense so we're ready for armv9l 😄
There's also another check here that needs updating:

CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv7l|armv6l|aarch64|arm|s390x)$")

@ayakael
Copy link
Contributor Author

ayakael commented Mar 10, 2022

Yeah I think that makes sense so we're ready for armv9l smile There's also another check here that needs updating:

CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv7l|armv6l|aarch64|arm|s390x)$")

I'm ignorant of proper syntax, does CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv*l|aarch64|arm|s390x)$") and elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv*l) work?

@akoeplinger
Copy link
Member

It's a regex so it'd need to be .*, but I wonder if we shouldn't just use the explicit names in this case and update it once/if armv9l becomes a thing.

@ayakael
Copy link
Contributor Author

ayakael commented Mar 10, 2022

It's a regex so it'd need to be .*, but I wonder if we shouldn't just use the explicit names in this case and update it once/if armv9l becomes a thing.

That might be more prudent, indeed.

@akoeplinger akoeplinger merged commit 1281626 into dotnet:main Mar 11, 2022
@ayakael ayakael deleted the armv8-support branch March 11, 2022 15:21
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure 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.

4 participants