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

Simplify platform macro definitions in PAL #73530

Merged
merged 6 commits into from
Aug 26, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Aug 7, 2022

  • Delete _M_{ARCH} definitions and replace usage with HOST_{ARCH} and TARGET_{ARCH}.
  • Remove redundant ${arch} from filenames in architecture specific directories.
  • Remove Arm from PAL_ArmInterlockedOperationBarrier and ArmInterlockedOperationBarrier as they are not ARM-specific anymore.
  • Add a few empty stubs for riscv64 so we can run src/coreclr/build-runtime.sh -cross -a riscv64 -component paltests (without running into missing files errors).

@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 ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 7, 2022
@am11 am11 requested review from jkotas and janvorli August 7, 2022 15:27
@am11 am11 marked this pull request as ready for review August 7, 2022 15:27
@am11 am11 requested a review from MichalStrehovsky as a code owner August 7, 2022 15:27
@am11 am11 force-pushed the feature/consolidation/platform-definitions branch from 16839f6 to 28204ea Compare August 9, 2022 12:42
@am11
Copy link
Member Author

am11 commented Aug 9, 2022

@janvorli, the CI issue was fixed after the rebase. PTAL.

@janvorli
Copy link
Member

@am11 I am sorry for the delay, I was OOF for the last 7 days.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@am11
Copy link
Member Author

am11 commented Aug 19, 2022

@EgorBo, any idea why superpmi leg would fail the osx-arm64 checks when I updated PAL_CS_NATIVE_DATA_SIZE only for linux-arm64 to 104 (osx-arm64 is 120 which hasn't changed):

[16:48:11] Invoking: C:\h\w\9D87090C\p\artifacts\spmi\pintools\1.0\windows\pin.exe -follow_execv -t C:\h\w\9D87090C\p\artifacts\spmi\pintools\1.0\windows\clrjit_inscount_x64\clrjit_inscount.dll -quiet -- C:\h\w\9D87090C\p\superpmi.exe -applyDiff -baseMetricsSummary C:\h\w\9D87090C\t\tmpsfrtx1z4\libraries_tests.pmi.OSX.arm64.checked.mch_base_metrics.csv -diffMetricsSummary C:\h\w\9D87090C\t\tmpsfrtx1z4\libraries_tests.pmi.OSX.arm64.checked.mch_diff_metrics.csv -target arm64 -p -failureLimit 100 C:\h\w\9D87090C\p\base\release\clrjit_universal_arm64_x64.dll C:\h\w\9D87090C\p\diff\release\clrjit_universal_arm64_x64.dll C:\h\w\9D87090C\p\artifacts\spmi\mch\1b9551b8-21f4-4233-9c90-f3eabd6a322b.OSX.arm64\libraries_tests.pmi.OSX.arm64.checked.mch

[16:48:15] ERROR: Unexpected exception c0000005 was thrown.

[16:48:15] ERROR: Unexpected exception c0000005 was thrown.

[16:48:15] ERROR: Method 1 of size 88 failed to load and compile correctly by JIT2 (C:\h\w\9D87090C\p\diff\release\clrjit_universal_arm64_x64.dll).

[16:48:15] ERROR: Method 1 of size 88 failed to load and compile correctly by JIT1 (C:\h\w\9D87090C\p\base\release\clrjit_universal_arm64_x64.dll).

The CI was green before PAL_CS_NATIVE_DATA_SIZE/DAC_CS_NATIVE_DATA_SIZE 116->104 change for linux-arm64.

@EgorBo
Copy link
Member

EgorBo commented Aug 19, 2022

@am11 we had some issues with it and fixed them yesterday, can you rebase your PR?

@am11 am11 closed this Aug 19, 2022
@am11 am11 reopened this Aug 19, 2022
@janvorli
Copy link
Member

@am11 let me retry the superpmi tests one more time and then I'll merge it.

@janvorli janvorli closed this Aug 25, 2022
@janvorli janvorli reopened this Aug 25, 2022
@janvorli janvorli merged commit 536f34d into dotnet:main Aug 26, 2022
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Aug 30, 2022
It was missed in dotnet#73530, before that we defined `HOST_ARM` for armv6 in pal.h too because of `defined(__arm__)`
akoeplinger added a commit that referenced this pull request Aug 30, 2022
Add missing HOST_ARM define to armv6 in configurecompiler.cmake, it was missed in #73530, before that we defined `HOST_ARM` for armv6 in pal.h too because of `defined(__arm__)`

We also only need the `clr.iltools+clr.packages` subset, not all of CoreCLR.

Also fix some dependencies that weren't working for FreeBSD since we missed the local variables. We don't need installer subset conditions there but we need to trigger on rolling builds.
@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-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