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

Fix lifetime1 test #55371

Merged
merged 1 commit into from
Jul 9, 2021
Merged

Conversation

BruceForstall
Copy link
Member

The test assumed that a GC didn't occur between the last use of a variable and a check
of a value set in the finalizer. This presumably worked up until now because the test
is set to build in debuggable code mode. But R2R compilations ignore that and build
with optimization enabled, and GCStress exposes the issue.

Fix the test by explicitly marking a variable as KeepAlive until after the requisite check.

Fixes #54042

The test assumed that a GC didn't occur between the last use of a variable and a check
of a value set in the finalizer. This presumably worked up until now because the test
is set to build in debuggable code mode. But R2R compilations ignore that and build
with optimization enabled, and GCStress exposes the issue.

Fix the test by explicitly marking a variable as KeepAlive until after the requisite check.

Fixes dotnet#54042
@BruceForstall
Copy link
Member Author

@davidwrighton @dotnet/jit-contrib PTAL

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Looks good to me. I couldn't figure out what exactly this was supposed to test, but the updated code looks like it should work.

@jakobbotsch
Copy link
Member

Is this supposed to test that the JIT properly extends the lifetime of refs in debug? Should crossgen2 be fixed to honor the debug flag instead? #55062 might be related if so.

@BruceForstall
Copy link
Member Author

Is this supposed to test that the JIT properly extends the lifetime of refs in debug?

That's a reasonable guess as to the intention.

Should crossgen2 be fixed to honor the debug flag instead?

I'm not sure that's a user experience cg2 would want to support.

An alternative fix is simply to disable the test for AOT compilation / R2R.

@BruceForstall
Copy link
Member Author

Is this supposed to test that the JIT properly extends the lifetime of refs in debug?

It occurs to me that the test would be incorrect, anyway, since it sets a = null and expects no GC to occur after, before the end of the function, which is an invalid assumption. Perhaps removing the a = null would get at the "extend lifetimes" goal.

However, at the risk of possibly reduced test coverage, I'm going to check in the change as-is, so it works in non-debug.

@BruceForstall BruceForstall merged commit d5aa75c into dotnet:main Jul 9, 2021
@BruceForstall BruceForstall deleted the FixLifetimeTest branch July 9, 2021 16:03
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure JIT/Directed/lifetime/lifetime1/lifetime1.sh
3 participants