From 0c81586da844b6949916e3d04bcb1141bed2478c Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 27 Jun 2024 15:45:06 -0700 Subject: [PATCH 1/2] Address PR feedback on https://github.com/dotnet/roslyn/pull/74132 --- .../Symbols/Metadata/PE/PEMethodSymbol.cs | 117 ++++++++++-------- .../Symbols/Metadata/PE/PEPropertySymbol.cs | 60 ++++++--- 2 files changed, 109 insertions(+), 68 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index d8b41d831de6a..3faad1c2047cc 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 hold lazily-initialized fields that many method will not need. We avoid creating it unless one 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..abcf0086037d3 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 hold lazily-initialized fields that many properties will not need. We avoid creating it unless one 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() From b6e43d6bde00fb3253d59a0fe3fd7f1670f4a6f8 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 27 Jun 2024 16:34:06 -0700 Subject: [PATCH 2/2] Spelling --- .../CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs | 2 +- .../CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index 3faad1c2047cc..a9406c77825ff 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs @@ -271,7 +271,7 @@ public bool InitializeIsUnscopedRef(bool value) } /// - /// This type is used hold lazily-initialized fields that many method will not need. We avoid creating it unless one the fields is needed; + /// 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 diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs index abcf0086037d3..e278301f2a4c1 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs @@ -144,7 +144,7 @@ public void SetCustomAttributesPopulated() } /// - /// This type is used hold lazily-initialized fields that many properties will not need. We avoid creating it unless one the fields is needed; + /// 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