Re-Enable StructABI test for xplat#18496
Conversation
|
Fixes #18485 |
|
/cc @dotnet/jit-contrib |
CarolEidt
left a comment
CarolEidt
left a comment
There was a problem hiding this comment.
This is great - thanks for doing this!
You mentioned that there was an issue with this test if not building tests natively - was fixing it as simple as quoting the name of the lib in the DllImport or was there some other fix I missed seeing?
| <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> | ||
| <CLRTestPriority>1</CLRTestPriority> |
There was a problem hiding this comment.
Great! Not sure why this wouldn't always have been Pri0 as long as it was not disabled.
There was a problem hiding this comment.
The csproj had the 1 set, so the project would not have been built (or run) without priority=1.
|
Correct, there was some cruft in the test to specify the dll to call into. This looked like it was required in older released of core, but has now been fixed. |
|
I think the test still has a fatal flaw, in that the structs passed are never promoted. My guess is because they are immediately returned. It is worth further investigation to possible add another call which does not echo the structs, to hopefully test promotion. @CarolEidt is there a stress mode that overly promotes struct types? Or any thoughts around such a mode? |
|
Thank you for reviewing! |
The bigger issue is that they've been passed in as parameters. Hopefully that will improve soon. In my test for #18499 I used a static struct to get around that.
This was proposed in #11888 |
| ok = EnoughRegistersSysV2Wrapper(); | ||
| ok = EnoughRegistersSysV3Wrapper(); | ||
| ok = EnoughRegistersSysV4Wrapper(); | ||
|
|
There was a problem hiding this comment.
This test is going to pass now if the last test passes -- it will ignore all other test case results.
There was a problem hiding this comment.
Thank you for catching this. I have opened #18529 to address it.
Commit migrated from dotnet/coreclr@6a35855
3 participants
I have also doubled the callsites to test the managed-to-managed caller/callee pair.
This change will also set this test to run by default as an innerloop test.