From 38463e2269889321ecea15317a20955f38d560a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Sun, 12 Nov 2023 15:47:21 -0500 Subject: [PATCH] [mono] catch the case of updated methods in mono_debug_lookup_source_location (#94540) * Add new crashing test * [mono] catch the case of updated methods in mono_debug_lookup_source_location If a newly added method is in a stack trace, look it up in the EnC debug information, if available. * add debugger test Co-authored-by: Thays Grazia --- .../NewMethodThrows.cs | 16 +++++ .../NewMethodThrows_v1.cs | 21 +++++++ ...ta.ApplyUpdate.Test.NewMethodThrows.csproj | 11 ++++ .../deltascript.json | 6 ++ .../tests/ApplyUpdateTest.cs | 47 +++++++++++++- .../tests/System.Runtime.Loader.Tests.csproj | 1 + src/mono/mono/metadata/mono-debug.c | 22 +++++++ .../DebuggerTestSuite/HotReloadTests.cs | 62 +++++++++++++++++++ .../MethodBody1.cs | 19 ++++++ .../MethodBody1_v1.cs | 14 +++++ .../MethodBody1_v2.cs | 14 +++++ 11 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows_v1.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows.csproj create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/deltascript.json diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows.cs new file mode 100644 index 0000000000000..baef24d1ab19e --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; + + +namespace System.Reflection.Metadata.ApplyUpdate.Test +{ + public class NewMethodThrows + { + public string ExistingMethod(string x) + { + return x; + } + + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows_v1.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows_v1.cs new file mode 100644 index 0000000000000..be7ee1389ce1d --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/NewMethodThrows_v1.cs @@ -0,0 +1,21 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; + + +namespace System.Reflection.Metadata.ApplyUpdate.Test +{ + public class NewMethodThrows + { + public string ExistingMethod(string x) + { + return NewMethod(x); + } + + public string NewMethod(string x) + { + throw new InvalidOperationException (x); + } + + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows.csproj b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows.csproj new file mode 100644 index 0000000000000..2948ee7979c52 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows.csproj @@ -0,0 +1,11 @@ + + + System.Runtime.Loader.Tests + $(NetCoreAppCurrent) + true + deltascript.json + + + + + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/deltascript.json b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/deltascript.json new file mode 100644 index 0000000000000..fbc53a9a091f9 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows/deltascript.json @@ -0,0 +1,6 @@ +{ + "changes": [ + {"document": "NewMethodThrows.cs", "update": "NewMethodThrows_v1.cs"}, + ] +} + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs index 8ca126834cfcd..2fdb62c7e4bfb 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs @@ -938,5 +938,50 @@ public static void TestGenericAddInstanceField() Assert.Equal(dt, z.GetIt()); }); } - } + + [ConditionalFact(typeof(ApplyUpdateUtil), nameof(ApplyUpdateUtil.IsSupported))] + public static void TestNewMethodThrows() + { + ApplyUpdateUtil.TestCase(static () => + { + var assm = typeof(System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows).Assembly; + + var x = new System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows(); + + Assert.Equal("abcd", x.ExistingMethod("abcd")); + + ApplyUpdateUtil.ApplyUpdate(assm); + + InvalidOperationException exn = Assert.Throws(() => x.ExistingMethod("spqr")); + + Assert.Equal("spqr", exn.Message); + + var stackTrace = new System.Diagnostics.StackTrace(exn, fNeedFileInfo: true); + + var frames = stackTrace.GetFrames(); + + // the throwing method and its caller and a few frames of XUnit machinery for Assert.Throws, above + Assert.True(frames.Length >= 2); + + var throwingMethod = frames[0].GetMethod(); + + var newMethod = typeof (System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows).GetMethod("NewMethod"); + + Assert.Equal(newMethod, throwingMethod); + + // We don't have the filename on all runtimes and platforms + var frame0Name = frames[0].GetFileName(); + Assert.True(frame0Name == null || frame0Name.Contains("NewMethodThrows.cs")); + + var existingMethod = typeof (System.Reflection.Metadata.ApplyUpdate.Test.NewMethodThrows).GetMethod("ExistingMethod"); + + var throwingMethodCaller = frames[1].GetMethod(); + + Assert.Equal(existingMethod, throwingMethodCaller); + + var frame1Name = frames[0].GetFileName(); + Assert.True(frame1Name == null || frame1Name.Contains("NewMethodThrows.cs")); + }); + } + } } diff --git a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj index 6ae8f59512e03..03138144a9db7 100644 --- a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj +++ b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj @@ -64,6 +64,7 @@ + diff --git a/src/mono/mono/metadata/mono-debug.c b/src/mono/mono/metadata/mono-debug.c index e39fc35b2a841..392581889def5 100644 --- a/src/mono/mono/metadata/mono-debug.c +++ b/src/mono/mono/metadata/mono-debug.c @@ -777,6 +777,28 @@ mono_debug_lookup_source_location (MonoMethod *method, guint32 address, MonoDoma if (mono_debug_format == MONO_DEBUG_FORMAT_NONE) return NULL; + MonoImage *img = m_class_get_image (method->klass); + if (G_UNLIKELY (img->has_updates)) { + guint32 idx = mono_metadata_token_index (method->token); + MonoDebugInformationEnc *mdie = (MonoDebugInformationEnc *) mono_metadata_update_get_updated_method_ppdb (img, idx); + if (mdie != NULL) { + offset = il_offset_from_address (method, address); + if (offset < 0) { + mono_debugger_unlock (); + return NULL; + } + + MonoDebugSourceLocation * ret = mono_ppdb_lookup_location_enc (mdie->ppdb_file, mdie->idx, offset); + if (ret) + return ret; + } else { + /// added method without EnC info, maybe the delta came in without a PDB delta + gboolean added_method = idx >= table_info_get_rows (&img->tables[MONO_TABLE_METHOD]); + if (added_method) + return NULL; + } + } + mono_debugger_lock (); minfo = lookup_method (method); if (!minfo || !minfo->handle) { diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/HotReloadTests.cs b/src/mono/wasm/debugger/DebuggerTestSuite/HotReloadTests.cs index e2bfcce85cf72..3a5174d12eec9 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/HotReloadTests.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/HotReloadTests.cs @@ -627,6 +627,68 @@ public async Task DebugHotReloadMethod_AddingNewClassUsingDebugAttribute() await StepAndCheck(StepKind.Into, $"dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 119, 12, "ApplyUpdateReferencedAssembly.MethodBody10.StaticMethod1"); } + [ConditionalFact(nameof(RunningOnChrome))] + public async Task DebugHotReloadMethod_AddingNewMethodAndThrowException() + { + //await SetPauseOnException("all"); + string asm_file = Path.Combine(DebuggerTestAppPath, "ApplyUpdateReferencedAssembly.dll"); + string pdb_file = Path.Combine(DebuggerTestAppPath, "ApplyUpdateReferencedAssembly.pdb"); + string asm_file_hot_reload = Path.Combine(DebuggerTestAppPath, "../wasm/ApplyUpdateReferencedAssembly.dll"); + + var bp_notchanged = await SetBreakpoint(".*/MethodBody1.cs$", 129, 12, use_regex: true); + var bp_invalid = await SetBreakpoint(".*/MethodBody1.cs$", 133, 12, use_regex: true); + + var pause_location = await LoadAssemblyAndTestHotReloadUsingSDBWithoutChanges( + asm_file, pdb_file, "MethodBody11", "StaticMethod1", expectBpResolvedEvent: true, sourcesToWait: new string [] { "MethodBody0.cs", "MethodBody1.cs" }); + + CheckLocation("dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 129, 12, scripts, pause_location["callFrames"]?[0]["location"]); + //apply first update + pause_location = await LoadAssemblyAndTestHotReloadUsingSDB( + asm_file_hot_reload, "MethodBody11", "NewMethodStaticWithThrow", 1); + + JToken top_frame = pause_location["callFrames"]?[0]; + AssertEqual("ApplyUpdateReferencedAssembly.MethodBody11.NewMethodStaticWithThrow", top_frame?["functionName"]?.Value(), top_frame?.ToString()); + CheckLocation("dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 133, 12, scripts, top_frame["location"]); + await StepAndCheck(StepKind.Over, "dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 134, 12, "ApplyUpdateReferencedAssembly.MethodBody11.NewMethodStaticWithThrow", + locals_fn: async (locals) => + { + CheckNumber(locals, "i", 20); + await Task.CompletedTask; + } + ); + + await SetPauseOnException("all"); + + pause_location = await SendCommandAndCheck(JObject.FromObject(new { }), "Debugger.resume", null, 0, 0, null); + + await CheckValue(pause_location["data"], JObject.FromObject(new + { + type = "object", + subtype = "error", + className = "System.Exception", + uncaught = false + }), "exception0.data"); + + pause_location = await SendCommandAndCheck(JObject.FromObject(new { }), "Debugger.resume", null, 0, 0, null); + try + { + pause_location = await SendCommandAndCheck(JObject.FromObject(new { }), "Debugger.resume", null, 0, 0, null); + } + catch (System.Exception ae) + { + System.Console.WriteLine(ae); + var eo = JObject.Parse(ae.Message); + + AssertEqual("Uncaught", eo["exceptionDetails"]?["text"]?.Value(), "text"); + + await CheckValue(eo["exceptionDetails"]?["exception"], JObject.FromObject(new + { + type = "object", + subtype = "error", + className = "Error" + }), "exception"); + } + } // Enable this test when https://github.com/dotnet/hotreload-utils/pull/264 flows into dotnet/runtime repo // [ConditionalFact(nameof(RunningOnChrome))] // public async Task DebugHotReloadMethod_ChangeParameterName() diff --git a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1.cs b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1.cs index 2c510d1415d51..bb23eb2b9d2b5 100644 --- a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1.cs +++ b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1.cs @@ -112,4 +112,23 @@ public static void StaticMethod1 () { // return M1(1, 2); // } // } + + + + + + + + + + + + + + public class MethodBody11 { + public static void StaticMethod1 () { + Console.WriteLine("breakpoint in a line that will not be changed"); + Console.WriteLine("original"); + } + } } diff --git a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v1.cs b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v1.cs index b813b506d8692..7b6c0403de45f 100644 --- a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v1.cs +++ b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v1.cs @@ -124,4 +124,18 @@ public static void StaticMethod2 () { Console.WriteLine($"do not step into here"); } } + + public class MethodBody11 { + public static void StaticMethod1 () { + Console.WriteLine("breakpoint in a line that will not be changed"); + Console.WriteLine("original"); + } + public static void NewMethodStaticWithThrow () { + int i = 20; + Console.WriteLine($"add a breakpoint in the new static method, look at locals {i}"); + throw new Exception("my exception"); + /*var newvar = new MethodBody6(); + newvar.NewMethodInstance (10);*/ + } + } } diff --git a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v2.cs b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v2.cs index 3ea27466fa549..dda7496ccc91e 100644 --- a/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v2.cs +++ b/src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v2.cs @@ -124,4 +124,18 @@ public static void StaticMethod2 () { Console.WriteLine($"do not step into here"); } } + + public class MethodBody11 { + public static void StaticMethod1 () { + Console.WriteLine("breakpoint in a line that will not be changed"); + Console.WriteLine("original"); + } + public static void NewMethodStaticWithThrow () { + int i = 20; + Console.WriteLine($"add a breakpoint in the new static method, look at locals {i}"); + throw new Exception("my exception"); + /*var newvar = new MethodBody6(); + newvar.NewMethodInstance (10);*/ + } + } }