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

Add the exception set for ObjGetType #64106

Merged
merged 2 commits into from
Jan 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 5 additions & 13 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5250,7 +5250,7 @@ bool GenTree::OperRequiresCallFlag(Compiler* comp)
// Return Value:
// True if the given node contains an implicit indirection
//
// Note that for the GT_HWINTRINSIC node we have to examine the
// Note that for the [HW]INTRINSIC nodes we have to examine the
// details of the node to determine its result.
//

Expand All @@ -5275,6 +5275,8 @@ bool GenTree::OperIsImplicitIndir() const
case GT_ARR_ELEM:
case GT_ARR_OFFSET:
return true;
case GT_INTRINSIC:
return AsIntrinsic()->gtIntrinsicName == NI_System_Object_GetType;
#ifdef FEATURE_SIMD
case GT_SIMD:
{
Expand Down Expand Up @@ -5331,18 +5333,8 @@ bool GenTree::OperMayThrow(Compiler* comp)

case GT_INTRINSIC:
// If this is an intrinsic that represents the object.GetType(), it can throw an NullReferenceException.
// Report it as may throw.
// Note: Some of the rest of the existing intrinsics could potentially throw an exception (for example
// the array and string element access ones). They are handled differently than the GetType intrinsic
// and are not marked with GTF_EXCEPT. If these are revisited at some point to be marked as
// GTF_EXCEPT,
// the code below might need to be specialized to handle them properly.
if ((this->gtFlags & GTF_EXCEPT) != 0)
{
return true;
}

break;
// Currently, this is the only intrinsic that can throw an exception.
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand from the comment is at least object.GetType() are marked with GTF_EXCEPT (if not rest of the existing intrinsics)? Then why not keep GTF_EXCEPT check?

Copy link
Contributor Author

@SingleAccretion SingleAccretion Jan 21, 2022

Choose a reason for hiding this comment

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

The predicate that OperMayThrow implements is "does this node induce a new exception by itself". The GTF_EXCEPT check that was here before was a workaround for the lack of precise handling of intrinsics - it would return true for intrinsics (such as all the math ones) that themselves don't throw, but happened to have children that are throwing.

Additionally, bringing the check back would trigger the new assert in fgValueNumberAddExceptionSet that only treats the GetType intrinsic as possibly throwing.

(Note that the part about ...the array and string element access ones... is stale - these are handled directly by importation and do not use the intrinsic nodes)

return AsIntrinsic()->gtIntrinsicName == NI_System_Object_GetType;

case GT_CALL:

Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/namedintrinsiclist.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
#ifndef _NAMEDINTRINSICLIST_H_
#define _NAMEDINTRINSICLIST_H_

// Named jit intrinsics
// Named jit intrinsics.

// When adding a new intrinsic that will use the GT_INTRINSIC node and can throw, make sure
// to update the "OperMayThrow" and "fgValueNumberAddExceptionSet" methods to account for that.

enum NamedIntrinsic : unsigned short
{
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10979,7 +10979,8 @@ void Compiler::fgValueNumberAddExceptionSet(GenTree* tree)
break;

case GT_INTRINSIC:
// ToDo: model the exceptions for Intrinsics
assert(tree->AsIntrinsic()->gtIntrinsicName == NI_System_Object_GetType);
fgValueNumberAddExceptionSetForIndirection(tree, tree->AsIntrinsic()->gtGetOp1());
break;

case GT_IND:
Expand Down
37 changes: 37 additions & 0 deletions src/tests/JIT/opt/ValueNumbering/ExceptionSets.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// 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.Runtime.CompilerServices;

#pragma warning disable CS0253 // Possible unintended reference comparison

class ExceptionSets
{
public static int Main()
{
try
{
TestObjGetType(null, 0);
return 101;
}
catch (NullReferenceException) { }

return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static bool TestObjGetType(object a, int i)
{
var fls = false;
var c1 = i == 0;
var c2 = c1;

if (((a.GetType() == a) & fls) | (i == 0))
{
return true;
}

return c2;
}
}
13 changes: 13 additions & 0 deletions src/tests/JIT/opt/ValueNumbering/ExceptionSets.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<CLRTestPriority>1</CLRTestPriority>
Copy link
Member

Choose a reason for hiding this comment

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

is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary per se, I just don't think the tests (there will be more) here are valuable enough to be Pri0. The reason being that they're protecting against bugs we have had for a very long time now, and haven't seen anyone hit them.

</PropertyGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>