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

Convert JIT/CodeGenBringUpTests to a merged test group #85847

Merged
merged 5 commits into from
May 18, 2023

Conversation

markples
Copy link
Member

@markples markples commented May 5, 2023

See https://github.com/markples/utils/tree/for-PR-dotnet-runtime-85847-others for ILTransform tool. As usual, I recommend viewing the commit list since it partitions the changes in a more readable way and paying more attention to manual changes.

pre-merge:

  • update porting-ryujit.md
  • validation that codegenbringup scenarios still work
  • outerloop, extra-platforms, gcstress

@markples markples added the test-enhancement Improvements of test source code label May 5, 2023
@markples markples self-assigned this May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023

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

Issue Details

null

Author: markples
Assignees: markples
Labels:

area-System.Reflection.Metadata, test-enhancement

Milestone: -

@markples markples added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection.Metadata labels May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023

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

Issue Details

See https://github.com/markples/utils/tree/for-PR-dotnet-runtime-85847-others for ILTransform tool. As usual, I recommend viewing the commit list since it partitions the changes in a more readable way.

Author: markples
Assignees: markples
Labels:

test-enhancement, area-CodeGen-coreclr

Milestone: -

@markples
Copy link
Member Author

markples commented May 6, 2023

/azp run runtime-coreclr outerloop, runtime-extra-platforms, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@markples
Copy link
Member Author

Hi @clamp03 @alpencolt - this PR is one of many that changes how we usually run tests. Very briefly, we combine many tests into one process for execution for efficiency. While this mode could theoretically be used directly for bringup testing, setting BuildAsStandalone to true when building the tests can restore the old behavior where you get a script per test project to run that test alone. Since you have been using JIT/CodeGetBringUpTests, could you please verify that this branch isn't going to cause you any trouble? I'd like to avoid merging something that blocks you. Thank you!

@markples markples marked this pull request as ready for review May 12, 2023 11:46
@markples
Copy link
Member Author

This is ready @trylek @dotnet/jit-contrib (though see my previous comment). I have built this directory with set BuildAsStandalone=true.

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@clamp03
Copy link
Member

clamp03 commented May 13, 2023

Hi @clamp03 @alpencolt - this PR is one of many that changes how we usually run tests. Very briefly, we combine many tests into one process for execution for efficiency. While this mode could theoretically be used directly for bringup testing, setting BuildAsStandalone to true when building the tests can restore the old behavior where you get a script per test project to run that test alone. Since you have been using JIT/CodeGetBringUpTests, could you please verify that this branch isn't going to cause you any trouble? I'd like to avoid merging something that blocks you. Thank you!

@markples Thank you for the notification!!!

@t-mustafin @gbalykov @alpencolt Could you check this? Thank you in advance. (cc @HJLeee @wscho77)

@t-mustafin
Copy link
Contributor

@clamp03 @markples thanks for notification.

I checked this changes with our usual testing way. There are three things I want to share.

  1. BuildAsStandalone=true build allows to get separate .dll files for tests which could be successfully started by corerun. All CodeGenBringUpTests passed during run on this build. So this PR does not get us into trouble.

  2. Default (no BuildAsStandalone env) build produces separate .dll files which could not be launched by corerun due to lack of entry point. On launching general ./JIT/CodeGenBringUpTests/JIT.CodeGenBringUpTests/JIT.CodeGenBringUpTests.dll SIGSEGV occurs:

(gdb) bt full
#0  0x0000003ff6fde478 in MethodTable::GetWriteableData_NoLogging (this=0x1) at /home/runtime/src/coreclr/vm/methodtable.h:2753
No locals.
#1  0x0000003ff702a60c in MethodTable::IsRestored_NoLogging (this=0x1) at /home/runtime/src/coreclr/vm/methodtable.h:860
No locals.
#2  0x0000003ff71f01d6 in TypeHandle::IsRestored_NoLogging (this=0x3fffffd2d0) at /home/runtime/src/coreclr/vm/typehandle.cpp:1039
No locals.
#3  0x0000003ff71f0080 in TypeHandle::Verify (this=0x3fffffd2d0) at /home/runtime/src/coreclr/vm/typehandle.cpp:34
No locals.
#4  0x0000003ff70e41d8 in TypeHandle::TypeHandle (this=0x3fffffd2d0, aPtr=0x1) at /home/runtime/src/coreclr/vm/typehandle.h:116
No locals.
#5  0x0000003ff7300806 in JIT_NewS_MP_FastPortable (typeHnd_=0x1) at /home/runtime/src/coreclr/vm/jithelpers.cpp:2423
        thread = 0x2aaab500e0
        typeHandle = {{m_asTAddr = 0x1, m_asPtr = 0x1, m_asMT = 0x1, m_asTypeDesc = 0x1, m_asParamTypeDesc = 0x1, m_asTypeVarTypeDesc = 0x1, m_asFnPtrTypeDesc = 0x1}}
        methodTable = 0x3fffffd9e8
        allocContext = 0x3fffffd350
        size = 0x3f78275b68
        allocPtr = 0x0
        object = 0x0
        __me = 0x0
#6  0x0000003f78275efc in ?? ()

This SIGSEGV exists on RISCV64 and does not exists on x64. We had not got cause of this fault yet.

  1. Inside of default (no BuildAsStandalone env) build there are duplication of dll's in particular and general directory:
md5sum ./JIT/CodeGenBringUpTests/Add1_d/Add1_d.dll ./JIT/CodeGenBringUpTests/JIT.CodeGenBringUpTests/Add1_d.dll 
94f09249559a851ea67177e102996859  ./JIT/CodeGenBringUpTests/Add1_d/Add1_d.dll
94f09249559a851ea67177e102996859  ./JIT/CodeGenBringUpTests/JIT.CodeGenBringUpTests/Add1_d.dll

State main branch 4c0f2e7 with markples:JIT/CodeGenBringUpTests merged.
Platform VisionFive2/Bookworm

@markples
Copy link
Member Author

Thank you for checking this, @t-mustafin

  1. BuildAsStandalone=true build allows to get separate .dll files for tests which could be successfully started by corerun. All CodeGenBringUpTests passed during run on this build. So this PR does not get us into trouble.

Great, I am, of course, happy to hear this.

  1. Default (no BuildAsStandalone env) build produces separate .dll files which could not be launched by corerun due to lack of entry point. On launching general ./JIT/CodeGenBringUpTests/JIT.CodeGenBringUpTests/JIT.CodeGenBringUpTests.dll SIGSEGV occurs:

Yes, this first part is expected. The general JIT.CodeGenBringUpTests.dll that you found calls the tests in each of the individual test DLLs directly.

This adds some new code that runs before the actual tests. It is intended to be minimal. It contains build-time generated code to run tests that are marked with xunit-style attributes without things like runtime reflection. It looks like that is an allocation method, which perhaps isn't supported yet? If so, that would mean that BuildAsStandalone=true is the only way to run the initial tests, which would be useful feedback that we need to make sure that we don't eliminate that mode without finding an alternative for this scenario. (We don't have immediate plans to eliminate it other than vague thoughts that it's always good to have fewer modes to maintain.)

  1. Inside of default (no BuildAsStandalone env) build there are duplication of dll's in particular and general directory:

My guess is that this is a byproduct of msbuild handling the merged group application and the individual test libraries. This might be standard behavior for the intermediate steps ("obj" directories), but perhaps there is a way to avoid it in the final test artifacts since the individual test directories shouldn't need to exist.

Thank you for the feedback! It sounds like this PR is ok to merge because of (1) but that there may be some future improvements that we can make. Am I summarizing your reply correctly?

@trylek
Copy link
Member

trylek commented May 15, 2023

I don't think we'll want to remove BuildAsStandalone anytime soon, we promised that to multiple stakeholders, in particular the JIT team and the low-level runtime people were super passionate about keeping the ability to run just a single test, period. From my perspective I'm much more looking forward to removing the obsolete xUnit wrapper generator crap and two variants of running the tests, both locally and in Helix, from the test build / execution scripts.

@t-mustafin
Copy link
Contributor

Thank you for the feedback! It sounds like this PR is ok to merge because of (1) but that there may be some future improvements that we can make. Am I summarizing your reply correctly?

Yes, it is correct.

@markples markples merged commit 0096ba5 into dotnet:main May 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants