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

Make getTypeForPrimitiveValueClass treat different signs as different types #71633

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Make getTypeForPrimitiveValueClass treat different signs as different types #71633

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85030be
Make getTypeForPrimitiveValueClass treat different sizes as different…
MichalPetryka Jul 5, 2022
e888318
Handle missing types
MichalPetryka Jul 5, 2022
4a8b6cb
Update jitinterface.cpp
MichalPetryka Jul 5, 2022
eca0900
Test removing pointers
MichalPetryka Jul 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 9 additions & 50 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3969,54 +3969,13 @@ CorInfoType CEEInfo::getTypeForPrimitiveValueClass(
}
else
{
switch (th.GetInternalCorElementType())
CorElementType elementType = th.GetInternalCorElementType();
if (elementType >= ELEMENT_TYPE_BOOLEAN && elementType <= ELEMENT_TYPE_R8
Copy link
Member

@jkotas jkotas Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We have a helper method for this. I think it should be possible to simplify the whole thing into:

TypeHandle th(clsHnd);
_ASSERTE (!th.IsGenericVariable());

CorElementType elementType = th.GetVerifierCorElementType();
if (CorIsPrimitiveType(elementtype) || elementType == ELEMENT_TYPE_PTR || ELEMENT_TYPE_FNPTR)
{
    result = asCorInfoType(elementType);
}

It looks odd that this method is handling ELEMENT_TYPE_PTR and ELEMENT_TYPE_FNPTR. It does not match the name getTypeForPrimitiveValueClass.

Is the JIT really expecting this method to handle ELEMENT_TYPE_PTR and ELEMENT_TYPE_FNPTR?

Copy link
Contributor Author

@MichalPetryka MichalPetryka Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is the JIT really expecting this method to handle ELEMENT_TYPE_PTR and ELEMENT_TYPE_FNPTR?

Yes, for example in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L4203

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CorIsPrimitiveType would also include ELEMENT_TYPE_END, would that be okay?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for example in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L4203

I do not see the connection between this line and the method being changed in this PR. Could you please shed some light on it?

CorIsPrimitiveType would also include ELEMENT_TYPE_END, would that be okay?

Yes, that' ok. ELEMENT_TYPE_END is never returned by as CorElementType of a type.

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 do not see the connection between this line and the method being changed in this PR. Could you please shed some light on it?

Right, wrong line, here: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L6586

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'll try it after the CI finishes, want to see if everything's okay with the current changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkotas Could you take a look if the musl failure that appeared after removing the pointers looks related?

Copy link
Member

Choose a reason for hiding this comment

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

The failure looks like the same as #66413 (comment) which might be related to #71439. Does not look related to your change.

Copy link
Member

Choose a reason for hiding this comment

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

The failure is unrelated and failed in other PRs - #66413 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I have opened blocking-clean-in issue #71684 on this.

When merging on red, failures should be always tracked by active blocking-clean-in issue.

|| elementType == ELEMENT_TYPE_I || elementType == ELEMENT_TYPE_U
|| elementType == ELEMENT_TYPE_PTR || elementType == ELEMENT_TYPE_FNPTR
|| elementType == ELEMENT_TYPE_VOID)
{
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_U1:
case ELEMENT_TYPE_BOOLEAN:
result = asCorInfoType(ELEMENT_TYPE_I1);
break;

case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U2:
case ELEMENT_TYPE_CHAR:
result = asCorInfoType(ELEMENT_TYPE_I2);
break;

case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
result = asCorInfoType(ELEMENT_TYPE_I4);
break;

case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
result = asCorInfoType(ELEMENT_TYPE_I8);
break;

case ELEMENT_TYPE_I:
case ELEMENT_TYPE_U:
result = asCorInfoType(ELEMENT_TYPE_I);
break;

case ELEMENT_TYPE_R4:
result = asCorInfoType(ELEMENT_TYPE_R4);
break;

case ELEMENT_TYPE_R8:
result = asCorInfoType(ELEMENT_TYPE_R8);
break;

case ELEMENT_TYPE_VOID:
result = asCorInfoType(ELEMENT_TYPE_VOID);
break;

case ELEMENT_TYPE_PTR:
case ELEMENT_TYPE_FNPTR:
result = asCorInfoType(ELEMENT_TYPE_PTR);
break;

default:
break;
result = asCorInfoType(elementType);
}
}

Expand Down Expand Up @@ -13938,7 +13897,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
{
DWORD dwBlobSize = CorSigUncompressData(pBlob);
const uint8_t *const pBlobStart = pBlob;
pBlob += dwBlobSize;
pBlob += dwBlobSize;
StackSArray<TypeHandle> types;
DWORD cTypes = CorSigUncompressData(pBlob);
bool fail = false;
Expand All @@ -13965,7 +13924,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
}

MethodDesc *pMDCompare = NULL;

if (!fail)
{
if (kind == ENCODE_CHECK_IL_BODY)
Expand Down Expand Up @@ -13995,7 +13954,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,

fail = fail || (pMethodMetadata->cByteData != dwBlobSize);
}

if (!fail)
{
fail = 0 != memcmp(pBlobStart, pMethodMetadata->pByteData, dwBlobSize);
Expand Down
45 changes: 45 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_71632/Runtime_71632.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Numerics;
using System.Runtime.CompilerServices;

public class Runtime_71632
{
public static int Main()
{
try
{
Problem(1);
return 101;
}
catch (InvalidCastException) {}

try
{
Problem2(1);
return 102;
}
catch (InvalidCastException) {}

return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static sbyte Problem(byte a)
{
object box = a;
Use(ref box);
return (sbyte)box;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static sbyte Problem2(byte a)
{
object box = a;
return (sbyte)box;
}

static void Use<T>(ref T arg) { }
}
9 changes: 9 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_71632/Runtime_71632.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>