Concurrency issue with fixed fields #56581

jcouv · 2021-09-21T16:10:55Z

The issue is that we're collecting synthesized nested types into concurrent queues but adding them in a non-deterministic order.
The two code paths below compete to add those nested types and include tasks/threads when not under a debugger (tests turn off concurrency when debugged).
The result was that those nested types didn't always appear in the same order in metadata.

   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceFixedFieldSymbol.FixedImplementationType(PEModuleBuilder emitModule)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(NamedTypeSymbol containingType)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.VisitNamedType(NamedTypeSymbol symbol, TypeCompilationState arg)
   at Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol.Accept[TArgument,TResult](CSharpSymbolVisitor`2 visitor, TArgument argument)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamespace(NamespaceSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethodBodies(CSharpCompilation compilation, PEModuleBuilder moduleBeingBuiltOpt, Boolean emittingPdb, Boolean emitTestCoverageData, Boolean hasDeclarationErrors, Boolean emitMethodBodies, BindingDiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(CommonPEModuleBuilder moduleBuilder, Boolean emittingPdb, Boolean emitMetadataOnly, Boolean emitTestCoverageData, DiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream metadataPEStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, RebuildData rebuildData, CompilationTestData testData, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, RebuildData rebuildData, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.CSharp.UnitTests.Emit.DeterministicTests.TODO2()

   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceFixedFieldSymbol.FixedImplementationType(PEModuleBuilder emitModule)
   at Microsoft.CodeAnalysis.CSharp.Symbols.FieldSymbolAdapter.Microsoft.Cci.IFieldReference.GetType(EmitContext context)
   at Microsoft.CodeAnalysis.CodeGen.ReferenceDependencyWalker.VisitFieldReference(IFieldReference fieldReference, EmitContext context)
   at Microsoft.CodeAnalysis.CodeGen.ReferenceDependencyWalker.VisitReference(IReference reference, EmitContext context)
   at Microsoft.CodeAnalysis.Emit.CommonPEModuleBuilder.GetFakeSymbolTokenForIL(IReference symbol, SyntaxNode syntaxNode, DiagnosticBag diagnostics)
   at Microsoft.CodeAnalysis.CodeGen.ILBuilder.EmitToken(IReference value, SyntaxNode syntaxNode, DiagnosticBag diagnostics, Boolean encodeAsRawToken)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitSymbolToken(FieldSymbol symbol, SyntaxNode syntaxNode)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitInstanceFieldAddress(BoundFieldAccess fieldAccess, AddressKind addressKind)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitFieldAddress(BoundFieldAccess fieldAccess, AddressKind addressKind)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAddress(BoundExpression expression, AddressKind addressKind)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAddressOfExpression(BoundAddressOfOperator expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(BoundExpression expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpression(BoundExpression expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAssignmentPreamble(BoundAssignmentOperator assignmentOperator)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAssignmentExpression(BoundAssignmentOperator assignmentOperator, UseKind useKind)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(BoundExpression expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCoreWithStackGuard(BoundExpression expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpression(BoundExpression expression, Boolean used)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatement(BoundStatement statement)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatementAndCountInstructions(BoundStatement statement)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitSequencePointStatement(BoundSequencePoint node)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatement(BoundStatement statement)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatements(ImmutableArray`1 statements)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitBlock(BoundBlock block)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatement(BoundStatement statement)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatementList(BoundStatementList list)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitStatement(BoundStatement statement)
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.GenerateImpl()
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.Generate(Boolean& hasStackalloc)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.GenerateMethodBody(PEModuleBuilder moduleBuilder, MethodSymbol method, Int32 methodOrdinal, BoundStatement block, ImmutableArray`1 lambdaDebugInfo, ImmutableArray`1 closureDebugInfo, StateMachineTypeSymbol stateMachineTypeOpt, VariableSlotAllocator variableSlotAllocatorOpt, BindingDiagnosticBag diagnostics, DebugDocumentProvider debugDocumentProvider, ImportChain importChainOpt, Boolean emittingPdb, Boolean emitTestCoverageData, ImmutableArray`1 dynamicAnalysisSpans, AsyncForwardEntryPoint entryPointOpt)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(MethodSymbol methodSymbol, Int32 methodOrdinal, ProcessedFieldInitializers& processedInitializers, SynthesizedSubmissionFields previousSubmissionFields, TypeCompilationState compilationState)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(NamedTypeSymbol containingType)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.VisitNamedType(NamedTypeSymbol symbol, TypeCompilationState arg)
   at Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol.Accept[TArgument,TResult](CSharpSymbolVisitor`2 visitor, TArgument argument)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamespace(NamespaceSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethodBodies(CSharpCompilation compilation, PEModuleBuilder moduleBeingBuiltOpt, Boolean emittingPdb, Boolean emitTestCoverageData, Boolean hasDeclarationErrors, Boolean emitMethodBodies, BindingDiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(CommonPEModuleBuilder moduleBuilder, Boolean emittingPdb, Boolean emitMetadataOnly, Boolean emitTestCoverageData, DiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream metadataPEStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, RebuildData rebuildData, CompilationTestData testData, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, RebuildData rebuildData, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.CSharp.UnitTests.Emit.DeterministicTests.TODO2()

jcouv · 2021-09-21T16:11:23Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

@@ -1417,17 +1417,6 @@ public NamedTypeSymbol SetFixedImplementationType(SourceMemberFieldSymbol field)
            }
        }

-        internal NamedTypeSymbol GetFixedImplementationType(FieldSymbol field)


📝 This code was not referenced.

jcouv · 2021-09-29T19:16:10Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+                .WithOverflowChecks(false)
+                .WithModuleName(assemblyName);
+
+            for (int j = 0; j < 10; j++)


📝 I've tested with as many as 100 loops and max=100, but found this was a sufficient number to repro well

cston · 2021-09-29T22:47:48Z

src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs

@@ -763,7 +763,7 @@ public ImmutableArray<ISymbolInternal> GetAllMembers()

                if (NestedTypes != null)
                {
-                    foreach (var type in NestedTypes)
+                    foreach (var type in NestedTypes.OrderBy(t => t.Name, StringComparer.Ordinal))


NestedTypes.OrderBy(t => t.Name, StringComparer.Ordinal)

Consider adding an IEnumerable<Cci.INestedTypeDefinition> NestedTypesOrdered() method to this type, for use here and below. #Resolved

cston · 2021-09-29T23:01:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/TopLevelStatementsTests.cs

-        <forward declaringType=""{ EscapeForXML(WellKnownMemberNames.TopLevelStatementsEntryPointTypeName) }+&lt;&gt;c"" methodName=""&lt;{ EscapeForXML(WellKnownMemberNames.TopLevelStatementsEntryPointMethodName) }&gt;b__0_0"" />
+        <using>
+          <namespace usingCount=""2"" />
+        </using>


Is this change because namespace imports are now attached to a different method, because of re-ordered types? #Resolved

Yes, this information about usings is order dependent. One method gets it and the following ones refer to the first method instead of repeating it.

333fred · 2021-09-30T00:19:10Z

        public ConcurrentQueue<Cci.INestedTypeDefinition> NestedTypes;

Can we make this private and add a public AddNestedType for additions, so that we know there are no reads of the values in this property?

In reply to: 930640897

Refers to: src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs:731 in a15e86b. [](commit_id = a15e86b, deletion_comment = False)

333fred · 2021-09-30T00:20:59Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+            byte[] result = null;
+
+            var sourceBuilder = ArrayBuilder<string>.GetInstance();
+            int max = 20;


Nit: const #Resolved

333fred · 2021-09-30T00:26:03Z

Done review pass (commit 2)

jcouv · 2021-09-30T16:59:17Z

@333fred Please take another look. Thanks

CyrusNajmabadi · 2021-09-30T17:48:39Z

src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs

+            // Nested types may be queued from concurrent threads, but we need to emit them
+            // in a deterministic order.
+            internal IEnumerable<Cci.INestedTypeDefinition> DeterministicNestedTypes
+                => NestedTypes?.OrderBy(t => t.Name, StringComparer.Ordinal);


what if you have multiple nested types with the same name, but different arity?

why not emit them in source-order (taking into account partials as well of course).

These nested types are synthesized. Not directly from source. I don't think we synthesize nested types with different arities. Could add this in 17.1 for extra safety (future-proofing)...
As for the order itself, I don't think there's a reason to prefer one specific order to another, as long as it's deterministic.

I don't think we synthesize nested types with different arities

Can you add a contract call to that effect hten? if that assumption ever changes we want to update this.

Good idea. I'll create a 17.1 PR to add assertion. Thanks

Actually, since this PR isn't merged yet, I'll add the assertion here. Still have time for tomorrow's deadline

333fred

LGTM (commit 3)

cston · 2021-09-30T19:04:20Z

src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs

+                            Debug.Assert(arities.All(a => a == arities.First()));
+                        }
+                    }
+#endif


Can the assert be simplified (or perhaps documented)?

And should we simply assert that there are no duplicate names?

Perhaps:

Debug.Assert(NestedTypes is null || NestedTypes.OrderBy(t => t.Name, StringComparer.Ordinal).Distinct());

Distinct returns an enumerable...
But we can indeed compare counts.

jcouv added the Area-Compilers label Sep 21, 2021

jcouv self-assigned this Sep 21, 2021

jcouv commented Sep 21, 2021

View reviewed changes

jcouv added this to the 17.0 milestone Sep 21, 2021

jcouv changed the base branch from main to release/dev17.0 September 28, 2021 00:13

dotnet deleted a comment from azure-pipelines bot Sep 28, 2021

jcouv closed this Sep 28, 2021

jcouv reopened this Sep 28, 2021

jcouv closed this Sep 28, 2021

jcouv reopened this Sep 28, 2021

jcouv force-pushed the fixed-bug branch 5 times, most recently from 58ac696 to dab8420 Compare September 29, 2021 19:10

jcouv commented Sep 29, 2021

View reviewed changes

Concurrency issue with fixed fields

19dd7dd

jcouv force-pushed the fixed-bug branch from dab8420 to 19dd7dd Compare September 29, 2021 19:17

runfoapp bot mentioned this pull request Sep 29, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

jcouv marked this pull request as ready for review September 29, 2021 22:03

jcouv requested a review from a team as a code owner September 29, 2021 22:03

cston reviewed Sep 29, 2021

View reviewed changes

jcouv requested a review from a team September 29, 2021 23:01

cston approved these changes Sep 29, 2021

View reviewed changes

Extract common code

a15e86b

333fred reviewed Sep 30, 2021

View reviewed changes

Address feedback

3e3606f

jcouv requested a review from 333fred September 30, 2021 00:40

CyrusNajmabadi reviewed Sep 30, 2021

View reviewed changes

333fred approved these changes Sep 30, 2021

View reviewed changes

jcouv enabled auto-merge (squash) September 30, 2021 18:03

jcouv disabled auto-merge September 30, 2021 18:19

Add assertion

0264fc7

cston reviewed Sep 30, 2021

View reviewed changes

Address feedback

4bb559c

cston approved these changes Sep 30, 2021

View reviewed changes

jcouv enabled auto-merge (squash) September 30, 2021 23:57

jcouv merged commit 9c798fd into dotnet:release/dev17.0 Oct 1, 2021

jcouv deleted the fixed-bug branch October 1, 2021 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concurrency issue with fixed fields #56581

Concurrency issue with fixed fields #56581

jcouv commented Sep 21, 2021

jcouv Sep 21, 2021

jcouv Sep 29, 2021 •

edited

Loading

cston Sep 29, 2021 •

edited by jcouv

Loading

cston Sep 29, 2021 •

edited by jcouv

Loading

jcouv Sep 29, 2021

333fred commented Sep 30, 2021 •

edited by jcouv

Loading

333fred Sep 30, 2021 •

edited by jcouv

Loading

333fred commented Sep 30, 2021

jcouv commented Sep 30, 2021

CyrusNajmabadi Sep 30, 2021

CyrusNajmabadi Sep 30, 2021

jcouv Sep 30, 2021

CyrusNajmabadi Sep 30, 2021

jcouv Sep 30, 2021

jcouv Sep 30, 2021

333fred left a comment •

edited

Loading

cston Sep 30, 2021 •

edited

Loading

jcouv Sep 30, 2021

Concurrency issue with fixed fields #56581

Concurrency issue with fixed fields #56581

Conversation

jcouv commented Sep 21, 2021

Choose a reason for hiding this comment

jcouv Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

cston Sep 29, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 29, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Sep 30, 2021 • edited by jcouv Loading

333fred Sep 30, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred commented Sep 30, 2021

jcouv commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment • edited Loading

Choose a reason for hiding this comment

cston Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Sep 29, 2021 •

edited

Loading

cston Sep 29, 2021 •

edited by jcouv

Loading

cston Sep 29, 2021 •

edited by jcouv

Loading

333fred commented Sep 30, 2021 •

edited by jcouv

Loading

333fred Sep 30, 2021 •

edited by jcouv

Loading

333fred left a comment •

edited

Loading

cston Sep 30, 2021 •

edited

Loading