From e74cd1a7bf5ca0790ff1c359cdf236d8902338bf Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 13 Mar 2015 13:41:57 -0700 Subject: [PATCH] Consider MVID when testing EC cache validity Our EvaluationContext cache invalidation computation was consuming the method token and version, but not the identity of the declaring assembly. This lead to a strange bug where we would reuse the cache across assembly boundaries if consecutive breakpoint happened to have the same method token (each in its own assembly). I believe this bug existed before I revised the cache invalidation computation and we were just getting lucky - we used to check the spans of all containing scopes, which are unlikely to match exactly across assemblies. --- .../ExpressionCompiler/EvaluationContext.cs | 4 +-- .../ExpressionCompilerTests.cs | 6 ++--- .../MethodContextReuseConstraintsTests.cs | 25 ++++++++++++------- .../ExpressionCompiler/MetadataUtilities.cs | 4 +-- .../MethodContextReuseConstraints.cs | 23 +++++++++++------ .../ExpressionCompiler/EvaluationContext.vb | 4 +-- .../ExpressionCompilerTests.vb | 6 ++--- 7 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs index 165254072be..df83dd6ed59 100644 --- a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs +++ b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs @@ -129,7 +129,7 @@ internal sealed class EvaluationContext : EvaluationContextBase var previousContext = previous.EvaluationContext; if (previousContext != null && previousContext.MethodContextReuseConstraints.HasValue && - previousContext.MethodContextReuseConstraints.GetValueOrDefault().AreSatisfied(methodToken, methodVersion, ilOffset)) + previousContext.MethodContextReuseConstraints.GetValueOrDefault().AreSatisfied(moduleVersionId, methodToken, methodVersion, ilOffset)) { return previousContext; } @@ -144,7 +144,7 @@ internal sealed class EvaluationContext : EvaluationContextBase var allScopes = ArrayBuilder.GetInstance(); var containingScopes = ArrayBuilder.GetInstance(); typedSymReader.GetScopes(methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive, allScopes, containingScopes); - var methodContextReuseConstraints = allScopes.GetReuseConstraints(methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive); + var methodContextReuseConstraints = allScopes.GetReuseConstraints(moduleVersionId, methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive); allScopes.Free(); var localNames = containingScopes.GetLocalNames(); diff --git a/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs b/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs index ba477c42611..f9ffee47e44 100644 --- a/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs +++ b/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs @@ -374,7 +374,7 @@ static void G() var constraints = previous.EvaluationContext.MethodContextReuseConstraints; if (constraints.HasValue) { - Assert.Equal(scope == previousScope, constraints.GetValueOrDefault().AreSatisfied(methodToken, methodVersion, offset)); + Assert.Equal(scope == previousScope, constraints.GetValueOrDefault().AreSatisfied(moduleVersionId, methodToken, methodVersion, offset)); } context = EvaluationContext.CreateMethodContext(previous, methodBlocks, symReader, moduleVersionId, methodToken, methodVersion, offset, localSignatureToken); @@ -405,7 +405,7 @@ static void G() // Different references. No reuse. context = EvaluationContext.CreateMethodContext(previous, methodBlocks, symReader, moduleVersionId, methodToken, methodVersion, endOffset, localSignatureToken); Assert.NotEqual(context, previous.EvaluationContext); - Assert.True(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(methodToken, methodVersion, endOffset)); + Assert.True(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(moduleVersionId, methodToken, methodVersion, endOffset)); Assert.NotEqual(context.Compilation, previous.Compilation); previous = new CSharpMetadataContext(context); @@ -413,7 +413,7 @@ static void G() GetContextState(runtime, "C.G", out methodBlocks, out moduleVersionId, out symReader, out methodToken, out localSignatureToken); context = EvaluationContext.CreateMethodContext(previous, methodBlocks, symReader, moduleVersionId, methodToken, methodVersion, ilOffset: 0, localSignatureToken: localSignatureToken); Assert.NotEqual(context, previous.EvaluationContext); - Assert.False(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(methodToken, methodVersion, 0)); + Assert.False(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(moduleVersionId, methodToken, methodVersion, 0)); Assert.Equal(context.Compilation, previous.Compilation); // No EvaluationContext. Should reuse Compilation diff --git a/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/MethodContextReuseConstraintsTests.cs b/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/MethodContextReuseConstraintsTests.cs index b657062365b..20f9058cfda 100644 --- a/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/MethodContextReuseConstraintsTests.cs +++ b/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/MethodContextReuseConstraintsTests.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using Microsoft.CodeAnalysis.ExpressionEvaluator; using Xunit; @@ -10,33 +11,37 @@ public class MethodContextReuseConstraintsTests : ExpressionCompilerTestBase [Fact] public void AreSatisfied() { + var moduleVersionId = Guid.NewGuid(); const int methodToken = 0x06000001; const int methodVersion = 1; const uint startOffset = 1; const uint endOffsetExclusive = 3; var constraints = MethodContextReuseConstraints.CreateTestInstance( + moduleVersionId, methodToken, methodVersion, startOffset, endOffsetExclusive); - Assert.True(constraints.AreSatisfied(methodToken, methodVersion, (int)startOffset)); - Assert.True(constraints.AreSatisfied(methodToken, methodVersion, (int)endOffsetExclusive - 1)); + Assert.True(constraints.AreSatisfied(moduleVersionId, methodToken, methodVersion, (int)startOffset)); + Assert.True(constraints.AreSatisfied(moduleVersionId, methodToken, methodVersion, (int)endOffsetExclusive - 1)); - Assert.False(constraints.AreSatisfied(methodToken + 1, methodVersion, (int)startOffset)); - Assert.False(constraints.AreSatisfied(methodToken, methodVersion + 1, (int)startOffset)); - Assert.False(constraints.AreSatisfied(methodToken, methodVersion, (int)startOffset - 1)); - Assert.False(constraints.AreSatisfied(methodToken, methodVersion, (int)endOffsetExclusive)); + Assert.False(constraints.AreSatisfied(Guid.NewGuid(), methodToken, methodVersion, (int)startOffset)); + Assert.False(constraints.AreSatisfied(moduleVersionId, methodToken + 1, methodVersion, (int)startOffset)); + Assert.False(constraints.AreSatisfied(moduleVersionId, methodToken, methodVersion + 1, (int)startOffset)); + Assert.False(constraints.AreSatisfied(moduleVersionId, methodToken, methodVersion, (int)startOffset - 1)); + Assert.False(constraints.AreSatisfied(moduleVersionId, methodToken, methodVersion, (int)endOffsetExclusive)); } [Fact] public void EndInclusive() { + var moduleVersionId = Guid.NewGuid(); const int methodToken = 0x06000001; const int methodVersion = 1; - var builder = new MethodContextReuseConstraints.Builder(methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: true); + var builder = new MethodContextReuseConstraints.Builder(moduleVersionId, methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: true); Assert.True(builder.Build().HasExpectedSpan(0u, uint.MaxValue)); builder.AddRange(1, 9); @@ -55,10 +60,11 @@ public void EndInclusive() [Fact] public void EndExclusive() { + var moduleVersionId = Guid.NewGuid(); const int methodToken = 0x06000001; const int methodVersion = 1; - var builder = new MethodContextReuseConstraints.Builder(methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: false); + var builder = new MethodContextReuseConstraints.Builder(moduleVersionId, methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: false); Assert.True(builder.Build().HasExpectedSpan(0u, uint.MaxValue)); builder.AddRange(1, 9); @@ -77,10 +83,11 @@ public void EndExclusive() [Fact] public void Cumulative() { + var moduleVersionId = Guid.NewGuid(); const int methodToken = 0x06000001; const int methodVersion = 1; - var builder = new MethodContextReuseConstraints.Builder(methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: false); + var builder = new MethodContextReuseConstraints.Builder(moduleVersionId, methodToken, methodVersion, ilOffset: 5, areRangesEndInclusive: false); Assert.True(builder.Build().HasExpectedSpan(0u, uint.MaxValue)); builder.AddRange(1, 10); diff --git a/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs b/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs index 191f5f16ef1..f13c8bffe33 100644 --- a/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs +++ b/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs @@ -296,9 +296,9 @@ internal static bool IsWindowsAssemblyIdentity(this AssemblyIdentity assemblyIde symMethod.GetAllScopes(allScopes, containingScopes, ilOffset, isScopeEndInclusive); } - internal static MethodContextReuseConstraints GetReuseConstraints(this ArrayBuilder scopes, int methodToken, int methodVersion, int ilOffset, bool isEndInclusive) + internal static MethodContextReuseConstraints GetReuseConstraints(this ArrayBuilder scopes, Guid moduleVersionId, int methodToken, int methodVersion, int ilOffset, bool isEndInclusive) { - var builder = new MethodContextReuseConstraints.Builder(methodToken, methodVersion, ilOffset, isEndInclusive); + var builder = new MethodContextReuseConstraints.Builder(moduleVersionId, methodToken, methodVersion, ilOffset, isEndInclusive); foreach (ISymUnmanagedScope scope in scopes) { builder.AddRange((uint)scope.GetStartOffset(), (uint)scope.GetEndOffset()); diff --git a/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MethodContextReuseConstraints.cs b/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MethodContextReuseConstraints.cs index dd211fbdd3e..2e5a1d99d68 100644 --- a/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MethodContextReuseConstraints.cs +++ b/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MethodContextReuseConstraints.cs @@ -9,34 +9,38 @@ namespace Microsoft.CodeAnalysis.ExpressionEvaluator { internal struct MethodContextReuseConstraints { + private readonly Guid _moduleVersionId; private readonly int _methodToken; private readonly int _methodVersion; private readonly uint _startOffset; private readonly uint _endOffsetExclusive; - private MethodContextReuseConstraints(int methodToken, int methodVersion, uint startOffset, uint endOffsetExclusive) + private MethodContextReuseConstraints(Guid moduleVersionId, int methodToken, int methodVersion, uint startOffset, uint endOffsetExclusive) { + Debug.Assert(moduleVersionId != default(Guid)); Debug.Assert(MetadataTokens.Handle(methodToken).Kind == HandleKind.MethodDefinition); Debug.Assert(methodVersion >= 1); Debug.Assert(startOffset <= endOffsetExclusive); + _moduleVersionId = moduleVersionId; _methodToken = methodToken; _methodVersion = methodVersion; _startOffset = startOffset; _endOffsetExclusive = endOffsetExclusive; } - public bool AreSatisfied(int methodToken, int methodVersion, int ilOffset) + public bool AreSatisfied(Guid moduleVersionId, int methodToken, int methodVersion, int ilOffset) { - return methodToken == _methodToken && + return moduleVersionId == _moduleVersionId && + methodToken == _methodToken && methodVersion == _methodVersion && ilOffset >= _startOffset && ilOffset < _endOffsetExclusive; } - internal static MethodContextReuseConstraints CreateTestInstance(int methodToken, int methodVersion, uint startOffset, uint endOffsetExclusive) + internal static MethodContextReuseConstraints CreateTestInstance(Guid moduleVersionId, int methodToken, int methodVersion, uint startOffset, uint endOffsetExclusive) { - return new MethodContextReuseConstraints(methodToken, methodVersion, startOffset, endOffsetExclusive); + return new MethodContextReuseConstraints(moduleVersionId, methodToken, methodVersion, startOffset, endOffsetExclusive); } internal bool HasExpectedSpan(uint startOffset, uint endOffsetExclusive) @@ -46,11 +50,12 @@ internal bool HasExpectedSpan(uint startOffset, uint endOffsetExclusive) public override string ToString() { - return $"0x{_methodToken:x8}v{_methodVersion} [{_startOffset}, {_endOffsetExclusive})"; + return $"0x{_methodToken:x8}v{_methodVersion} from {_moduleVersionId} [{_startOffset}, {_endOffsetExclusive})"; } public class Builder { + private readonly Guid _moduleVersionId; private readonly int _methodToken; private readonly int _methodVersion; private readonly int _ilOffset; @@ -59,12 +64,14 @@ public class Builder private uint _startOffset; private uint _endOffsetExclusive; - public Builder(int methodToken, int methodVersion, int ilOffset, bool areRangesEndInclusive) + public Builder(Guid moduleVersionId, int methodToken, int methodVersion, int ilOffset, bool areRangesEndInclusive) { + Debug.Assert(moduleVersionId != default(Guid)); Debug.Assert(MetadataTokens.Handle(methodToken).Kind == HandleKind.MethodDefinition); Debug.Assert(methodVersion >= 1); Debug.Assert(ilOffset >= 0); + _moduleVersionId = moduleVersionId; _methodToken = methodToken; _methodVersion = methodVersion; _ilOffset = ilOffset; @@ -76,6 +83,7 @@ public Builder(int methodToken, int methodVersion, int ilOffset, bool areRangesE public Builder(MethodContextReuseConstraints existingConstraints, int ilOffset, bool areRangesEndInclusive) { + _moduleVersionId = existingConstraints._moduleVersionId; _methodToken = existingConstraints._methodToken; _methodVersion = existingConstraints._methodVersion; _ilOffset = ilOffset; @@ -111,6 +119,7 @@ public void AddRange(uint startOffset, uint endOffset) public MethodContextReuseConstraints Build() { return new MethodContextReuseConstraints( + _moduleVersionId, _methodToken, _methodVersion, _startOffset, diff --git a/src/ExpressionEvaluator/VisualBasic/Source/ExpressionCompiler/EvaluationContext.vb b/src/ExpressionEvaluator/VisualBasic/Source/ExpressionCompiler/EvaluationContext.vb index ae9195fd950..b5281b3d89b 100644 --- a/src/ExpressionEvaluator/VisualBasic/Source/ExpressionCompiler/EvaluationContext.vb +++ b/src/ExpressionEvaluator/VisualBasic/Source/ExpressionCompiler/EvaluationContext.vb @@ -130,7 +130,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExpressionEvaluator Dim previousContext = previous.EvaluationContext If previousContext IsNot Nothing AndAlso previousContext.MethodContextReuseConstraints.HasValue AndAlso - previousContext.MethodContextReuseConstraints.GetValueOrDefault().AreSatisfied(methodToken, methodVersion, ilOffset) Then + previousContext.MethodContextReuseConstraints.GetValueOrDefault().AreSatisfied(moduleVersionId, methodToken, methodVersion, ilOffset) Then Return previousContext End If compilation = previous.Compilation @@ -142,7 +142,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExpressionEvaluator Dim allScopes = ArrayBuilder(Of ISymUnmanagedScope).GetInstance() Dim containingScopes = ArrayBuilder(Of ISymUnmanagedScope).GetInstance() typedSymReader.GetScopes(methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive, allScopes, containingScopes) - Dim reuseConstraints = allScopes.GetReuseConstraints(methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive) + Dim reuseConstraints = allScopes.GetReuseConstraints(moduleVersionId, methodToken, methodVersion, ilOffset, IsLocalScopeEndInclusive) allScopes.Free() Dim methodHandle = CType(MetadataTokens.Handle(methodToken), MethodDefinitionHandle) diff --git a/src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/ExpressionCompilerTests.vb b/src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/ExpressionCompilerTests.vb index 72d3a30a8e5..5b47a9f88af 100644 --- a/src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/ExpressionCompilerTests.vb +++ b/src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/ExpressionCompilerTests.vb @@ -275,7 +275,7 @@ End Class" Dim scope = scopes.GetInnermostScope(offset) Dim constraints = previous.EvaluationContext.MethodContextReuseConstraints If constraints.HasValue Then - Assert.Equal(scope Is previousScope, constraints.GetValueOrDefault().AreSatisfied(methodToken, methodVersion, offset)) + Assert.Equal(scope Is previousScope, constraints.GetValueOrDefault().AreSatisfied(moduleVersionId, methodToken, methodVersion, offset)) End If context = EvaluationContext.CreateMethodContext(previous, methodBlocks, MakeDummyLazyAssemblyReaders(), symReader, moduleVersionId, methodToken, methodVersion, offset, localSignatureToken) @@ -307,7 +307,7 @@ End Class" ' Different references. No reuse. context = EvaluationContext.CreateMethodContext(previous, methodBlocks, MakeDummyLazyAssemblyReaders(), symReader, moduleVersionId, methodToken, methodVersion, endOffset - 1, localSignatureToken) Assert.NotEqual(context, previous.EvaluationContext) - Assert.True(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(methodToken, methodVersion, endOffset - 1)) + Assert.True(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(moduleVersionId, methodToken, methodVersion, endOffset - 1)) Assert.NotEqual(context.Compilation, previous.Compilation) previous = new VisualBasicMetadataContext(context) @@ -315,7 +315,7 @@ End Class" GetContextState(runtime, "C.G", methodBlocks, moduleVersionId, symReader, methodToken, localSignatureToken) context = EvaluationContext.CreateMethodContext(previous, methodBlocks, MakeDummyLazyAssemblyReaders(), symReader, moduleVersionId, methodToken, methodVersion, ilOffset:=0, localSignatureToken:=localSignatureToken) Assert.NotEqual(context, previous.EvaluationContext) - Assert.False(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(methodToken, methodVersion, 0)) + Assert.False(previous.EvaluationContext.MethodContextReuseConstraints.Value.AreSatisfied(moduleVersionId, methodToken, methodVersion, 0)) Assert.Equal(context.Compilation, previous.Compilation) ' No EvaluationContext. Should reuse Compilation -- GitLab