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

Spilling can overwrite adjacent stack parameters on ARM64 macOS ABI #67188

Closed
SingleAccretion opened this issue Mar 26, 2022 · 4 comments · Fixed by #67274
Closed

Spilling can overwrite adjacent stack parameters on ARM64 macOS ABI #67188

SingleAccretion opened this issue Mar 26, 2022 · 4 comments · Fixed by #67274
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug
Milestone

Comments

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Mar 26, 2022

Reproduction:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Problem(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7, short stk0, short stk1)
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Call(int k) => 0;

    stk1 = (short)(stk0 + stk1 + stk0);

    Call(stk1);

    return stk0 + stk1;
}

Compile with CG2: --targetarch:arm64 --targetos:OSX --codegenopt NgenDump=Problem --codegenopt JitNoCSE=1 --codegenopt JitStressRegs=2.

Expected result: spilling of the small stack parameters cannot overwrite anything.

Actual result: it can, due to using TYP_INT-wide stores:

IN0014: 000008  B94013A1          ldr     w1, [fp,#16]  // [V08 arg8]
IN0015: 00000C  B84123A2          ldr     w2, [fp,#18]  // [V09 arg9]

...

IN0008: 00002C  B90013A1          str     w1, [fp,#16]

Also see #67152.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Mar 26, 2022
@ghost
Copy link

ghost commented Mar 26, 2022

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

Issue Details

Reproduction:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Problem(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7, short stk0, short stk1)
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Call(int k) => 0;

    stk1 = (short)(stk0 + stk1 + stk0);

    Call(stk1);

    return stk0 + stk1;
}

Compile with CG2: --targetarch:arm64 --targetos:OSX --codegenopt NgenDump=Problem --codegenopt JitNoCSE=1 --codegenopt JitStressRegs=2.

Expected result: spilling of the small stack parameters cannot overwrite anything.

Actual result: it can, due to using TYP_INT-wide stores:

IN0014: 000008  B94013A1          ldr     w1, [fp,#16]  // [V08 arg8]
IN0015: 00000C  B84123A2          ldr     w2, [fp,#18]  // [V09 arg9]

...

IN0008: 00002C  B90013A1          str     w1, [fp,#16]

Essentially this is the same issue as in #67152.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@SingleAccretion SingleAccretion added this to the 7.0.0 milestone Mar 26, 2022
@am11
Copy link
Member

am11 commented Mar 26, 2022

Similar case #66720 (comment)

@sandreenko
Copy link
Contributor

@AndyAyersMS I can't comment in #52039 (comment) because it is locked but I think you are right.
The root case is that there are 2 scenarios:

  1. local lives in a register and due to register pressure we need to spill it and we allocate a new stack slot for it. This stack slot always is at least TARGET_POINTER_SIZE wide;
  2. local comes on a stack, gets temporary allocated to a register and then needs to be spilled. We don't need to allocate a new spill stack space because because we can reuse the stack index that it came on. However, on mac arm64 abi this stack space can be less than TARGET_POINTER_SIZE.

I would try a radical fix first in:

var_types LclVarDsc::GetActualRegisterType() const
{
return genActualType(GetRegisterType());
}

instead do:

var_types LclVarDsc::GetActualRegisterType() const
{
#ifdef OSX_ARM64_ABI
   if (lclVar->is argument stack slot) 
   {
       return GetRegisterType();
    }
#endif

    return genActualType(GetRegisterType());
}

@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Mar 28, 2022
@AndyAyersMS AndyAyersMS self-assigned this Mar 28, 2022
@AndyAyersMS
Copy link
Member

I'll fix this since OSR is seeing a related issue.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Mar 29, 2022
Fix two cases where small enregisterable locals can't be saved to the stack
using actual (widened) types:
* small memory args for OSX ARM64
* promoted fields of OSR locals

Closes dotnet#67152.
Closes dotnet#67188.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 29, 2022
AndyAyersMS added a commit that referenced this issue Mar 29, 2022
Fix two cases where small enregisterable locals can't be saved to the stack
using actual (widened) types:
* small memory args for OSX ARM64
* promoted fields of OSR locals

Closes #67152.
Closes #67188.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2022
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 bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants