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

[RISC-V] Fix struct info value in crossgen2 #99602

Closed
wants to merge 2 commits into from

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Mar 12, 2024

Fixed assertions in fgMorphMultiregStructArg when crossgen2 compiles in Debug build with --verify-type-and-field-layout option

Error messages

/runtime/src/coreclr/jit/morph.cpp:3766
Assertion failed 'roundUp(structSize, TARGET_POINTER_SIZE) == roundUp(loadExtent, TARGET_POINTER_SIZE)' in ...
  • System.Private.CoreLib.dll: ValueType which contains two double fields
  • ./Interop/StructMarshalling/PInvoke/MarshalStructAsLayoutExp/MarshalStructAsLayoutExp: ValueType which contains FieldOffset.

Part of #84834
cc @dotnet/samsung

@clamp03 clamp03 self-assigned this Mar 12, 2024
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Mar 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 12, 2024
Asserts in `./Interop/StructMarshalling/PInvoke/MarshalStructAsLayoutExp/MarshalStructAsLayoutExp.sh`
Error message is
`Assertion failed 'roundUp(structSize, TARGET_POINTER_SIZE) == roundUp(loadExtent, TARGET_POINTER_SIZE)' in 'Managed:MarshalStructAsParam_AsExpByVal(int)' during 'Morph - Global' (IL size 2208; hash 0x9fd9734a; MinOpts)`
Copied missed codes of GetRiscV64PassStructInRegiste in vm to crossgen2
@clamp03 clamp03 marked this pull request as ready for review March 19, 2024 07:36
@clamp03
Copy link
Member Author

clamp03 commented Mar 19, 2024

Checked the same assertions in coreclr tests (with crossgened test dll) and fixed.
There are still other issues. These will be fixed in other PR.
@dotnet/samsung @jkotas Could you please review it?
@dotnet/loongarch64-contrib I think the same issue will be in LoongArch64 too.

@clamp03 clamp03 requested a review from jkotas March 19, 2024 11:48
@jkotas
Copy link
Member

jkotas commented Mar 19, 2024

Could you please highlight the equivalent code in MethodTable::GetRiscV64PassStructInRegisterFlags? Would it make sense to make the code structure of MethodTable::GetRiscV64PassStructInRegisterFlags to be more similar so that they are easier to compare?

@clamp03
Copy link
Member Author

clamp03 commented Mar 20, 2024

@jkotas
I referenced

if (pFieldStart->GetOffset() || !pFieldStart[1].GetOffset() || (pFieldStart[0].GetSize() > pFieldStart[1].GetOffset()))
{
goto _End_arg;
}

and
if ((GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable2) & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
{
if (pFieldStart[1].GetSize() == 4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (pFieldStart[1].GetSize() == 8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
}

Right. These looks so different. AndMethodTable::GetRiscV64PassStructInRegisterFlags is too complex.
I agree that it is good to make the code structure of MethodTable::GetRiscV64PassStructInRegisterFlags to be more similar to RISCV64PassStructInRegister:GetRISCV64PassStructInRegisterFlags and simpler.

@bartlomiejko Today, I am off. Could you refactor both codes to make to be more similar and simpler in your team (including this patch)? If then, I will close this PR. If no one in your team is available, I will handle from tomorrow. Thank you.

@tomeksowi
Copy link
Contributor

Could you refactor both codes to make to be more similar and simpler in your team (including this patch)?

I'll take this, I worked on these flags recently.

@clamp03
Copy link
Member Author

clamp03 commented Mar 20, 2024

@tomeksowi Thank you.

@clamp03 clamp03 closed this Mar 20, 2024
@tomeksowi
Copy link
Contributor

(...) make the code structure of MethodTable::GetRiscV64PassStructInRegisterFlags to be more similar to RISCV64PassStructInRegister:GetRISCV64PassStructInRegisterFlags and simpler

Speaking of simpler, I noticed TypeHandle::IsBlittable always returns true for native value types:

if (!IsTypeDesc())
{
// This is a simple type (not an array, ptr or byref) so if
// simply check to see if the type is blittable.
return AsMethodTable()->IsBlittable();
}
else if (AsTypeDesc()->IsNativeValueType())
{
return TRUE;
}

So given blittable types have the same layout as managed, I think we could remove the whole NativeLayoutInfo branch from MethodTable::GetRiscV64PassStructInRegisterFlags. If anyone sees a fault in this reasoning, let me know.

@jkotas
Copy link
Member

jkotas commented Mar 20, 2024

NativeValueType is unmanaged marshalled view of the managed type. For example, if you have struct MyStruct { [MarshalAs(UnmanagedType.Bool)] bool myField; }, the NativeValueType is going to be equivalent to struct MyStruct { int myField; }.

The NativeLayoutInfo branch is only needed for built-in runtime marshalling that we have been moving away from. If you were to remove the NativeLayoutInfo branch, you would be effectively forcing DisableRuntimeMarshalling(true). User code that depends on built-in runtime marshalling for interop would be broken some of the time.

@tomeksowi
Copy link
Contributor

OK, for the time being I'll keep support for NativeLayoutInfo.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants