Skip to content

Remove lifetime2 test and remove its crossgen2 test exclusion#117995

Merged
kg merged 2 commits intodotnet:mainfrom
kg:issue109313
Jul 24, 2025
Merged

Remove lifetime2 test and remove its crossgen2 test exclusion#117995
kg merged 2 commits intodotnet:mainfrom
kg:issue109313

Conversation

@kg
Copy link
Copy Markdown
Member

@kg kg commented Jul 23, 2025

Should fix #109313

EDIT: Based on jkotas's feedback below I'm removing this test. I agree that it is not testing anything of value that isn't already covered well by the rest of our test suite, and I couldn't see an easy way to refactor it into a reliable test that still attempts to do the same things.

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 23, 2025
@kg
Copy link
Copy Markdown
Member Author

kg commented Jul 23, 2025

/azp run runtime-coreclr crossgen2 outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Copy Markdown
Member Author

kg commented Jul 23, 2025

Unfortunately the part I couldn't figure out how to refactor is the one that broke, and it doesn't fail on windows. This is kind of a recurring theme with R2R... things do not behave the same on linux/mac as they do on windows.

{
C b = new C();
b = null;
GC.Collect();
GC.WaitForPendingFinalizers();
Console.WriteLine();
Console.WriteLine("testcase f3");
if (aExists != 0)
{
Console.WriteLine("f3 failed");
return -1;
}
b = null;
return 100;
}

In this case, it seems like the test author expects the = null to make the original value of the variable unreachable immediately so it will get collected and finalized. In C# terms this is logical, and in IL terms it's the same local variable so we are overwriting it with null. But is the JIT required to behave this way? Worse still, b is nulled out a second time later, and it's never read from, so these null stores are effectively dead - their only observable effect would be on the liveness of the object inside b.

I could modify this part of the test to make it pass consistently by using NoInlining as I did on other parts of it but it feels like maybe it's intentionally trying to codify some fussy behavior in the JIT that we wish to preserve and that behavior is somehow only broken for composite R2R on linux/mac.

cc @jkotas this is one of the tests I've been looking at.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Jul 23, 2025

I would delete this test. I do not think it is testing anything interesting.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Jul 23, 2025

(And whatever it was attempting to test is probably not going to be tested after it is fixed to be reliable.)

@kg kg changed the title Rework lifetime2 test so it uses noinlining'd methods as lifetime barriers + enable for crossgen2 Remove lifetime2 test and remove its crossgen2 test exclusion Jul 24, 2025
@kg kg marked this pull request as ready for review July 24, 2025 14:24
Copilot AI review requested due to automatic review settings July 24, 2025 14:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the lifetime2 test from the JIT test suite and eliminates its corresponding crossgen2 test exclusion. The change addresses issue #109313 by completely removing a test that was deemed unreliable and not testing anything of value that isn't already covered by other tests in the suite.

Key Changes:

  • Complete removal of the lifetime2 test files and project
  • Removal of the crossgen2 exclusion entry for this test from the issues.targets file

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/tests/issues.targets Removes the crossgen2 exclusion entry for the lifetime2 test
src/tests/JIT/Directed/lifetime/lifetime2.csproj Completely removes the project file for the lifetime2 test
src/tests/JIT/Directed/lifetime/lifetime2.cs Completely removes the C# source code for the lifetime2 test

@kg kg merged commit 2a7789d into dotnet:main Jul 24, 2025
75 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crossgen outerloop regression: JIT/Directed/lifetime/lifetime2/lifetime2.cmd

3 participants