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.

Sign up for GitHub

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

Jump to bottom

Fix CoreRT frozen strings handling. #45095

Merged
merged 5 commits into from
Nov 24, 2020
Merged

Fix CoreRT frozen strings handling. #45095

merged 5 commits into from
Nov 24, 2020

Conversation

sandreenko
Copy link
Contributor

Fix asserts about const non-zero refs that are possible on CoreRT, in the past they were always hidden with GT_NOP but it was creating other issues.

It should fix failures seen in dotnet/runtimelab#374 (comment) introduced by #44398.

With the changes the tests results are:

src\tests\run.cmd runnativeaottests Checked
Total tests run    : 2578
Total passing tests: 2565
Total failed tests : 13
Total skipped tests: 0

the 13 failing tests are failing without the change as well, so they are probably preexisting, PTAL @jkotas :

JIT\Directed\tailcall\mutual_recursion\mutual_recursion.cmd [FAIL]	Line 271:       JIT\Methodical\tailcall_v4\hijacking\hijacking.cmd [FAIL]
JIT\opt\Tailcall\TailcallVerifyWithPrefix\TailcallVerifyWithPrefix.cmd [FAIL]
JIT\Regression\JitBlue\devdiv_902271\DevDiv_902271\DevDiv_902271.cmd [FAIL]
JIT\Regression\JitBlue\GitHub_27924\GitHub_27924\GitHub_27924.cmd [FAIL]
JIT\Regression\CLR-x86-JIT\V2.0-Beta2\b353858\b353858\b353858.cmd [FAIL]
JIT\Regression\CLR-x86-JIT\V1-M12-Beta2\b71120\b71120\b71120.cmd [FAIL]
JIT\Stress\ABI\pinvokes_d\pinvokes_d.cmd [FAIL]
JIT\Stress\ABI\tailcalls_d\tailcalls_d.cmd [FAIL]
JIT\Stress\ABI\stubs_do\stubs_do.cmd [FAIL]
JIT\Stress\ABI\pinvokes_do\pinvokes_do.cmd [FAIL]
JIT\Stress\ABI\tailcalls_do\tailcalls_do.cmd [FAIL]
Loader\classloader\MethodImpl\CovariantReturns\UnitTest\OverrideMoreDerivedReturn\OverrideMoreDerivedReturn.cmd [FAIL]

before the fix there were 90 failures including this 13.

PTAL @dotnet/jit-contrib

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 23, 2020
jkotas
jkotas reviewed Nov 23, 2020
View reviewed changes
src/coreclr/src/jit/assertionprop.cpp Outdated
@@ -1625,7 +1625,11 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion)
assert(assertion->op2.u1.iconFlags != 0);
break;
case O1K_LCLVAR:
assert((lvaTable[assertion->op1.lcl.lclNum].lvType != TYP_REF) || (assertion->op2.u1.iconVal == 0));
if (!IsTargetAbi(CORINFO_CORERT_ABI)) // CoreRT allows non-zero const refs for frozen strings.
Copy link
Member

@jkotas jkotas Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The frozen strings are not really inherent part of CoreRT ABI. Nothing prevents them from being used with CoreCLR as well, we just happen to not do that currently.

It would be better to have a bool frozenObjectsUsed; flag that gets set whenever the JIT/EE interface returns a frozen object, and use this flag for these asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, thank you, fixed.

@jkotas
Copy link
Member

jkotas commented Nov 23, 2020

Total failed tests : 13

@MichalStrehovsky Is this known?

jkotas
jkotas approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@MichalStrehovsky
Copy link
Member

Total failed tests : 13

@MichalStrehovsky Is this known?

Nope but we're not running the tests very often and the runs have been very noisy ever since we picked up the regression this and the previous pull request is fixing.

I'll take a look once this is mirrored over.

Maybe we should make Pri0 runs mandatory in the CI.

@runfoapp runfoapp bot mentioned this pull request Nov 23, 2020
Closed
AndyAyersMS
AndyAyersMS reviewed Nov 23, 2020
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Currently you have a broad permissive check. Should we instead do more surgical fine-grained checks by instead asserting that any nonzero TYP_REF integer must be a string handle?

I'm surprised we don't hit asserts elsewhere.

src/coreclr/src/jit/gentree.cpp
noway_assert(!"unreachable IAT_VALUE case in gtNewStringLiteralNode");
}

setMethodHasFrozenString();
Copy link
Member

Choose a reason for hiding this comment

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

We only see these on CoreRT, so should we keep part of the old assertion check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#45095 (comment) there was a discussion about it

@sandreenko sandreenko marked this pull request as draft November 23, 2020 23:27
@sandreenko sandreenko marked this pull request as ready for review November 23, 2020 23:50
sandreenko
sandreenko commented Nov 23, 2020
View reviewed changes
src/coreclr/src/jit/compiler.hpp
assert(obj->IsIconHandle(GTF_ICON_STR_HDL));
if (!doesMethodHaveFrozenString())
{
assert(compIsForInlining());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed additional changes for a case where we inline a method and pass such string there(as an argument), the inlinee doesn't have a frozen string flag so the check was failing.

@sandreenko sandreenko merged commit 1fa2054 into dotnet:master Nov 24, 2020
@sandreenko sandreenko deleted the fixCoreRT branch November 24, 2020 02:42
@jkotas
Copy link
Member

jkotas commented Nov 24, 2020

Thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sandreenko @jkotas @MichalStrehovsky @AndyAyersMS