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

[Mono AOT] Fix error when returning zero sized struct #106013

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Aug 6, 2024

This PR allows Mono AOT to handle returns of zero sized structs in registers.

Example:

public struct Foo
{
}

The [StructLayout(LayoutKind.Sequential, Size = 1)] attribute will be added during compilation SharpLab.

[StructLayout(LayoutKind.Sequential)] // or [StructLayout(LayoutKind.Auto)] 
public struct Foo
{
}

However, adding an explicit [StructLayout(LayoutKind.Sequential] or [StructLayout(LayoutKind.Auto)] will not generate any explicit size metadata causing the struct to have size 0 SharpLab.


Mono Codegen:

[StructLayout(LayoutKind.Sequential)]
public struct Foo
{
}

public class Bar
{
   public static Foo GetFoo() => new Foo();
}

When processing the return of the GetFoo() method mono will evaluate size of struct Foo to be equal to zero. Then it tries to pass it in registers, calculating that it needs 0 registers to do so.

nregs = align_size / 8;

Mono JIT handles this by omitting codegen part when cinfo->ret.nregs is equal to zero.

case ArgVtypeInIRegs: {
MonoInst *loc = cfg->arch.vret_addr_loc;
int i;
/* Load the destination address */
g_assert (loc && loc->opcode == OP_REGOFFSET);
code = emit_ldrx (code, ARMREG_LR, loc->inst_basereg, GTMREG_TO_INT (loc->inst_offset));
for (i = 0; i < cinfo->ret.nregs; ++i) {
int offs = i * 8;
// emit a pair of str as one stp
if (i+1 < cinfo->ret.nregs && IS_VALID_STPX_OFFSET (offs)) {
arm_stpx (code, cinfo->ret.reg + i, cinfo->ret.reg + i + 1, ARMREG_LR, offs);
i++;
} else {
arm_strx (code, cinfo->ret.reg + i, ARMREG_LR, offs);
}
}
break;

This PR proposes to fix the issue in Mono AOT by treating zero sized structs as if they have size 1.


Fixes #103628
Fixes #106259

@jkurdek
Copy link
Member Author

jkurdek commented Aug 8, 2024

/cc @vitek-karas

@lambdageek
Copy link
Member

This PR proposes to fix the issue in Mono AOT by treating zero sized structs as if they have size 1.

Won't this break scenarios where we might call an AOTed method from the JIT or vice-versa? Or does this not leak out into callers?

What happens if you set the ret type to void? do we get ill-formed LLVM IR because we end up trying to return a value?

@jkurdek
Copy link
Member Author

jkurdek commented Aug 8, 2024

Won't this break scenarios where we might call an AOTed method from the JIT or vice-versa? Or does this not leak out into callers?

I'm not sure I get it. The change is made only on llvm codegen level. The size of the struct is not overridden for any other calls. Without this change we would just encounter compile time error. Am I missing something?

I don't think void return type would even enter this code path. Will verify.

@jkurdek
Copy link
Member Author

jkurdek commented Aug 12, 2024

I have disabled the new test on x64 as it results in a new issue #106259

@jkurdek
Copy link
Member Author

jkurdek commented Aug 12, 2024

/azp run runtime-extra-platforms

@jkurdek jkurdek changed the title [Mono AOT] Fix error when returning zero sized struct [Mono AOT] [arm64] Fix error when returning zero sized struct Aug 12, 2024
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkurdek
Copy link
Member Author

jkurdek commented Aug 13, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkurdek jkurdek changed the title [Mono AOT] [arm64] Fix error when returning zero sized struct [Mono AOT] Fix error when returning zero sized struct Aug 13, 2024
@jkurdek
Copy link
Member Author

jkurdek commented Aug 13, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkurdek
Copy link
Member Author

jkurdek commented Aug 14, 2024

Failures seem to be unrelated.

@jkurdek jkurdek merged commit b0dbee9 into dotnet:main Aug 14, 2024
155 of 165 checks passed
@derjabkin
Copy link

Is this PR also going to be merged into version 8.0?

@vitek-karas
Copy link
Member

@jkurdek when you have some time, please open a backport, I'll try to get it through the triage. I think it should be doable.

@jkurdek
Copy link
Member Author

jkurdek commented Aug 30, 2024

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/10636307769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants