[release/7.0-preview2][mono][hot reload] Fix sequence point logic for…

… compiler-generated methods in updates (#65867)

* [tests] Pass PDB delta to ApplyUpdate

* extra tracing output

* Return 0 if a method has no sequence points

-1 means there was some kind of problem

0 means 0 sequence points

In particular for mono_ppdb_get_seq_points_enc a compiler-generated method
might have 0 sequence points in the PDB delta, but we should consider that as
ok, and not fall back to looking in the baseline image

* pass MONO_DEBUG=getn-seq-points to hot reload tests

For platforms where we use the remote executor, set the environment variable
when starting the remote process.

For WASM, pass --setenv when building the project.

* fix whitespace

* [debug] Handle gen-seq-points, and hot reload updates without PDBs

When hot reload is used with dotnet watch, the baseline PDBs are available, and
sequence points are enabled, but there are no PDB updates.

* [hot_reload] Fix off by one error

when counting added and modified rows

* [hot_reload] Allow custom attribute deletions, even without ChangeCustomAttributes capability

Roslyn seems to delete nullability annotations sometimes

* Add regression test for adding static lambdas

* param attributes for hot reload

* don't run test where it isn't supported

src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression/StaticLambdaRegression.cs

@@ -0,0 +1,24 @@
+// 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 StaticLambdaRegression
+{
+    public int count;
+
+    public string TestMethod()
+    {
+        count++;
+#if false
+        Message (static () => "hello");
+#endif
+        return count.ToString();
+    }
+
+#if false
+    public void Message (Func<string> msg) => Console.WriteLine (msg());
+#endif
+}

src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression/StaticLambdaRegression_v1.cs

@@ -0,0 +1,24 @@
+// 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 StaticLambdaRegression
+{
+    public int count;
+
+    public string TestMethod()
+    {
+        count++;
+#if true
+        Message (static () => "hello2");
+#endif
+        return count.ToString();
+    }
+
+#if true
+    public void Message (Func<string> msg) => Console.WriteLine (msg());
+#endif
+}

src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression/StaticLambdaRegression_v2.cs

@@ -0,0 +1,25 @@
+// 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 StaticLambdaRegression
+{
+    public int count;
+
+    public string TestMethod()
+    {
+        count++;
+#if true
+        Message (static () => "hello2");
+#endif
+        Message (static () => "goodbye");
+        return count.ToString();
+    }
+
+#if true
+    public void Message (Func<string> msg) => Console.WriteLine (msg());
+#endif
+}

src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression.csproj

@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <RootNamespace>System.Runtime.Loader.Tests</RootNamespace>
+    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+    <TestRuntime>true</TestRuntime>
+    <DeltaScript>deltascript.json</DeltaScript>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="StaticLambdaRegression.cs" />
+  </ItemGroup>
+</Project>

src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression/deltascript.json

@@ -0,0 +1,7 @@
+{
+    "changes": [
+        {"document": "StaticLambdaRegression.cs", "update": "StaticLambdaRegression_v1.cs"},
+        {"document": "StaticLambdaRegression.cs", "update": "StaticLambdaRegression_v2.cs"},
+    ]
+}
+

src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs

@@ -395,5 +395,37 @@ public static void IsSupported()
             bool result = MetadataUpdater.IsSupported;
             Assert.False(result);
         }
+
+        [ConditionalFact(typeof(ApplyUpdateUtil), nameof(ApplyUpdateUtil.IsSupported))]
+        public static void TestStaticLambdaRegression()
+        {
+            ApplyUpdateUtil.TestCase(static () =>
+            {
+                var assm = typeof(System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression).Assembly;
+                var x = new System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression();
+
+                Assert.Equal (0, x.count);
+
+                x.TestMethod();
+                x.TestMethod();
+
+                Assert.Equal (2, x.count);
+
+                ApplyUpdateUtil.ApplyUpdate(assm, usePDB: false);
+
+                x.TestMethod();
+                x.TestMethod();
+
+                Assert.Equal (4, x.count);
+
+                ApplyUpdateUtil.ApplyUpdate(assm, usePDB: false);
+
+                x.TestMethod();
+                x.TestMethod();
+
+                Assert.Equal (6, x.count);
+
+            });
+        }
     }
 }

src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs

@@ -66,7 +66,7 @@ internal static bool HasApplyUpdateCapabilities()
 
         private static System.Collections.Generic.Dictionary<Assembly, int> assembly_count = new();
 
-        internal static void ApplyUpdate (System.Reflection.Assembly assm)
+        internal static void ApplyUpdate (System.Reflection.Assembly assm, bool usePDB = true)
         {
             int count;
             if (!assembly_count.TryGetValue(assm, out count))
@@ -83,9 +83,13 @@ internal static void ApplyUpdate (System.Reflection.Assembly assm)
 
             string dmeta_name = $"{basename}.{count}.dmeta";
             string dil_name = $"{basename}.{count}.dil";
+            string dpdb_name = $"{basename}.{count}.dpdb";
             byte[] dmeta_data = System.IO.File.ReadAllBytes(dmeta_name);
             byte[] dil_data = System.IO.File.ReadAllBytes(dil_name);
-            byte[] dpdb_data = null; // TODO also use the dpdb data
+            byte[] dpdb_data = null;
+
+            if (usePDB)
+                dpdb_data = System.IO.File.ReadAllBytes(dpdb_name);
 
             MetadataUpdater.ApplyUpdate(assm, dmeta_data, dil_data, dpdb_data);
         }
@@ -94,6 +98,20 @@ internal static void AddRemoteInvokeOptions (ref RemoteInvokeOptions options)
         {
             options = options ?? new RemoteInvokeOptions();
             options.StartInfo.EnvironmentVariables.Add(DotNetModifiableAssembliesSwitch, DotNetModifiableAssembliesValue);
+            /* Ask mono to use .dpdb data to generate sequence points even without a debugger attached */
+            if (IsMonoRuntime)
+                AppendEnvironmentVariable(options.StartInfo.EnvironmentVariables, "MONO_DEBUG", "gen-seq-points");
+        }
+
+        private static void AppendEnvironmentVariable(System.Collections.Specialized.StringDictionary env, string key, string addedValue)
+        {
+            if (!env.ContainsKey(key))
+                env.Add(key, addedValue);
+            else
+            {
+                string oldValue = env[key];
+                env[key] = oldValue + "," + addedValue;
+            }
         }
 
         /// Run the given test case, which applies updates to the given assembly.

src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj

@@ -7,7 +7,8 @@
     <!-- Some tests rely on no deps.json file being present. -->
     <GenerateDependencyFile>false</GenerateDependencyFile>
     <!-- EnC tests on targets without a remote executor need the environment variable set before launching the test -->
-    <WasmXHarnessMonoArgs>--setenv=DOTNET_MODIFIABLE_ASSEMBLIES=debug</WasmXHarnessMonoArgs>
+    <!-- Also ask Mono to use make sequence points even without a debugger attached to match "dotnet watch" behavior -->
+    <WasmXHarnessMonoArgs>--setenv=DOTNET_MODIFIABLE_ASSEMBLIES=debug --setenv=MONO_DEBUG=gen-seq-points</WasmXHarnessMonoArgs>
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="ApplyUpdateTest.cs" />
@@ -56,6 +57,7 @@
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticField\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticField.csproj" />
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddNestedClass\System.Reflection.Metadata.ApplyUpdate.Test.AddNestedClass.csproj" />
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda.csproj" />
+    <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression.csproj" />
   </ItemGroup>
   <ItemGroup Condition="'$(TargetOS)' == 'Browser'">
     <WasmFilesToIncludeFromPublishDir Include="$(AssemblyName).dll" />

src/mono/mono/component/hot_reload.c

@@ -1176,8 +1176,7 @@ delta_info_compute_table_records (MonoImage *image_dmeta, MonoImage *image_base,
 		g_assert (table != MONO_TABLE_ENCLOG);
 		g_assert (table != MONO_TABLE_ENCMAP);
 		g_assert (table >= prev_table);
-		/* FIXME: check bounds - is it < or <=. */
-		if (rid < delta_info->count[table].prev_gen_rows) {
+		if (rid <= delta_info->count[table].prev_gen_rows) {
 			base_info->any_modified_rows[table] = TRUE;
 			delta_info->count[table].modified_rows++;
 		} else
@@ -1483,7 +1482,13 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, DeltaInfo *de
 				 * still resolves to the same MonoMethod* (but we can't check it in
 				 * pass1 because we haven't added the new AssemblyRefs yet.
 				 */
-				if (ca_base_cols [MONO_CUSTOM_ATTR_PARENT] != ca_upd_cols [MONO_CUSTOM_ATTR_PARENT]) {
+				/* NOTE: Apparently Roslyn sometimes sends NullableContextAttribute
+				 * deletions even if the ChangeCustomAttribute capability is unset.
+				 * So tacitly accept updates where a custom attribute is deleted
+				 * (its parent is set to 0).  Once we support custom attribute
+				 * changes, we will support this kind of deletion for real.
+				 */
+				if (ca_base_cols [MONO_CUSTOM_ATTR_PARENT] != ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] && ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] != 0) {
 					mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing CA table cols with a different Parent. token=0x%08x", log_token);
 					unsupported_edits = TRUE;
 					continue;
@@ -1799,6 +1804,7 @@ apply_enclog_pass2 (MonoImage *image_base, BaselineInfo *base_info, uint32_t gen
 				g_assert (add_member_klass);
 				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "Adding new method 0x%08x to class %s.%s", log_token, m_class_get_name_space (add_member_klass), m_class_get_name (add_member_klass));
 				MonoDebugInformationEnc *method_debug_information = hot_reload_get_method_debug_information (delta_info->ppdb_file, token_index);
+				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "Debug info for method 0x%08x has ppdb idx 0x%08x", log_token, method_debug_information ? method_debug_information->idx : 0);
 				add_method_to_baseline (base_info, delta_info, add_member_klass, log_token, method_debug_information);
 				add_member_klass = NULL;
 			}
@@ -1941,6 +1947,21 @@ apply_enclog_pass2 (MonoImage *image_base, BaselineInfo *base_info, uint32_t gen
 	return TRUE;
 }
 
+static void
+dump_methodbody (MonoImage *image)
+{
+	if (!mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE))
+		return;
+	MonoTableInfo *t = &image->tables [MONO_TABLE_METHODBODY];
+	uint32_t rows = table_info_get_rows (t);
+	for (uint32_t i = 0; i < rows; ++i)
+	{
+		uint32_t cols[MONO_METHODBODY_SIZE];
+		mono_metadata_decode_row (t, i, cols, MONO_METHODBODY_SIZE);
+		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, " row[%02d] = doc: 0x%08x seq: 0x%08x", i + 1, cols [MONO_METHODBODY_DOCUMENT], cols [MONO_METHODBODY_SEQ_POINTS]);
+	}
+}
+
 /**
  *
  * LOCKING: Takes the publish_lock
@@ -2002,7 +2023,10 @@ hot_reload_apply_changes (int origin, MonoImage *image_base, gconstpointer dmeta
 		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image user string size: 0x%08x", image_dpdb->heap_us.size);
 		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image blob heap addr: %p", image_dpdb->heap_blob.data);
 		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image blob heap size: 0x%08x", image_dpdb->heap_blob.size);
+		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "ppdb methodbody: ");
+		dump_methodbody (image_dpdb);
 		ppdb_file = mono_create_ppdb_file (image_dpdb, FALSE);
+		g_assert (ppdb_file->image == image_dpdb);
 	}
 
 	BaselineInfo *base_info = baseline_info_lookup_or_add (image_base);

src/mono/mono/metadata/debug-mono-ppdb.c

@@ -505,7 +505,7 @@ mono_ppdb_get_seq_points_internal (MonoImage *image, MonoPPDBFile *ppdb, MonoMet
 	docidx = cols [MONO_METHODBODY_DOCUMENT];
 
 	if (!cols [MONO_METHODBODY_SEQ_POINTS])
-		return -1;
+		return 0;
 
 	ptr = mono_metadata_blob_heap (image, cols [MONO_METHODBODY_SEQ_POINTS]);
 	size = mono_metadata_decode_blob_size (ptr, &ptr);
@@ -599,7 +599,7 @@ gboolean
 mono_ppdb_get_seq_points_enc (MonoDebugMethodInfo *minfo, MonoPPDBFile *ppdb_file, int idx, char **source_file, GPtrArray **source_file_list, int **source_files, MonoSymSeqPoint **seq_points, int *n_seq_points)
 {
 	MonoMethod *method = minfo->method;
-	if (mono_ppdb_get_seq_points_internal (ppdb_file->image, ppdb_file, method, idx, source_file, source_file_list, source_files, seq_points, n_seq_points) > 0)
+	if (mono_ppdb_get_seq_points_internal (ppdb_file->image, ppdb_file, method, idx, source_file, source_file_list, source_files, seq_points, n_seq_points) >= 0)
 		return TRUE;
 	return FALSE;
 }

src/mono/mono/metadata/metadata.c

@@ -2276,6 +2276,9 @@ mono_metadata_method_has_param_attrs (MonoImage *m, int def)
 	MonoTableInfo *methodt = &m->tables [MONO_TABLE_METHOD];
 	guint lastp, i, param_index = mono_metadata_decode_row_col (methodt, def - 1, MONO_METHOD_PARAMLIST);
 
+	if (param_index == 0)
+		return FALSE;
+
 	/* FIXME: metadata-update */
 	if (def < table_info_get_rows (methodt))
 		lastp = mono_metadata_decode_row_col (methodt, def, MONO_METHOD_PARAMLIST);

src/mono/mono/metadata/mono-debug.c

@@ -846,6 +846,10 @@ mono_debug_method_lookup_location (MonoDebugMethodInfo *minfo, int il_offset)
 			MonoDebugSourceLocation * ret = mono_ppdb_lookup_location_enc (mdie->ppdb_file, mdie->idx, il_offset);
 			if (ret)
 				return ret;
+		} else {
+			gboolean added_method = idx >= table_info_get_rows (&img->tables[MONO_TABLE_METHOD]);
+			if (added_method)
+				return NULL;
 		}
 	}
 
@@ -1144,6 +1148,25 @@ mono_debug_get_seq_points (MonoDebugMethodInfo *minfo, char **source_file, GPtrA
 			if (mono_ppdb_get_seq_points_enc (minfo, mdie->ppdb_file, mdie->idx, source_file, source_file_list, source_files, seq_points, n_seq_points))
 				return;
 		}
+		/*
+		 * dotnet watch sometimes sends us updated with PPDB deltas, but the baseline
+		 * project has debug info (and we use it for seq points?).  In tht case, just say
+		 * the added method has no sequence points.  N.B. intentionally, comparing idx to
+		 * the baseline tables.  For methods that already existed, use their old seq points.
+		 */
+		if (idx >= table_info_get_rows (&img->tables[MONO_TABLE_METHOD])) {
+			if (source_file)
+				*source_file = NULL;
+			if (source_file_list)
+				*source_file_list = NULL;
+			if (source_files)
+				*source_files = NULL;
+			if (seq_points)
+				*seq_points = NULL;
+			if (n_seq_points)
+				*n_seq_points = 0;
+			return;
+		}
 	}
 	if (minfo->handle->ppdb)
 		mono_ppdb_get_seq_points (minfo, source_file, source_file_list, source_files, seq_points, n_seq_points);