-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Localsinit #24723
Localsinit #24723
Changes from 4 commits
339bac2
1712070
cd07338
dc64263
665e690
ecb278d
bd4f01b
3c1f7d7
a6d98d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,6 +166,8 @@ public void EnsureMetadataVirtual() | |
|
||
private OverriddenOrHiddenMembersResult _lazyOverriddenOrHiddenMembers; | ||
|
||
protected bool _localsAreZeroed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protected members should not begin with an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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; | ||
|
@@ -358,6 +361,14 @@ public override Symbol AssociatedSymbol | |
} | ||
} | ||
|
||
public override bool LocalsAreZeroed | ||
{ | ||
get | ||
{ | ||
return _localsAreZeroed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why is it safe to return this value? Are we certain attributes have been bound and processed? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
} | ||
} | ||
|
||
#region Flags | ||
|
||
public override bool ReturnsVoid | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not set this directly to false here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have a point. I don't know why I did that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It feels like we should follow the pattern for other well-known attributes and keep this information in attribute data. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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); | ||
|
||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
|
@@ -72,6 +74,7 @@ public MethodBody( | |
_methodId = methodId; | ||
_locals = locals; | ||
_exceptionHandlers = exceptionHandlers; | ||
_localsAreZeroed = localsAreZeroed; | ||
_localScopes = localScopes; | ||
_hasDynamicLocalVariables = hasDynamicLocalVariables; | ||
_importScopeOpt = importScopeOpt; | ||
|
@@ -102,7 +105,7 @@ public MethodBody( | |
|
||
ImmutableArray<Cci.ExceptionHandlerRegion> Cci.IMethodBody.ExceptionRegions => _exceptionHandlers; | ||
|
||
bool Cci.IMethodBody.LocalsAreZeroed => true; | ||
bool Cci.IMethodBody.LocalsAreZeroed => _localsAreZeroed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Are we planning to add recognition of the attribute to VB as well? #Pending There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -1610,6 +1610,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |
builder.RealizedSequencePoints, | ||
debugDocumentProvider, | ||
builder.RealizedExceptionHandlers, | ||
localsAreZeroed:=True, | ||
localScopes, | ||
hasDynamicLocalVariables:=False, | ||
importScopeOpt:=importScopeOpt, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding virtual properties at the root is bug prone. We leant to always add abstract properties and have each derived type implement it and then have targeted tests to cover each implementation. I think you'd be better off with the cast in MethodCompiler, otherwise you will need to add and test implementations for retargeted methods, PE methods, etc. Does that worth it? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can explicitly throw for all symbols for which we don't expect to call this API. Again this requires this method to have no default implementation. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we keep the virtual property and throw if the default implementation is called? Then only the method in the derived class could be used. I feel like it has the same result with less effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with having the property be virtual is future derivations of MethodSymbol. A year from now if a new derivation of MethodSymbol is added the developer may or may not know to re-evaluate the behavior of LocalsAreZeroed in the context of the type. Whether they do or don't comes down to the dillegence of the developer, and code reviewers, involved in the change. By making it abstract though we force the developer to think about this property and make a decision on its value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, it is an option to throw from default implementation in this particular situation (for non public API used in very specific place and time in the pipeline), then, assuming that we have adequate test coverage, tests should catch derived classes that fail to override the method. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I set it to abstract. We're returning true in some derived classes for now (mostly because of synthesized members).