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

Jump to bottom

Address PR feedback on https://github.com/dotnet/roslyn/pull/74132 #74193

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Address PR feedback on https://github.com/dotnet/roslyn/pull/74132 #74193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c81586
Address PR feedback on https://github.com/dotnet/roslyn/pull/74132
333fred Jun 27, 2024
b6e43d6
Spelling
333fred Jun 27, 2024
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
117 changes: 68 additions & 49 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,27 @@ public bool InitializeIsUnscopedRef(bool value)
}

/// <summary>
/// Holds infrequently accessed fields. See <seealso cref="_uncommonFields"/> 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 <see cref="_packedFlags"/>.
/// 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:
/// <list type="number">
/// <item><see cref="_uncommonFields"/> is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of
/// <see cref="_uncommonFields"/> however it chooses.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field has been initialized to some empty value, such as
/// <see cref="ImmutableArray{T}.Empty"/>. In this case, again, no race has occurred, and the consuming code can simply trust the empty value.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field is uninitialized, either being <see langword="default" />, 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.</item>
/// </list>
/// </summary>
/// <remarks>
/// The initialization pattern for this type <b>must</b> 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 <b>must</b> first call <see cref="AccessUncommonFields"/>,
/// set the backing field using an atomic operation, and then set the flag in <see cref="_packedFlags"/>. 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.
/// </remarks>
private sealed class UncommonFields
{
public ParameterSymbol _lazyThisParameter;
Expand All @@ -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()
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
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<CSharpAttributeData>.Empty;
}
if (!_packedFlags.IsUnmanagedCallersOnlyAttributePopulated)
{
retVal._lazyUnmanagedCallersOnlyAttributeData = UnmanagedCallersOnlyAttributeData.Uninitialized;
}

if (_packedFlags.IsConditionalPopulated)
{
retVal._lazyConditionalAttributeSymbols = ImmutableArray<string>.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<CSharpAttributeData>.Empty;
}

if (_packedFlags.IsOverriddenOrHiddenMembersPopulated)
{
retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty;
}
if (_packedFlags.IsConditionalPopulated)
{
retVal._lazyConditionalAttributeSymbols = ImmutableArray<string>.Empty;
}

if (_packedFlags.IsMemberNotNullPopulated)
{
retVal._lazyNotNullMembers = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenTrue = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenFalse = ImmutableArray<string>.Empty;
}
if (_packedFlags.IsOverriddenOrHiddenMembersPopulated)
{
retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty;
}

if (_packedFlags.IsExplicitOverrideIsPopulated)
{
retVal._lazyExplicitClassOverride = null;
}
if (_packedFlags.IsMemberNotNullPopulated)
{
retVal._lazyNotNullMembers = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenTrue = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenFalse = ImmutableArray<string>.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;
Expand Down
60 changes: 41 additions & 19 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,28 @@ public void SetCustomAttributesPopulated()
public readonly bool IsCustomAttributesPopulated => (_bits & IsCustomAttributesPopulatedBit) != 0;
}

/// <summary>
/// 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 <see cref="_flags"/>.
/// 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:
/// <list type="number">
/// <item><see cref="_uncommonFields"/> is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of
/// <see cref="_uncommonFields"/> however it chooses.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field has been initialized to some empty value, such as
/// <see cref="ImmutableArray{T}.Empty"/>. In this case, again, no race has occurred, and the consuming code can simply trust the empty value.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field is uninitialized, either being <see langword="default" />, 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.</item>
/// </list>
/// </summary>
/// <remarks>
/// The initialization pattern for this type <b>must</b> 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 <b>must</b> first call <see cref="AccessUncommonFields"/>,
/// set the backing field using an atomic operation, and then set the flag in <see cref="_flags"/>. 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.
/// </remarks>
private sealed class UncommonFields
{
public ImmutableArray<CSharpAttributeData> _lazyCustomAttributes;
Expand Down Expand Up @@ -312,31 +334,31 @@ static bool anyUnexpectedRequiredModifiers(ParamInfo<TypeSymbol>[] 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<AssemblySymbol>.Uninitialized;
}
var retVal = new UncommonFields();
if (!_flags.IsObsoleteAttributePopulated)
{
retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
}

if (_flags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}
if (!_flags.IsUseSiteDiagnosticPopulated)
{
retVal._lazyCachedUseSiteInfo = CachedUseSiteInfo<AssemblySymbol>.Uninitialized;
}

return retVal;
}
if (_flags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}

private UncommonFields AccessUncommonFields()
{
var retVal = _uncommonFields;
return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, CreateUncommonFields());
return retVal;
}
}

private bool MustCallMethodsDirectlyCore()
Expand Down