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

Localsinit #24723

Merged
merged 9 commits into from
Feb 22, 2018
Merged

Localsinit #24723

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
339bac2
SkipLocalsInitAttribute recognized as a WellKnownAttribute and proper…
t-camaia Feb 6, 2018
1712070
ILVisualizer locals init depends on LocalsAreZeroed flag
t-camaia Feb 8, 2018
cd07338
SkipLocalsInitAttribute on method tests added
t-camaia Feb 8, 2018
dc64263
LocalsAreZeroed passed as parameter instead of methodBody. Virtual Lo…
t-camaia Feb 9, 2018
665e690
LocalsAreZeroed renamed to AreLocalsZeroed. SourceMemberMethodSymbol.…
t-camaia Feb 12, 2018
ecb278d
AreLocalsZeroed in MethodSymbol changed from 'virtual' to 'abstract'
t-camaia Feb 14, 2018
bd4f01b
SerializeMethodBody cannot use body from cache if they don't have the…
t-camaia Feb 14, 2018
3c1f7d7
Packages and tests updated. HashCode issue fixed.
t-camaia Feb 21, 2018
a6d98d0
MetadataWriter and tests updated
t-camaia Feb 21, 2018
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
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,7 +1346,7 @@ private static MethodBody GenerateMethodBody(
var localSlotManager = new LocalSlotManager(variableSlotAllocatorOpt);
var optimizations = compilation.Options.OptimizationLevel;

ILBuilder builder = new ILBuilder(moduleBuilder, localSlotManager, optimizations);
ILBuilder builder = new ILBuilder(moduleBuilder, localSlotManager, optimizations, (method as SourceMemberMethodSymbol)?.LocalsAreZeroed ?? true);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

(method as SourceMemberMethodSymbol)?.LocalsAreZeroed ?? true [](start = 94, length = 61)

It looks like this is not going to cover lambdas and local functions. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our main objective right now is making sure that the attribute works on the method where it's directly applied. In future PRs we'll propagate it to nested lambdas and local functions.

DiagnosticBag diagnosticsForThisMethod = DiagnosticBag.GetInstance();
try
{
Expand Down Expand Up @@ -1460,6 +1460,7 @@ private static MethodBody GenerateMethodBody(
builder.RealizedSequencePoints,
debugDocumentProvider,
builder.RealizedExceptionHandlers,
builder.LocalsAreZeroed,
builder.GetAllScopes(),
builder.HasDynamicLocal,
importScopeOpt,
Expand Down
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ public void EnsureMetadataVirtual()

private OverriddenOrHiddenMembersResult _lazyOverriddenOrHiddenMembers;

protected bool _localsAreZeroed;
Copy link
Member

Choose a reason for hiding this comment

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

Protected members should not begin with an _. Really protected members should be Pascal cased but somehow other members here are camelCased. Lots of bad examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'll set it to private. I just realized there's no benefit in it being protected.


// some symbols may not have a syntax (e.g. lambdas, synthesized event accessors)
protected readonly SyntaxReference syntaxReferenceOpt;

Expand Down Expand Up @@ -202,6 +204,7 @@ protected SourceMemberMethodSymbol(NamedTypeSymbol containingType, SyntaxReferen
Debug.Assert(!locations.IsEmpty);

_containingType = containingType;
_localsAreZeroed = true;
this.syntaxReferenceOpt = syntaxReferenceOpt;
this.bodySyntaxReferenceOpt = bodySyntaxReferenceOpt;
this.locations = locations;
Expand Down Expand Up @@ -358,6 +361,14 @@ public override Symbol AssociatedSymbol
}
}

public bool LocalsAreZeroed
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be virtual on MethodSymbol and just return "true" in the default implementation. Then you do not need to type-check in MethodCompiler.
You will need this anyways when dealing with synthesized members.

Copy link
Member

Choose a reason for hiding this comment

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

Consider the name: AreLocalsZeroed. The rationale here is that we should try to prefix boolean member names with a true / false word: skip, is, are, etc ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All occurrences of LocalsAreZeroed were changed to AreLocalsZeroed.

{
get
{
return _localsAreZeroed;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

_localsAreZeroed [](start = 23, length = 16)

Why is it safe to return this value? Are we certain attributes have been bound and processed? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
}

#region Flags

public override bool ReturnsVoid
Expand Down Expand Up @@ -1128,6 +1139,10 @@ private void DecodeWellKnownAttributeAppliedToMethod(ref DecodeWellKnownAttribut
arguments.Diagnostics.Add(ErrorCode.ERR_SecurityCriticalOrSecuritySafeCriticalOnAsync, arguments.AttributeSyntaxOpt.Location, arguments.AttributeSyntaxOpt.GetErrorDisplayName());
}
}
else if (attribute.IsTargetAttribute(this, AttributeDescription.SkipLocalsInitAttribute))
{
_localsAreZeroed &= false;
Copy link
Member

Choose a reason for hiding this comment

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

Why not set this directly to false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point. I don't know why I did that.

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

_localsAreZeroed &= false; [](start = 16, length = 26)

It feels like we should follow the pattern for other well-known attributes and keep this information in attribute data. #Closed

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've made some changes. I followed the pattern from other simple boolean attributes like DynamicSecurityMethodAttribute and SuppressUnmanagedCodeSecurityAttribute.

}
else
{
var compilation = this.DeclaringCompilation;
Expand Down
99 changes: 99 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8559,6 +8559,105 @@ public static int Main()

#endregion

#region SkipLocalsInitAttribute

[Fact]
public void SkipLocalsInitOnMethod()
{
var source = @"
namespace System.Runtime.CompilerServices
{
public class SkipLocalsInitAttribute : System.Attribute
{
}
}

public class C
{
[System.Runtime.CompilerServices.SkipLocalsInitAttribute]
public void M_skip()
{
int x = 2;
x = x + x + x;
}

public void M_init()
{
int x = 2;
x = x + x + x;
}
}
";

var comp = CompileAndVerify(source, verify: Verification.Fails);

comp.VerifyIL("C.M_init", @"
{
// Code size 9 (0x9)
.maxstack 2
.locals init (int V_0) //x
IL_0000: ldc.i4.2
IL_0001: stloc.0
IL_0002: ldloc.0
IL_0003: ldloc.0
IL_0004: add
IL_0005: ldloc.0
IL_0006: add
IL_0007: stloc.0
IL_0008: ret
}", realIL: true);

comp.VerifyIL("C.M_skip", @"
{
// Code size 9 (0x9)
.maxstack 2
.locals (int V_0) //x
IL_0000: ldc.i4.2
IL_0001: stloc.0
IL_0002: ldloc.0
IL_0003: ldloc.0
IL_0004: add
IL_0005: ldloc.0
IL_0006: add
IL_0007: stloc.0
IL_0008: ret
}", realIL: true);

comp.VerifyIL("C.M_init", @"
{
// Code size 9 (0x9)
.maxstack 2
.locals init (int V_0) //x
IL_0000: ldc.i4.2
IL_0001: stloc.0
IL_0002: ldloc.0
IL_0003: ldloc.0
IL_0004: add
IL_0005: ldloc.0
IL_0006: add
IL_0007: stloc.0
IL_0008: ret
}", realIL: false);

comp.VerifyIL("C.M_skip", @"
{
// Code size 9 (0x9)
.maxstack 2
.locals (int V_0) //x
IL_0000: ldc.i4.2
IL_0001: stloc.0
IL_0002: ldloc.0
IL_0003: ldloc.0
IL_0004: add
IL_0005: ldloc.0
IL_0006: add
IL_0007: stloc.0
IL_0008: ret
}", realIL: false);
}

#endregion

[Fact, WorkItem(807, "https://github.com/dotnet/roslyn/issues/807")]
public void TestAttributePropagationForAsyncAndIterators_01()
{
Expand Down
6 changes: 5 additions & 1 deletion src/Compilers/Core/Portable/CodeGen/ILBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ internal sealed partial class ILBuilder
private SyntaxTree _lastSeqPointTree;

private readonly SmallDictionary<object, LabelInfo> _labelInfos;
private readonly bool _localsAreZeroed;
private int _instructionCountAtLastLabel = -1;

// This data is only relevant when builder has been realized.
Expand Down Expand Up @@ -62,7 +63,7 @@ internal sealed partial class ILBuilder
// created, in particular for leader blocks in exception handlers.
private bool _pendingBlockCreate;

internal ILBuilder(ITokenDeferral module, LocalSlotManager localSlotManager, OptimizationLevel optimizations)
internal ILBuilder(ITokenDeferral module, LocalSlotManager localSlotManager, OptimizationLevel optimizations, bool localsAreZeroed)
{
Debug.Assert(BitConverter.IsLittleEndian);

Expand All @@ -75,8 +76,11 @@ internal ILBuilder(ITokenDeferral module, LocalSlotManager localSlotManager, Opt

_labelInfos = new SmallDictionary<object, LabelInfo>(ReferenceEqualityComparer.Instance);
_optimizations = optimizations;
_localsAreZeroed = localsAreZeroed;
}

public bool LocalsAreZeroed => _localsAreZeroed;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

public bool LocalsAreZeroed => _localsAreZeroed; [](start = 8, length = 48)

It looks like it is not necessary to have this property on ILBuilder or have it inside ILBuilder because ILBuilder itself doen't take any advantage of it. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, it looks like ILBuilderToString is using this property.

In reply to: 167670235 [](ancestors = 167670235)


private BasicBlock GetCurrentBlock()
{
Debug.Assert(!_pendingBlockCreate || (_currentBlock == null));
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/Core/Portable/CodeGen/MethodBody.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ internal sealed class MethodBody : Cci.IMethodBody
private readonly ushort _maxStack;
private readonly ImmutableArray<Cci.ILocalDefinition> _locals;
private readonly ImmutableArray<Cci.ExceptionHandlerRegion> _exceptionHandlers;
private readonly bool _localsAreZeroed;

// Debug information emitted to Release & Debug PDBs supporting the debugger, EEs and other tools:
private readonly ImmutableArray<Cci.SequencePoint> _sequencePoints;
Expand Down Expand Up @@ -50,6 +51,7 @@ public MethodBody(
SequencePointList sequencePoints,
DebugDocumentProvider debugDocumentProvider,
ImmutableArray<Cci.ExceptionHandlerRegion> exceptionHandlers,
bool localsAreZeroed,
ImmutableArray<Cci.LocalScope> localScopes,
bool hasDynamicLocalVariables,
Cci.IImportScope importScopeOpt,
Expand All @@ -72,6 +74,7 @@ public MethodBody(
_methodId = methodId;
_locals = locals;
_exceptionHandlers = exceptionHandlers;
_localsAreZeroed = localsAreZeroed;
_localScopes = localScopes;
_hasDynamicLocalVariables = hasDynamicLocalVariables;
_importScopeOpt = importScopeOpt;
Expand Down Expand Up @@ -102,7 +105,7 @@ public MethodBody(

ImmutableArray<Cci.ExceptionHandlerRegion> Cci.IMethodBody.ExceptionRegions => _exceptionHandlers;

bool Cci.IMethodBody.LocalsAreZeroed => true;
bool Cci.IMethodBody.LocalsAreZeroed => _localsAreZeroed;
Copy link
Member

Choose a reason for hiding this comment

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

Now that this may vary, it should be another key in the method body cache. Just having same IL is not enough to say that methods may share the body.
Look at SerializeMethodBody

Copy link
Member

Choose a reason for hiding this comment

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

I wonder how this could be tested. We need to make sure the caching is not interfering.

Perhaps we can use code that relies on the zero init happening or not happening (probably using stackalloc or "out" parameters in VB), then we may just run bunch of similar methods that only differ in this bit and see if results are as expected?

In reply to: 167321124 [](ancestors = 167321124)

Copy link
Member

Choose a reason for hiding this comment

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

This is a good point, but I haven't tested to see whether it's likely that you get non-zero memory when running stackalloc. Our tests would have a dependency on having non-zero random memory, wouldn't they?

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 was talking to @agocke about this. We use a body from the cache if we can guarantee that localSignatureHandleOpt.IsNil returns true. localSignatureHandleOpt is obtained from SerializeLocalVariablesSignature, which seems to return a default object only if there are no locals. Maybe adding another check is not necessary. I still don't know how to test it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AreLocalsZeroed bool is now being used during IL mapping in cache. A test was added to check if method bodies that differ only by SkipLocalsInitAttribute have different Relative Virtual Addresses. The test was failing before the changes in the cache mapping and it's working now.


ImmutableArray<Cci.ILocalDefinition> Cci.IMethodBody.LocalVariables => _locals;

Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/Core/Portable/Symbols/Attributes/AttributeDescription.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ static AttributeDescription()

private static readonly byte[][] s_signaturesOfExperimentalAttribute = { s_signature_HasThis_Void };
private static readonly byte[][] s_signaturesOfExcludeFromCodeCoverageAttribute = { s_signature_HasThis_Void };
private static readonly byte[][] s_signaturesOfSkipLocalsInitAttribute = { s_signature_HasThis_Void };

// early decoded attributes:
internal static readonly AttributeDescription OptionalAttribute = new AttributeDescription("System.Runtime.InteropServices", "OptionalAttribute", s_signaturesOfOptionalAttribute);
Expand Down Expand Up @@ -515,5 +516,6 @@ static AttributeDescription()
internal static readonly AttributeDescription DeprecatedAttribute = new AttributeDescription("Windows.Foundation.Metadata", "DeprecatedAttribute", s_signaturesOfDeprecatedAttribute);
internal static readonly AttributeDescription ExperimentalAttribute = new AttributeDescription("Windows.Foundation.Metadata", "ExperimentalAttribute", s_signaturesOfExperimentalAttribute);
internal static readonly AttributeDescription ExcludeFromCodeCoverageAttribute = new AttributeDescription("System.Diagnostics.CodeAnalysis", "ExcludeFromCodeCoverageAttribute", s_signaturesOfExcludeFromCodeCoverageAttribute);
internal static readonly AttributeDescription SkipLocalsInitAttribute = new AttributeDescription("System.Runtime.CompilerServices", "SkipLocalsInitAttribute", s_signaturesOfSkipLocalsInitAttribute);
}
}
2 changes: 1 addition & 1 deletion src/Compilers/Test/Utilities/CSharp/BasicCompilationUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ protected override Compilation GetCompilationForEmit(IEnumerable<string> source,
throw new NotImplementedException();
}

internal override string VisualizeRealIL(IModuleSymbol peModule, CodeAnalysis.CodeGen.CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers)
internal override string VisualizeRealIL(IModuleSymbol peModule, CodeAnalysis.CodeGen.CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody)
{
throw new NotImplementedException();
}
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,9 +944,9 @@ internal static string GetDocumentationCommentText(CSharpCompilation compilation

#region IL Validation

internal override string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers)
internal override string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody)
{
return VisualizeRealIL((PEModuleSymbol)peModule, methodData, markers);
return VisualizeRealIL((PEModuleSymbol)peModule, methodData, markers, methodBody);
}

/// <summary>
Expand All @@ -959,7 +959,7 @@ internal override string VisualizeRealIL(IModuleSymbol peModule, CompilationTest
/// - winmd
/// - global methods
/// </remarks>
internal unsafe static string VisualizeRealIL(PEModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers)
internal unsafe static string VisualizeRealIL(PEModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Cci.IMethodBody methodBody [](start = 164, length = 26)

It feels like an overkill to require all consumers to pass an instance of Cci.IMethodBody just so we can get LocalsAreZeroed value. If this is all we need, we should simply have localsAreZeroed as a parameter. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

{
var typeName = GetContainingTypeMetadataName(methodData.Method);
// TODO (tomat): global methods (typeName == null)
Expand Down Expand Up @@ -1000,7 +1000,7 @@ internal unsafe static string VisualizeRealIL(PEModuleSymbol peModule, Compilati

var visualizer = new Visualizer(new MetadataDecoder(peModule, peMethod));

visualizer.DumpMethod(sb, maxStack, ilBytes, localDefinitions, ehHandlerRegions, markers);
visualizer.DumpMethod(sb, maxStack, ilBytes, localDefinitions, ehHandlerRegions, markers, methodBody.LocalsAreZeroed);

return sb.ToString();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Test/Utilities/VisualBasic/BasicTestBase.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ Public MustInherit Class BasicTestBaseBase
Return attributes.Select(Function(a) a.AttributeClass.Name)
End Function

Friend Overrides Function VisualizeRealIL(peModule As IModuleSymbol, methodData As CompilationTestData.MethodData, markers As IReadOnlyDictionary(Of Integer, String)) As String
Friend Overrides Function VisualizeRealIL(peModule As IModuleSymbol, methodData As CompilationTestData.MethodData, markers As IReadOnlyDictionary(Of Integer, String), methodBody As Cci.IMethodBody) As String
Throw New NotImplementedException()
End Function

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/VisualBasic/Portable/Compilation/MethodCompiler.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1507,7 +1507,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
optimizations = OptimizationLevel.Release
End If

Dim builder As ILBuilder = New ILBuilder(moduleBuilder, localSlotManager, optimizations)
Dim builder As ILBuilder = New ILBuilder(moduleBuilder, localSlotManager, optimizations, localsAreZeroed:=True)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

localsAreZeroed:=True) [](start = 101, length = 22)

Are we planning to add recognition of the attribute to VB as well? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only C#, as far as I know.


Try
Debug.Assert(Not diagnostics.HasAnyErrors)
Expand Down Expand Up @@ -1610,6 +1610,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
builder.RealizedSequencePoints,
debugDocumentProvider,
builder.RealizedExceptionHandlers,
localsAreZeroed:=True,
localScopes,
hasDynamicLocalVariables:=False,
importScopeOpt:=importScopeOpt,
Expand Down
2 changes: 1 addition & 1 deletion src/Test/Utilities/Portable/CommonTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ private void AddReferencedCompilations(IEnumerable<Compilation> referencedCompil

#region IL Verification

internal abstract string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers);
internal abstract string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing the method body, why not just pass the boolean for LocalsAreZeroed? Do we need the method body for anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point.


#endregion

Expand Down
6 changes: 3 additions & 3 deletions src/Test/Utilities/Portable/CompilationVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ public sealed class CompilationVerifier
public ImmutableArray<byte> EmittedAssemblyData;
public ImmutableArray<byte> EmittedAssemblyPdb;

private readonly Func<IModuleSymbol, CompilationTestData.MethodData, IReadOnlyDictionary<int, string>, string> _visualizeRealIL;
private readonly Func<IModuleSymbol, CompilationTestData.MethodData, IReadOnlyDictionary<int, string>, Cci.IMethodBody, string> _visualizeRealIL;

internal CompilationVerifier(
Compilation compilation,
Func<IModuleSymbol, CompilationTestData.MethodData, IReadOnlyDictionary<int, string>, string> visualizeRealIL = null,
Func<IModuleSymbol, CompilationTestData.MethodData, IReadOnlyDictionary<int, string>, Cci.IMethodBody, string> visualizeRealIL = null,
IEnumerable<ModuleData> dependencies = null)
{
_compilation = compilation;
Expand Down Expand Up @@ -222,7 +222,7 @@ internal string VisualizeIL(CompilationTestData.MethodData methodData, bool real
_lazyModuleSymbol = GetSymbolFromMetadata(targetReference, MetadataImportOptions.All);
}

return _lazyModuleSymbol != null ? _visualizeRealIL(_lazyModuleSymbol, methodData, markers) : null;
return _lazyModuleSymbol != null ? _visualizeRealIL(_lazyModuleSymbol, methodData, markers, _testData.Module.GetMethodBody(methodData.Method)) : null;
}

public CompilationVerifier VerifyMemberInIL(string methodName, bool expected)
Expand Down
2 changes: 1 addition & 1 deletion src/Test/Utilities/Portable/Metadata/ILBuilderVisualizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ internal static string ILBuilderToString(

if (!ilStream.IsDefault)
{
visualizer.DumpMethod(sb, builder.MaxStack, ilStream, locals, GetHandlerSpans(builder.RealizedExceptionHandlers), markers);
visualizer.DumpMethod(sb, builder.MaxStack, ilStream, locals, GetHandlerSpans(builder.RealizedExceptionHandlers), markers, builder.LocalsAreZeroed);
}
else
{
Expand Down