Skip to content

Commit

Permalink
Mark certain compiler-created locals as aliased (dotnet#74665)
Browse files Browse the repository at this point in the history
* Add a test for the devirt case

* Fix the devirt case

* Add a test for the ctor case

* Fix the ctor case

* Add a test for the localloc case

This one is actually ok, but only because "gtNewFieldRef" is quirky.

* Fix the localloc case

* Add an assert to catch future violations

* Fix up "arglist"

* Add a more elaborate "newobj" test case

One might be tempted to say that a "newobj" temp isn't
really aliased, since it is atomically assigned by the
constructor call.

As this new test case shows, such is not the case - the
"this" address pointing to the local being constructed
can be captured and used to mutate it.
  • Loading branch information
SingleAccretion committed Sep 1, 2022
1 parent 81f8e64 commit ba32931
Show file tree
Hide file tree
Showing 5 changed files with 255 additions and 10 deletions.
40 changes: 30 additions & 10 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15612,6 +15612,11 @@ void Compiler::impImportBlockCode(BasicBlock* block)
compSuppressedZeroInit = true;
}

// The constructor may store "this", with subsequent code mutating the underlying local
// through the captured reference. To correctly spill the node we'll push onto the stack
// in such a case, we must mark the temp as potentially aliased.
lclDsc->lvHasLdAddrOp = true;

// Obtain the address of the temp
newObjThisPtr =
gtNewOperNode(GT_ADDR, TYP_BYREF, gtNewLclvNode(lclNum, lvaTable[lclNum].TypeGet()));
Expand Down Expand Up @@ -16621,6 +16626,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
stackallocAsLocal);
lvaTable[stackallocAsLocal].lvType = TYP_BLK;
lvaTable[stackallocAsLocal].lvExactSize = (unsigned)allocSize;
lvaTable[stackallocAsLocal].lvHasLdAddrOp = true;
lvaTable[stackallocAsLocal].lvIsUnsafeBuffer = true;
op1 = gtNewLclvNode(stackallocAsLocal, TYP_BLK);
op1 = gtNewOperNode(GT_ADDR, TYP_I_IMPL, op1);
Expand Down Expand Up @@ -20203,18 +20209,25 @@ void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo,
return;
}

GenTree* lclVarTree;

GenTree* lclVarTree;
const bool isAddressInLocal = impIsAddressInLocal(curArgVal, &lclVarTree);
if (isAddressInLocal && varTypeIsStruct(lclVarTree))
if (isAddressInLocal)
{
inlCurArgInfo->argIsByRefToStructLocal = true;
#ifdef FEATURE_SIMD
if (lvaTable[lclVarTree->AsLclVarCommon()->GetLclNum()].lvSIMDType)
LclVarDsc* varDsc = lvaGetDesc(lclVarTree->AsLclVarCommon());

if (varTypeIsStruct(lclVarTree))
{
pInlineInfo->hasSIMDTypeArgLocalOrReturn = true;
}
inlCurArgInfo->argIsByRefToStructLocal = true;
#ifdef FEATURE_SIMD
if (varDsc->lvSIMDType)
{
pInlineInfo->hasSIMDTypeArgLocalOrReturn = true;
}
#endif // FEATURE_SIMD
}

// Spilling code relies on correct aliasability annotations.
assert(varDsc->lvHasLdAddrOp || varDsc->IsAddressExposed());
}

if (curArgVal->gtFlags & GTF_ALL_EFFECT)
Expand Down Expand Up @@ -22001,6 +22014,8 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
//
// Ideally, we then inline the boxed method and and if it turns out not to modify
// the copy, we can undo the copy too.
GenTree* localCopyThis = nullptr;

if (requiresInstMethodTableArg)
{
// Perform a trial box removal and ask for the type handle tree that fed the box.
Expand All @@ -22014,7 +22029,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
// If that worked, turn the box into a copy to a local var
//
JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg));
GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY);
localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY);

if (localCopyThis != nullptr)
{
Expand Down Expand Up @@ -22055,7 +22070,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
else
{
JITDUMP("Found unboxed entry point, trying to simplify box to a local copy\n");
GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY);
localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY);

if (localCopyThis != nullptr)
{
Expand All @@ -22078,6 +22093,11 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,

if (optimizedTheBox)
{
assert(localCopyThis->OperIs(GT_ADDR));

// We may end up inlining this call, so the local copy must be marked as "aliased",
// making sure the inlinee importer will know when to spill references to its value.
lvaGetDesc(localCopyThis->AsUnOp()->gtOp1->AsLclVar())->lvHasLdAddrOp = true;

#if FEATURE_TAILCALL_OPT
if (call->IsImplicitTailCall())
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1378,6 +1378,7 @@ void Compiler::lvaInitVarArgsHandle(InitVarDscInfo* varDscInfo)
// Codegen will need it for x86 scope info.
varDsc->lvImplicitlyReferenced = 1;
#endif // TARGET_X86
varDsc->lvHasLdAddrOp = 1;

lvaSetVarDoNotEnregister(lvaVarargsHandleArg DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr));

Expand Down
210 changes: 210 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_74635/Runtime_74635.il
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

.assembly extern System.Runtime { }
.assembly extern System.Console { }

.assembly Runtime_74635.dll { }

#define TRUE "1"
#define FALSE "0"
#define THIS "0"

#define EXPECTED "0"
#define THRASHED "1"

.class Runtime_74635 extends [System.Runtime]System.Object
{
.method public static int32 Main()
{
.entrypoint

call bool .this::ProblemWithDevirtualization()
brtrue DEVIRT_FAILED

call bool .this::ProblemWithCtors()
brtrue CTORS_FAILED

call bool .this::ProblemWithLocAlloc()
brtrue LOCALLOC_FAILED

call bool .this::ProblemWithCapturingCtors()
brtrue CAPTURING_CTORS_FAILED

ldc.i4 100
ret

DEVIRT_FAILED:
ldc.i4 101
ret

CTORS_FAILED:
ldc.i4 102
ret

LOCALLOC_FAILED:
ldc.i4 103
ret

CAPTURING_CTORS_FAILED:
ldc.i4 104
ret
}

.method private static bool ProblemWithDevirtualization() noinlining
{
ldc.i4 EXPECTED
newobj instance void Struct::.ctor(int32)
box valuetype Struct
callvirt instance int32 Interface::ReturnSelf()
ldc.i4 EXPECTED
bne.un FAILED

ldc.i4 FALSE
ret

FAILED:
ldc.i4 TRUE
ret
}

.method private static bool ProblemWithCtors() noinlining
{
ldc.i4 EXPECTED
ldc.i4 THRASHED
newobj instance void Struct::.ctor(int32, int32)
pop

ldsfld int32 Struct::StaticValue
ldc.i4 EXPECTED
bne.un FAILED

ldc.i4 FALSE
ret

FAILED:
ldc.i4 TRUE
ret
}

.method private static bool ProblemWithLocAlloc() noinlining
{
sizeof valuetype Struct
localloc
call int32 .this::ReturnExpectedWithLocAlloc(void*)
ldc.i4 EXPECTED
bne.un FAILED

ldc.i4 FALSE
ret

FAILED:
ldc.i4 TRUE
ret
}

.method private static int32 ReturnExpectedWithLocAlloc(void* pLocAlloc)
{
ldarg pLocAlloc
ldc.i4 EXPECTED
stind.i4

ldarg pLocAlloc
ldfld int32 Struct::Value
ldarg pLocAlloc
ldc.i4 THRASHED
stind.i4
ret
}

.method private static bool ProblemWithCapturingCtors() noinlining
{
.locals (valuetype Closure closure)

ldc.i4 EXPECTED
ldloca closure
newobj instance void Struct::.ctor(int32, valuetype Closure&)
ldloc closure
ldfld valuetype Struct& Closure::StructRef
ldc.i4 THRASHED
stfld int32 Struct::Value
ldfld int32 Struct::Value
ldc.i4 EXPECTED
bne.un FAILED

ldc.i4 FALSE
ret

FAILED:
ldc.i4 TRUE
ret
}
}

.class interface Interface
{
.method public abstract virtual int32 ReturnSelf() { }
}

.class sealed sequential Struct extends [System.Runtime]System.ValueType implements Interface
{
.field public static int32 StaticValue

.field public int32 Value

.method public void .ctor(int32 val)
{
ldarg THIS
ldarg val
stfld int32 .this::Value

ret
}

.method public void .ctor(int32 expected, int32 thrashed)
{
ldarg THIS
ldarg expected
stfld int32 .this::Value

ldarg THIS
ldfld int32 .this::Value
ldarg THIS
ldarg thrashed
stind.i4
stsfld int32 .this::StaticValue

ret
}

.method public void .ctor(int32 val, valuetype Closure& closureRef) noinlining
{
ldarg THIS
ldarg val
stfld int32 .this::Value

ldarg closureRef
ldarg THIS
stfld valuetype Struct& Closure::StructRef

ret
}

.method public virtual int32 ReturnSelf()
{
ldarg THIS
ldfld int32 .this::Value
ldarg THIS
ldc.i4 THRASHED
stind.i4

ret
}
}

.class sealed Closure extends [System.Runtime]System.ValueType
{
.custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = (01 00 00 00)

.field public valuetype Struct& StructRef
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk.IL">
<PropertyGroup>
<OutputType>Exe</OutputType>
</PropertyGroup>
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).il" />
</ItemGroup>
</Project>
3 changes: 3 additions & 0 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -2685,6 +2685,9 @@
<ExcludeList Include = "$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_64125/Runtime_64125/**">
<Issue>needs triage</Issue>
</ExcludeList>
<ExcludeList Include = "$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_74635/Runtime_74635/**">
<Issue>https://github.com/dotnet/runtime/issues/74687</Issue>
</ExcludeList>
<!-- End interpreter issues -->
</ItemGroup>

Expand Down

0 comments on commit ba32931

Please sign in to comment.