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

Enable NativeAOT win-x86 runtime tests #99688

Merged
merged 5 commits into from
Apr 1, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Mar 13, 2024

See #99688 (comment) for results of the last run.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 13, 2024
@filipnavara filipnavara added arch-x86 area-NativeAOT-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Mar 13, 2024
@filipnavara
Copy link
Member Author

filipnavara commented Mar 13, 2024

Can someone run /azp run runtime-nativeaot-outerloop, please?

@jakobbotsch
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Mar 13, 2024
@filipnavara
Copy link
Member Author

Can I get a re-run please? (Most of the individual commits are submitted as separate PRs; I'll rebase once they land and leave just the YAML changes here.)

@jakobbotsch
Copy link
Member

/azp run runtime-nativeaot-outerloop

@filipnavara
Copy link
Member Author

Remaining failed runtime test should be fixed by #99753. Since the Azure bot didn't respond to the /azp run runtime-nativeaot-outerloop command (😒) I pushed the additional change here as well.

@EgorBo
Copy link
Member

EgorBo commented Mar 14, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara
Copy link
Member Author

Nearly all MSQUIC tests started failing on the linux-arm (32-bit) leg. Wasn't there a MSQUIC update done recently that could have broken something? /cc @ManickaP

@MichalPetryka
Copy link
Contributor

Nearly all MSQUIC tests started failing on the linux-arm (32-bit) leg. Wasn't there a MSQUIC update done recently that could have broken something? /cc @ManickaP

#99681

@filipnavara
Copy link
Member Author

Rebased over the merged fixes. Can I get a /azp run runtime-nativeaot-outerloop rerun please?
image

@am11
Copy link
Member

am11 commented Mar 15, 2024

@filipnavara, I think being a member of @dotnet/external-ci-access, you can also trigger the leg with /az commands. :)

@filipnavara
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Commenter does not have sufficient privileges for PR 99688 in repo dotnet/runtime

@filipnavara
Copy link
Member Author

filipnavara commented Mar 15, 2024

@filipnavara, I think being a member of @dotnet/external-ci-access, you can also trigger the leg with /az commands. :)

Cool. Didn't know that was a thing. Unfortunately, it doesn't work.

@khellang
Copy link
Member

khellang commented Mar 15, 2024

@am11 And by @'ing that team, you just sent a notification to 166 people 😆 Hello everyone 👋

@am11
Copy link
Member

am11 commented Mar 15, 2024

Commenter does not have sufficient privileges for PR 99688 in repo dotnet/runtime

That's sad. I think dotnet/runtime is not part of that group?

And by @'ing that team, you just sent a notification to 166 people 😆 Hello everyone 👋

Ah, sorry I forgot to wrap it in ` 😂 but good things are happening here 😉

@@ -2,6 +2,7 @@
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
<RequiresProcessIsolation>true</RequiresProcessIsolation>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unintended change. I'll get rid of it with next rebase.

@khellang
Copy link
Member

but good things are happening here 😉

For sure. Keep up the good work @filipnavara! 💪😎

@jakobbotsch
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Mar 20, 2024

The System.Threading.Tests failure is in similar part of code that I tried to fix with #99934 (which did fix it in Release builds):

I think the stackunwind for the assembly code also needs to simulate pop of the 16 bytes worth of arguments. It means that you would need to special-case this method.

An alternative is to move the null check to managed code like I have done in #96916 for CoreCLR. It would allow the interlocked helper to be implemented in C/C++ instead of assembly.

@filipnavara
Copy link
Member Author

filipnavara commented Mar 20, 2024

I think the stackunwind for the assembly code also needs to simulate pop of the 16 bytes worth of arguments. It means that you would need to special-case this method.

I think you are right!

An alternative is to move the null check to managed code like I have done in #96916 for CoreCLR. It would allow the interlocked helper to be implemented in C/C++ instead of assembly.

Sounds like a reasonable thing to do. I'll submit a PR later today (at least for moving the null check, and if feasible the move to C++ as well).

@MihaZupan
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara
Copy link
Member Author

🎉 All the runtime tests passed! 🎉

Libraries tests:

@filipnavara filipnavara force-pushed the win-x86-runtime-tests branch from 4952a38 to 34b33d1 Compare March 28, 2024 22:14
@filipnavara filipnavara marked this pull request as ready for review March 28, 2024 22:14
@filipnavara
Copy link
Member Author

This is finally unblocked since all dependent PRs were merged.

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit c5200b6 into dotnet:main Apr 1, 2024
135 of 140 checks passed
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x86 area-NativeAOT-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.