diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index d8b41d831de6a..a9406c77825ff 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs @@ -271,8 +271,27 @@ public bool InitializeIsUnscopedRef(bool value) } /// - /// Holds infrequently accessed fields. See for an explanation. + /// This type is used to hold lazily-initialized fields that many methods will not need. We avoid creating it unless one of the fields is needed; + /// unfortunately, this means that we need to be careful of data races. The general pattern that we use is to check for a flag in . + /// If the flag for that field is set, and there was a positive result (ie, there are indeed custom attributes, or there is obsolete data), then it + /// is safe to rely on the data in the field. If the flag for a field is set but the result is empty (ie, there is no obsolete data), then we can be in + /// one of 3 scenarios: + /// + /// is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of + /// however it chooses. + /// is not null, and the backing field has been initialized to some empty value, such as + /// . In this case, again, no race has occurred, and the consuming code can simply trust the empty value. + /// is not null, and the backing field is uninitialized, either being , or is some + /// kind of sentinel value. In this case, a data race has occurred, and the consuming code must initialize the field to empty to bring it back + /// into scenario 2. + /// /// + /// + /// The initialization pattern for this type must follow the following pattern to make the safety guarantees above: + /// If the field initialization code determines that the backing field needs to be set to some non-empty value, it must first call , + /// set the backing field using an atomic operation, and then set the flag in . This ensures that the field is always set before the flag is set. + /// If this order is reversed, the consuming code may see the flag set, but the field not initialized, and incorrectly assume that there is no data. + /// private sealed class UncommonFields { public ParameterSymbol _lazyThisParameter; @@ -289,64 +308,64 @@ private sealed class UncommonFields public MethodSymbol _lazyExplicitClassOverride; } - private UncommonFields CreateUncommonFields() + private UncommonFields AccessUncommonFields() { - var retVal = new UncommonFields(); - if (!_packedFlags.IsObsoleteAttributePopulated) - { - retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized; - } + var retVal = _uncommonFields; + return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, createUncommonFields()); - if (!_packedFlags.IsUnmanagedCallersOnlyAttributePopulated) + UncommonFields createUncommonFields() { - retVal._lazyUnmanagedCallersOnlyAttributeData = UnmanagedCallersOnlyAttributeData.Uninitialized; - } - - // - // Do not set _lazyUseSiteDiagnostic !!!! - // - // "null" Indicates "no errors" or "unknown state", - // and we know which one of the states we have from IsUseSiteDiagnosticPopulated - // - // Setting _lazyUseSiteDiagnostic to a sentinel value here would introduce - // a number of extra states for various permutations of IsUseSiteDiagnosticPopulated, UncommonFields and _lazyUseSiteDiagnostic - // Some of them, in tight races, may lead to returning the sentinel as the diagnostics. - // + var retVal = new UncommonFields(); + if (!_packedFlags.IsObsoleteAttributePopulated) + { + retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized; + } - if (_packedFlags.IsCustomAttributesPopulated) - { - retVal._lazyCustomAttributes = ImmutableArray.Empty; - } + if (!_packedFlags.IsUnmanagedCallersOnlyAttributePopulated) + { + retVal._lazyUnmanagedCallersOnlyAttributeData = UnmanagedCallersOnlyAttributeData.Uninitialized; + } - if (_packedFlags.IsConditionalPopulated) - { - retVal._lazyConditionalAttributeSymbols = ImmutableArray.Empty; - } + // + // Do not set _lazyUseSiteDiagnostic !!!! + // + // "null" Indicates "no errors" or "unknown state", + // and we know which one of the states we have from IsUseSiteDiagnosticPopulated + // + // Setting _lazyUseSiteDiagnostic to a sentinel value here would introduce + // a number of extra states for various permutations of IsUseSiteDiagnosticPopulated, UncommonFields and _lazyUseSiteDiagnostic + // Some of them, in tight races, may lead to returning the sentinel as the diagnostics. + // + + if (_packedFlags.IsCustomAttributesPopulated) + { + retVal._lazyCustomAttributes = ImmutableArray.Empty; + } - if (_packedFlags.IsOverriddenOrHiddenMembersPopulated) - { - retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty; - } + if (_packedFlags.IsConditionalPopulated) + { + retVal._lazyConditionalAttributeSymbols = ImmutableArray.Empty; + } - if (_packedFlags.IsMemberNotNullPopulated) - { - retVal._lazyNotNullMembers = ImmutableArray.Empty; - retVal._lazyNotNullMembersWhenTrue = ImmutableArray.Empty; - retVal._lazyNotNullMembersWhenFalse = ImmutableArray.Empty; - } + if (_packedFlags.IsOverriddenOrHiddenMembersPopulated) + { + retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty; + } - if (_packedFlags.IsExplicitOverrideIsPopulated) - { - retVal._lazyExplicitClassOverride = null; - } + if (_packedFlags.IsMemberNotNullPopulated) + { + retVal._lazyNotNullMembers = ImmutableArray.Empty; + retVal._lazyNotNullMembersWhenTrue = ImmutableArray.Empty; + retVal._lazyNotNullMembersWhenFalse = ImmutableArray.Empty; + } - return retVal; - } + if (_packedFlags.IsExplicitOverrideIsPopulated) + { + retVal._lazyExplicitClassOverride = null; + } - private UncommonFields AccessUncommonFields() - { - var retVal = _uncommonFields; - return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, CreateUncommonFields()); + return retVal; + } } private readonly MethodDefinitionHandle _handle; diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs index 9151ef8c181a6..e278301f2a4c1 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs @@ -143,6 +143,28 @@ public void SetCustomAttributesPopulated() public readonly bool IsCustomAttributesPopulated => (_bits & IsCustomAttributesPopulatedBit) != 0; } + /// + /// This type is used to hold lazily-initialized fields that many properties will not need. We avoid creating it unless one of the fields is needed; + /// unfortunately, this means that we need to be careful of data races. The general pattern that we use is to check for a flag in . + /// If the flag for that field is set, and there was a positive result (ie, there are indeed custom attributes, or there is obsolete data), then it + /// is safe to rely on the data in the field. If the flag for a field is set but the result is empty (ie, there is no obsolete data), then we can be in + /// one of 3 scenarios: + /// + /// is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of + /// however it chooses. + /// is not null, and the backing field has been initialized to some empty value, such as + /// . In this case, again, no race has occurred, and the consuming code can simply trust the empty value. + /// is not null, and the backing field is uninitialized, either being , or is some + /// kind of sentinel value. In this case, a data race has occurred, and the consuming code must initialize the field to empty to bring it back + /// into scenario 2. + /// + /// + /// + /// The initialization pattern for this type must follow the following pattern to make the safety guarantees above: + /// If the field initialization code determines that the backing field needs to be set to some non-empty value, it must first call , + /// set the backing field using an atomic operation, and then set the flag in . This ensures that the field is always set before the flag is set. + /// If this order is reversed, the consuming code may see the flag set, but the field not initialized, and incorrectly assume that there is no data. + /// private sealed class UncommonFields { public ImmutableArray _lazyCustomAttributes; @@ -312,31 +334,31 @@ static bool anyUnexpectedRequiredModifiers(ParamInfo[] propertyParam } } - private UncommonFields CreateUncommonFields() + private UncommonFields AccessUncommonFields() { - var retVal = new UncommonFields(); - if (!_flags.IsObsoleteAttributePopulated) - { - retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized; - } + var retVal = _uncommonFields; + return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, createUncommonFields()); - if (!_flags.IsUseSiteDiagnosticPopulated) + UncommonFields createUncommonFields() { - retVal._lazyCachedUseSiteInfo = CachedUseSiteInfo.Uninitialized; - } + var retVal = new UncommonFields(); + if (!_flags.IsObsoleteAttributePopulated) + { + retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized; + } - if (_flags.IsCustomAttributesPopulated) - { - retVal._lazyCustomAttributes = ImmutableArray.Empty; - } + if (!_flags.IsUseSiteDiagnosticPopulated) + { + retVal._lazyCachedUseSiteInfo = CachedUseSiteInfo.Uninitialized; + } - return retVal; - } + if (_flags.IsCustomAttributesPopulated) + { + retVal._lazyCustomAttributes = ImmutableArray.Empty; + } - private UncommonFields AccessUncommonFields() - { - var retVal = _uncommonFields; - return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, CreateUncommonFields()); + return retVal; + } } private bool MustCallMethodsDirectlyCore()