[RISC-V] Add riscv64 arch to tracing/eventpipe/processinfo tests #103832
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
Open
3 tasks
RISC-V test results for starfive-prio1-checked (
|
RISC-V test results for qemu-prio1-checked (
|
am11
reviewed
Jun 22, 2024
| @@ -233,10 +233,11 @@ public static void TestEntryPoint() | |||
| Architecture.X64 => "x64", | |||
| Architecture.Arm => "arm32", | |||
| Architecture.Arm64 => "arm64", | |||
| Architecture.RiscV64 => "riscv64", | |||
There was a problem hiding this comment.
Please also add loongarch64 while updating these.
There was a problem hiding this comment.
I've added loongarch64 too, but please note that I've not verified that test passes on it now. @shushanhf @LuckyXu-HF please take a look.
There was a problem hiding this comment.
Thanks. Yes, it's funny that either that test is suppressed or it throws anyway. We have these unnecessary platform maps in too many places which add no value (just another string to update for the next platform).
There was a problem hiding this comment.
Thank you for heads up. I have verified on LA64 and these are also suitable for LA64.
LGTM, thanks very much.
am11
added
area-Tracing-coreclr
and removed
needs-area-label
This was referenced Jun 22, 2024
RISC-V test results for starfive-prio1-checked (
|
tomeksowi
approved these changes
Jun 23, 2024
clamp03
approved these changes
Jun 24, 2024
LuckyXu-HF
approved these changes
Jun 24, 2024
jkotas
reviewed
Jun 24, 2024
| @@ -224,14 +224,16 @@ public static void TestEntryPoint() | |||
| Architecture.X64 => "x64", | |||
| Architecture.Arm => "arm32", | |||
| Architecture.Arm64 => "arm64", | |||
| Architecture.RiscV64 => "riscv64", | |||
There was a problem hiding this comment.
string expectedArchValue = RuntimeInformation.ProcessArchitecture switch
{
Architecture.Arm => "arm32",
// All other architectures match the enum member name
_ => RuntimeInformation.ProcessArchitecture.ToString().ToLowerInvariant()
}
Can we make this more succinct and reduce need for updates with every new architecture?
There was a problem hiding this comment.
Thanks, I've updated PR
gbalykov
force-pushed
the
add-riscv64-processinfo
branch
from
852b3fa to
1936641
Compare
RISC-V test results for qemu-prio1-checked (
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With TieredCompilation tracing/eventpipe/processinfo tests are skipped by theirs .sh launchers. Disabling TieredCompilation leads to fails like:
This patch adds RiscV64 and makes pass tracing/eventpipe/processinfo tests with
DOTNET_TieredCompilation="0"option. Tests influenced:cc @dotnet/samsung. Part of #84834.
cc @shushanhf @LuckyXu-HF seems Loongarch64 needs same changes.