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 zero initialization use of "initReg" #58410

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

BruceForstall
Copy link
Member

If the only local is an enregistered must-init struct,
we were setting initReg to its register (in this case, xmm0).
However, initReg is expected to be an integer register. In the
test case, with GS cookie stress, the GS cookie code asserted
that initReg was an integer register, but it wasn't.

The fix is the change the condition to use the actual register
assigned to the variable (in this case, xmm0), not the variable
type (here, TYP_STRUCT).

No spmi asm diffs.

Fixes #57911

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

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

Issue Details

If the only local is an enregistered must-init struct,
we were setting initReg to its register (in this case, xmm0).
However, initReg is expected to be an integer register. In the
test case, with GS cookie stress, the GS cookie code asserted
that initReg was an integer register, but it wasn't.

The fix is the change the condition to use the actual register
assigned to the variable (in this case, xmm0), not the variable
type (here, TYP_STRUCT).

No spmi asm diffs.

Fixes #57911

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall
Copy link
Member Author

@sandreenko This looks like fallout from enregistered struct locals. Is the condition appropriate, or is there some other standard condition that should be used? PTAL.

Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM, I think the fix is correct.
Looks like LclVarDsc::IsFloatRegType is not prepared for enreg structs, we can change the function or we can delete its usage, there are only 3 callers, 1 you have fixed here, the other 2 are:

// Is this a floating-point argument?
if (varDsc->IsFloatRegType())

(varDsc->IsFloatRegType() || !isFloatReg) && (varDsc->lvSlotNum < info.compVarScopesCount))

could you please look at them as well?

@BruceForstall
Copy link
Member Author

How about usage of varTypeUsesFloatReg when applied to LclVarDsc::TypeGet()? Is that also problematic? If we have LclVarDsc vars with TYP_STRUCT, we can't tell whether they use integer registers or floating-point registers just by looking at their type?

@sandreenko
Copy link
Contributor

How about usage of varTypeUsesFloatReg when applied to LclVarDsc::TypeGet()? Is that also problematic? If we have LclVarDsc vars with TYP_STRUCT, we can't tell whether they use integer registers or floating-point registers just by looking at their type?

You need to use LclVarDsc->GetRegisterType() instead if TypeGet().

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall
Copy link
Member Author

@sandreenko Can you look at the additional changes in 16a9f78?

If the only local is an enregistered must-init struct,
we were setting `initReg` to its register (in this case, `xmm0`).
However, `initReg` is expected to be an integer register. In the
test case, with GS cookie stress, the GS cookie code asserted
that `initReg` was an integer register, but it wasn't.

The fix is the change the condition to use the actual register
assigned to the variable (in this case, `xmm0`), not the variable
type (here, `TYP_STRUCT`).

No spmi asm diffs.

Fixes dotnet#57911
@BruceForstall
Copy link
Member Author

cc @dotnet/jit-contrib

@BruceForstall BruceForstall merged commit c0fe589 into dotnet:main Sep 9, 2021
@BruceForstall BruceForstall deleted the Fix57911 branch September 9, 2021 04:48
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] Assertion failed '!genIsValidFloatReg(reg) Generate code'
3 participants