Skip to content

Commit

Permalink
Emit correct code when demands of new features interfere with peverif…
Browse files Browse the repository at this point in the history 
…y-compat.
  • Loading branch information
@VSadov
VSadov committed Oct 19, 2017
1 parent 9a09a70 commit e7ac95a
Show file tree
Hide file tree
Showing 8 changed files with 254 additions and 120 deletions.
75 changes: 44 additions & 31 deletions src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@ private enum AddressKind

// reference itself will not be written to, nor it will be used to modify fields.
ReadOnly,

// same as ReadOnly, but we are not supposed to get a reference to a clone
// regardless of compat settings.
ReadOnlyStrict,
}

private static bool IsReadOnly(AddressKind addressKind) => addressKind >= AddressKind.ReadOnly;

/// <summary>
/// Emits address as in &amp;
///
Expand Down Expand Up @@ -70,7 +76,7 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr

case BoundKind.ThisReference:
Debug.Assert(expression.Type.IsValueType, "only value types may need a ref to this");
Debug.Assert(HasHome(expression, addressKind == AddressKind.Writeable));
Debug.Assert(HasHome(expression, addressKind));
_builder.EmitOpCode(ILOpCode.Ldarg_0);
break;

Expand Down Expand Up @@ -101,7 +107,7 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr
var methodRefKind = call.Method.RefKind;

if (methodRefKind == RefKind.Ref ||
(addressKind == AddressKind.ReadOnly && methodRefKind == RefKind.RefReadOnly))
(IsReadOnly(addressKind) && methodRefKind == RefKind.RefReadOnly))
{
EmitCallExpression(call, UseKind.UsedAsAddress);
break;
Expand All @@ -121,7 +127,7 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr

case BoundKind.ConditionalOperator:
var conditional = (BoundConditionalOperator)expression;
if (!HasHome(conditional, addressKind != AddressKind.ReadOnly))
if (!HasHome(conditional, addressKind))
{
goto default;
}
Expand All @@ -144,7 +150,7 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr
return null;

default:
Debug.Assert(!HasHome(expression, addressKind != AddressKind.ReadOnly));
Debug.Assert(!HasHome(expression, addressKind));
return EmitAddressOfTempClone(expression);
}

Expand Down Expand Up @@ -218,7 +224,7 @@ private LocalDefinition EmitLocalAddress(BoundLocal localAccess, AddressKind add
{
var local = localAccess.LocalSymbol;

if (!HasHome(localAccess, needWriteable: addressKind != AddressKind.ReadOnly))
if (!HasHome(localAccess, addressKind))
{
return EmitAddressOfTempClone(localAccess);
}
Expand Down Expand Up @@ -249,7 +255,7 @@ private LocalDefinition EmitLocalAddress(BoundLocal localAccess, AddressKind add
/// </summary>
private LocalDefinition EmitDupAddress(BoundDup dup, AddressKind addressKind)
{
if (!HasHome(dup, needWriteable: addressKind != AddressKind.ReadOnly))
if (!HasHome(dup, addressKind))
{
return EmitAddressOfTempClone(dup);
}
Expand Down Expand Up @@ -339,7 +345,7 @@ private LocalSymbol DigForValueLocal(BoundSequence topSequence, BoundExpression
/// Checks if expression directly or indirectly represents a value with its own home. In
/// such cases it is possible to get a reference without loading into a temporary.
/// </summary>
private bool HasHome(BoundExpression expression, bool needWriteable)
private bool HasHome(BoundExpression expression, AddressKind addressKind)
{
switch (expression.Kind)
{
Expand All @@ -352,50 +358,51 @@ private bool HasHome(BoundExpression expression, bool needWriteable)
case BoundKind.ThisReference:
Debug.Assert(expression.Type.IsValueType);

if (needWriteable && expression.Type.IsReadOnly)
if (!IsReadOnly(addressKind) && expression.Type.IsReadOnly)
{
return _method.MethodKind == MethodKind.Constructor;
}

return true;

case BoundKind.ThrowExpression:
// vacuously this is true, we can take address of throw without temps
return true;

case BoundKind.Parameter:
return !needWriteable ||
return IsReadOnly(addressKind) ||
((BoundParameter)expression).ParameterSymbol.RefKind != RefKind.In;

case BoundKind.Local:
// locals have home unless they are byval stack locals or ref-readonly
// locals in a mutating call
var local = ((BoundLocal)expression).LocalSymbol;
return !((IsStackLocal(local) && local.RefKind == RefKind.None) ||
(needWriteable && local.RefKind == RefKind.RefReadOnly));
(!IsReadOnly(addressKind) && local.RefKind == RefKind.RefReadOnly));

case BoundKind.Call:
var methodRefKind = ((BoundCall)expression).Method.RefKind;
return methodRefKind == RefKind.Ref ||
(!needWriteable && methodRefKind == RefKind.RefReadOnly);
(IsReadOnly(addressKind) && methodRefKind == RefKind.RefReadOnly);

case BoundKind.Dup:
//NB: Dup represents locals that do not need IL slot
var dupRefKind = ((BoundDup)expression).RefKind;
return dupRefKind == RefKind.Ref ||
(!needWriteable && dupRefKind == RefKind.RefReadOnly);
(IsReadOnly(addressKind) && dupRefKind == RefKind.RefReadOnly);

case BoundKind.FieldAccess:
return HasHome((BoundFieldAccess)expression, needWriteable);
return HasHome((BoundFieldAccess)expression, addressKind);

case BoundKind.Sequence:
return HasHome(((BoundSequence)expression).Value, needWriteable);
return HasHome(((BoundSequence)expression).Value, addressKind);

case BoundKind.AssignmentOperator:
return ((BoundAssignmentOperator)expression).RefKind != RefKind.None;

case BoundKind.ComplexConditionalReceiver:
Debug.Assert(HasHome(((BoundComplexConditionalReceiver)expression).ValueTypeReceiver, needWriteable));
Debug.Assert(HasHome(((BoundComplexConditionalReceiver)expression).ReferenceTypeReceiver, needWriteable));
Debug.Assert(HasHome(((BoundComplexConditionalReceiver)expression).ValueTypeReceiver, addressKind));
Debug.Assert(HasHome(((BoundComplexConditionalReceiver)expression).ReferenceTypeReceiver, addressKind));
goto case BoundKind.ConditionalReceiver;

case BoundKind.ConditionalReceiver:
Expand All @@ -415,7 +422,7 @@ private bool HasHome(BoundExpression expression, bool needWriteable)
// branch that has no home will need a temporary
// if both have no home, just say whole expression has no home
// so we could just use one temp for the whole thing
return HasHome(ternary.Consequence, needWriteable) && HasHome(ternary.Alternative, needWriteable);
return HasHome(ternary.Consequence, addressKind) && HasHome(ternary.Alternative, addressKind);

default:
return false;
Expand All @@ -427,7 +434,7 @@ private bool HasHome(BoundExpression expression, bool needWriteable)
/// Fields have readable homes when they are not constants.
/// Fields have writeable homes unless they are readonly and used outside of the constructor.
/// </summary>
private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
private bool HasHome(BoundFieldAccess fieldAccess, AddressKind addressKind)
{
FieldSymbol field = fieldAccess.FieldSymbol;

Expand All @@ -437,7 +444,14 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
return false;
}

if (!needWriteable && !EnablePEVerifyCompat())
// when reference is strictly required, consider fields as addressable
if (addressKind == AddressKind.ReadOnlyStrict)
{
return true;
}

// ReadOnly references can always be taken unless we are in peverify compat mode
if (addressKind == AddressKind.ReadOnly && !EnablePEVerifyCompat())
{
return true;
}
Expand All @@ -456,7 +470,7 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
// we would not be able to dig for the inner field using references and the outer struct will have to be copied to a temp anyways.
if (!EnablePEVerifyCompat())
{
Debug.Assert(needWriteable == true);
Debug.Assert(!IsReadOnly(addressKind));

var receiver = fieldAccess.ReceiverOpt;
if (receiver?.Type.IsValueType == true)
Expand All @@ -466,8 +480,8 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
// has readable home -> return false - we need to copy the field
// otherwise -> return true - the copy will be made at higher level so the leaf field can have writeable home

return HasHome(receiver, needWriteable: true) ||
!HasHome(receiver, needWriteable: false);
return HasHome(receiver, addressKind) ||
!HasHome(receiver, AddressKind.ReadOnly);
}
}

Expand Down Expand Up @@ -534,7 +548,7 @@ private LocalDefinition EmitFieldAddress(BoundFieldAccess fieldAccess, AddressKi
{
FieldSymbol field = fieldAccess.FieldSymbol;

if (!HasHome(fieldAccess, addressKind != AddressKind.ReadOnly))
if (!HasHome(fieldAccess, addressKind))
{
// accessing a field that is not writable (const or readonly)
return EmitAddressOfTempClone(fieldAccess);
Expand All @@ -546,11 +560,7 @@ private LocalDefinition EmitFieldAddress(BoundFieldAccess fieldAccess, AddressKi
}
else
{
//NOTE: we are not propagating AddressKind here.
// the reason is that while Constrained permits calls, it does not permit
// taking field addresses, so we have to turn Constrained into writeable.
// It is less error prone to just pass a bool "isReadonly"
return EmitInstanceFieldAddress(fieldAccess, isReadonly: addressKind == AddressKind.ReadOnly);
return EmitInstanceFieldAddress(fieldAccess, addressKind);
}
}

Expand All @@ -564,7 +574,7 @@ private LocalDefinition EmitParameterAddress(BoundParameter parameter, AddressKi
{
ParameterSymbol parameterSymbol = parameter.ParameterSymbol;

if (!HasHome(parameter, needWriteable: addressKind != AddressKind.ReadOnly))
if (!HasHome(parameter, addressKind))
{
// accessing a parameter that is not writable
return EmitAddressOfTempClone(parameter);
Expand Down Expand Up @@ -634,11 +644,14 @@ private LocalDefinition EmitReceiverRef(BoundExpression receiver, AddressKind ad
/// <summary>
/// May introduce a temp which it will return. (otherwise returns null)
/// </summary>
private LocalDefinition EmitInstanceFieldAddress(BoundFieldAccess fieldAccess, bool isReadonly)
private LocalDefinition EmitInstanceFieldAddress(BoundFieldAccess fieldAccess, AddressKind addressKind)
{
var field = fieldAccess.FieldSymbol;

var tempOpt = EmitReceiverRef(fieldAccess.ReceiverOpt, isReadonly? AddressKind.ReadOnly: AddressKind.Writeable);
//NOTE: we are not propagating AddressKind.Constrained here.
// the reason is that while Constrained permits calls, it does not permit
// taking field addresses, so we have to turn Constrained into writeable.
var tempOpt = EmitReceiverRef(fieldAccess.ReceiverOpt, addressKind == AddressKind.Constrained? AddressKind.Writeable: addressKind);

_builder.EmitOpCode(ILOpCode.Ldflda);
EmitSymbolToken(field, fieldAccess.Syntax);
Expand Down
Loading

0 comments on commit e7ac95a

Please sign in to comment.