Skip to content

Fix __SOFTFP__ case for arm getcontext - #73449

Merged
AaronRobinsonMSFT merged 1 commit into
dotnet:mainfrom
am11:feature/native/libunwind-arm
Aug 5, 2022
Merged

Fix __SOFTFP__ case for arm getcontext#73449
AaronRobinsonMSFT merged 1 commit into
dotnet:mainfrom
am11:feature/native/libunwind-arm

Conversation

@am11

@am11am11 commented Aug 5, 2022

Copy link
Copy Markdown
Member

@ghost

ghost commented Aug 5, 2022

Copy link
Copy Markdown

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.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Aug 5, 2022
@am11am11 added the area-Infrastructure-coreclr Only use for closed issues label Aug 5, 2022
@ghost

ghost commented Aug 5, 2022

Copy link
Copy Markdown

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

Issue Details

Upstream PR: libunwind/libunwind#400.

Author:am11
Assignees:-
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone:-

@am11am11 added area-PAL-coreclr only for closed issues and removed area-Infrastructure-coreclr Only use for closed issues labels Aug 5, 2022
@am11am11 closed this Aug 5, 2022
@am11am11 reopened this Aug 5, 2022
@am11
am11force-pushed the feature/native/libunwind-arm branch from 3fb94e7 to 52793f9CompareAugust 5, 2022 14:49
@am11
am11 marked this pull request as ready for review August 5, 2022 16:53
@am11

am11 commented Aug 5, 2022

Copy link
Copy Markdown
MemberAuthor

Linux arm checked failure is #73247. I have double checked that the leg is actually building with hard fp support (and #ifdef __SOFTFP__ is false).

We use __SOFTFP__ macro in debugger support e.g.

#if defined(__arm__) && defined(__VFP_FP__) && !defined(__SOFTFP__)
so it is a known macro for both clang and gcc arm soft float toolchains.

@janvorlijanvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you!

@AaronRobinsonMSFT

Copy link
Copy Markdown
Member

@am11 Any concerns with waiting until upstream takes this?

@am11

am11 commented Aug 5, 2022

Copy link
Copy Markdown
MemberAuthor

No concerns (with a but) :)
Just wanted to note that it might be useful for .NET 7 as linux-armv6 is new platform in 7.0 and CLR_CMAKE_HOST_ARCH_ARMV6 is used in libunwind.cmake; as corehost requires coreclr-pal (even for RuntimeFlavor=mono). Though I am not sure if we support (or have CI coverage for) softfp variant with armv6 as the compilation should had failed in the CI. git grep '\-mfpu=' suggests we don't but in places without -mfpu option set, it defaults to toolchain's baseline, so it is unclear if we actually support softfp/-mfpu=none.

@am11

am11 commented Aug 5, 2022

Copy link
Copy Markdown
MemberAuthor

Upstream PR is merged.

@AaronRobinsonMSFT

Copy link
Copy Markdown
Member

Issues are #73247 and #73474.

@AaronRobinsonMSFT
AaronRobinsonMSFT merged commit e2511ae into dotnet:mainAug 5, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 5, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-PAL-coreclronly for closed issuescommunity-contributionIndicates 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

@am11@AaronRobinsonMSFT@janvorli