Skip to content

Commit e24f66d

Browse files
lambdageeklambdageek
authored
[release/7.0-preview2][mono][hot reload] Fix sequence point logic for compiler-generated methods in updates (dotnet#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
1 parent 190a47e commit e24f66d

File tree

12 files changed

+201
-8
lines changed

12 files changed

+201
-8
lines changed

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
using System;
4+
5+
6+
namespace System.Reflection.Metadata.ApplyUpdate.Test;
7+
8+
public class StaticLambdaRegression
9+
{
10+
public int count;
11+
12+
public string TestMethod()
13+
{
14+
count++;
15+
#if false
16+
Message (static () => "hello");
17+
#endif
18+
return count.ToString();
19+
}
20+
21+
#if false
22+
public void Message (Func<string> msg) => Console.WriteLine (msg());
23+
#endif
24+
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
using System;
4+
5+
6+
namespace System.Reflection.Metadata.ApplyUpdate.Test;
7+
8+
public class StaticLambdaRegression
9+
{
10+
public int count;
11+
12+
public string TestMethod()
13+
{
14+
count++;
15+
#if true
16+
Message (static () => "hello2");
17+
#endif
18+
return count.ToString();
19+
}
20+
21+
#if true
22+
public void Message (Func<string> msg) => Console.WriteLine (msg());
23+
#endif
24+
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
using System;
4+
5+
6+
namespace System.Reflection.Metadata.ApplyUpdate.Test;
7+
8+
public class StaticLambdaRegression
9+
{
10+
public int count;
11+
12+
public string TestMethod()
13+
{
14+
count++;
15+
#if true
16+
Message (static () => "hello2");
17+
#endif
18+
Message (static () => "goodbye");
19+
return count.ToString();
20+
}
21+
22+
#if true
23+
public void Message (Func<string> msg) => Console.WriteLine (msg());
24+
#endif
25+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<RootNamespace>System.Runtime.Loader.Tests</RootNamespace>
4+
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
5+
<TestRuntime>true</TestRuntime>
6+
<DeltaScript>deltascript.json</DeltaScript>
7+
</PropertyGroup>
8+
<ItemGroup>
9+
<Compile Include="StaticLambdaRegression.cs" />
10+
</ItemGroup>
11+
</Project>

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"changes": [
3+
{"document": "StaticLambdaRegression.cs", "update": "StaticLambdaRegression_v1.cs"},
4+
{"document": "StaticLambdaRegression.cs", "update": "StaticLambdaRegression_v2.cs"},
5+
]
6+
}
7+

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,5 +395,37 @@ public static void IsSupported()
395395
bool result = MetadataUpdater.IsSupported;
396396
Assert.False(result);
397397
}
398+
399+
[ConditionalFact(typeof(ApplyUpdateUtil), nameof(ApplyUpdateUtil.IsSupported))]
400+
public static void TestStaticLambdaRegression()
401+
{
402+
ApplyUpdateUtil.TestCase(static () =>
403+
{
404+
var assm = typeof(System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression).Assembly;
405+
var x = new System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression();
406+
407+
Assert.Equal (0, x.count);
408+
409+
x.TestMethod();
410+
x.TestMethod();
411+
412+
Assert.Equal (2, x.count);
413+
414+
ApplyUpdateUtil.ApplyUpdate(assm, usePDB: false);
415+
416+
x.TestMethod();
417+
x.TestMethod();
418+
419+
Assert.Equal (4, x.count);
420+
421+
ApplyUpdateUtil.ApplyUpdate(assm, usePDB: false);
422+
423+
x.TestMethod();
424+
x.TestMethod();
425+
426+
Assert.Equal (6, x.count);
427+
428+
});
429+
}
398430
}
399431
}

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ internal static bool HasApplyUpdateCapabilities()
6666

6767
private static System.Collections.Generic.Dictionary<Assembly, int> assembly_count = new();
6868

69-
internal static void ApplyUpdate (System.Reflection.Assembly assm)
69+
internal static void ApplyUpdate (System.Reflection.Assembly assm, bool usePDB = true)
7070
{
7171
int count;
7272
if (!assembly_count.TryGetValue(assm, out count))
@@ -83,9 +83,13 @@ internal static void ApplyUpdate (System.Reflection.Assembly assm)
8383

8484
string dmeta_name = $"{basename}.{count}.dmeta";
8585
string dil_name = $"{basename}.{count}.dil";
86+
string dpdb_name = $"{basename}.{count}.dpdb";
8687
byte[] dmeta_data = System.IO.File.ReadAllBytes(dmeta_name);
8788
byte[] dil_data = System.IO.File.ReadAllBytes(dil_name);
88-
byte[] dpdb_data = null; // TODO also use the dpdb data
89+
byte[] dpdb_data = null;
90+
91+
if (usePDB)
92+
dpdb_data = System.IO.File.ReadAllBytes(dpdb_name);
8993

9094
MetadataUpdater.ApplyUpdate(assm, dmeta_data, dil_data, dpdb_data);
9195
}
@@ -94,6 +98,20 @@ internal static void AddRemoteInvokeOptions (ref RemoteInvokeOptions options)
9498
{
9599
options = options ?? new RemoteInvokeOptions();
96100
options.StartInfo.EnvironmentVariables.Add(DotNetModifiableAssembliesSwitch, DotNetModifiableAssembliesValue);
101+
/* Ask mono to use .dpdb data to generate sequence points even without a debugger attached */
102+
if (IsMonoRuntime)
103+
AppendEnvironmentVariable(options.StartInfo.EnvironmentVariables, "MONO_DEBUG", "gen-seq-points");
104+
}
105+
106+
private static void AppendEnvironmentVariable(System.Collections.Specialized.StringDictionary env, string key, string addedValue)
107+
{
108+
if (!env.ContainsKey(key))
109+
env.Add(key, addedValue);
110+
else
111+
{
112+
string oldValue = env[key];
113+
env[key] = oldValue + "," + addedValue;
114+
}
97115
}
98116

99117
/// Run the given test case, which applies updates to the given assembly.

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
<!-- Some tests rely on no deps.json file being present. -->
88
<GenerateDependencyFile>false</GenerateDependencyFile>
99
<!-- EnC tests on targets without a remote executor need the environment variable set before launching the test -->
10-
<WasmXHarnessMonoArgs>--setenv=DOTNET_MODIFIABLE_ASSEMBLIES=debug</WasmXHarnessMonoArgs>
10+
<!-- Also ask Mono to use make sequence points even without a debugger attached to match "dotnet watch" behavior -->
11+
<WasmXHarnessMonoArgs>--setenv=DOTNET_MODIFIABLE_ASSEMBLIES=debug --setenv=MONO_DEBUG=gen-seq-points</WasmXHarnessMonoArgs>
1112
</PropertyGroup>
1213
<ItemGroup>
1314
<Compile Include="ApplyUpdateTest.cs" />
@@ -56,6 +57,7 @@
5657
<ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticField\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticField.csproj" />
5758
<ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddNestedClass\System.Reflection.Metadata.ApplyUpdate.Test.AddNestedClass.csproj" />
5859
<ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda.csproj" />
60+
<ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression.csproj" />
5961
</ItemGroup>
6062
<ItemGroup Condition="'$(TargetOS)' == 'Browser'">
6163
<WasmFilesToIncludeFromPublishDir Include="$(AssemblyName).dll" />

‎src/mono/mono/component/hot_reload.c

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1176,8 +1176,7 @@ delta_info_compute_table_records (MonoImage *image_dmeta, MonoImage *image_base,
11761176
g_assert (table != MONO_TABLE_ENCLOG);
11771177
g_assert (table != MONO_TABLE_ENCMAP);
11781178
g_assert (table >= prev_table);
1179-
/* FIXME: check bounds - is it < or <=. */
1180-
if (rid < delta_info->count[table].prev_gen_rows) {
1179+
if (rid <= delta_info->count[table].prev_gen_rows) {
11811180
base_info->any_modified_rows[table] = TRUE;
11821181
delta_info->count[table].modified_rows++;
11831182
} else
@@ -1483,7 +1482,13 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, DeltaInfo *de
14831482
* still resolves to the same MonoMethod* (but we can't check it in
14841483
* pass1 because we haven't added the new AssemblyRefs yet.
14851484
*/
1486-
if (ca_base_cols [MONO_CUSTOM_ATTR_PARENT] != ca_upd_cols [MONO_CUSTOM_ATTR_PARENT]) {
1485+
/* NOTE: Apparently Roslyn sometimes sends NullableContextAttribute
1486+
* deletions even if the ChangeCustomAttribute capability is unset.
1487+
* So tacitly accept updates where a custom attribute is deleted
1488+
* (its parent is set to 0). Once we support custom attribute
1489+
* changes, we will support this kind of deletion for real.
1490+
*/
1491+
if (ca_base_cols [MONO_CUSTOM_ATTR_PARENT] != ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] && ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] != 0) {
14871492
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);
14881493
unsupported_edits = TRUE;
14891494
continue;
@@ -1799,6 +1804,7 @@ apply_enclog_pass2 (MonoImage *image_base, BaselineInfo *base_info, uint32_t gen
17991804
g_assert (add_member_klass);
18001805
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));
18011806
MonoDebugInformationEnc *method_debug_information = hot_reload_get_method_debug_information (delta_info->ppdb_file, token_index);
1807+
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);
18021808
add_method_to_baseline (base_info, delta_info, add_member_klass, log_token, method_debug_information);
18031809
add_member_klass = NULL;
18041810
}
@@ -1941,6 +1947,21 @@ apply_enclog_pass2 (MonoImage *image_base, BaselineInfo *base_info, uint32_t gen
19411947
return TRUE;
19421948
}
19431949

1950+
static void
1951+
dump_methodbody (MonoImage *image)
1952+
{
1953+
if (!mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE))
1954+
return;
1955+
MonoTableInfo *t = &image->tables [MONO_TABLE_METHODBODY];
1956+
uint32_t rows = table_info_get_rows (t);
1957+
for (uint32_t i = 0; i < rows; ++i)
1958+
{
1959+
uint32_t cols[MONO_METHODBODY_SIZE];
1960+
mono_metadata_decode_row (t, i, cols, MONO_METHODBODY_SIZE);
1961+
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]);
1962+
}
1963+
}
1964+
19441965
/**
19451966
*
19461967
* LOCKING: Takes the publish_lock
@@ -2002,7 +2023,10 @@ hot_reload_apply_changes (int origin, MonoImage *image_base, gconstpointer dmeta
20022023
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image user string size: 0x%08x", image_dpdb->heap_us.size);
20032024
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image blob heap addr: %p", image_dpdb->heap_blob.data);
20042025
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "pdb image blob heap size: 0x%08x", image_dpdb->heap_blob.size);
2026+
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "ppdb methodbody: ");
2027+
dump_methodbody (image_dpdb);
20052028
ppdb_file = mono_create_ppdb_file (image_dpdb, FALSE);
2029+
g_assert (ppdb_file->image == image_dpdb);
20062030
}
20072031

20082032
BaselineInfo *base_info = baseline_info_lookup_or_add (image_base);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,7 @@ mono_ppdb_get_seq_points_internal (MonoImage *image, MonoPPDBFile *ppdb, MonoMet
505505
docidx = cols [MONO_METHODBODY_DOCUMENT];
506506

507507
if (!cols [MONO_METHODBODY_SEQ_POINTS])
508-
return -1;
508+
return 0;
509509

510510
ptr = mono_metadata_blob_heap (image, cols [MONO_METHODBODY_SEQ_POINTS]);
511511
size = mono_metadata_decode_blob_size (ptr, &ptr);
@@ -599,7 +599,7 @@ gboolean
599599
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)
600600
{
601601
MonoMethod *method = minfo->method;
602-
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)
602+
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)
603603
return TRUE;
604604
return FALSE;
605605
}

‎src/mono/mono/metadata/metadata.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2276,6 +2276,9 @@ mono_metadata_method_has_param_attrs (MonoImage *m, int def)
22762276
MonoTableInfo *methodt = &m->tables [MONO_TABLE_METHOD];
22772277
guint lastp, i, param_index = mono_metadata_decode_row_col (methodt, def - 1, MONO_METHOD_PARAMLIST);
22782278

2279+
if (param_index == 0)
2280+
return FALSE;
2281+
22792282
/* FIXME: metadata-update */
22802283
if (def < table_info_get_rows (methodt))
22812284
lastp = mono_metadata_decode_row_col (methodt, def, MONO_METHOD_PARAMLIST);

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -846,6 +846,10 @@ mono_debug_method_lookup_location (MonoDebugMethodInfo *minfo, int il_offset)
846846
MonoDebugSourceLocation * ret = mono_ppdb_lookup_location_enc (mdie->ppdb_file, mdie->idx, il_offset);
847847
if (ret)
848848
return ret;
849+
} else {
850+
gboolean added_method = idx >= table_info_get_rows (&img->tables[MONO_TABLE_METHOD]);
851+
if (added_method)
852+
return NULL;
849853
}
850854
}
851855

@@ -1144,6 +1148,25 @@ mono_debug_get_seq_points (MonoDebugMethodInfo *minfo, char **source_file, GPtrA
11441148
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))
11451149
return;
11461150
}
1151+
/*
1152+
* dotnet watch sometimes sends us updated with PPDB deltas, but the baseline
1153+
* project has debug info (and we use it for seq points?). In tht case, just say
1154+
* the added method has no sequence points. N.B. intentionally, comparing idx to
1155+
* the baseline tables. For methods that already existed, use their old seq points.
1156+
*/
1157+
if (idx >= table_info_get_rows (&img->tables[MONO_TABLE_METHOD])) {
1158+
if (source_file)
1159+
*source_file = NULL;
1160+
if (source_file_list)
1161+
*source_file_list = NULL;
1162+
if (source_files)
1163+
*source_files = NULL;
1164+
if (seq_points)
1165+
*seq_points = NULL;
1166+
if (n_seq_points)
1167+
*n_seq_points = 0;
1168+
return;
1169+
}
11471170
}
11481171
if (minfo->handle->ppdb)
11491172
mono_ppdb_get_seq_points (minfo, source_file, source_file_list, source_files, seq_points, n_seq_points);

0 commit comments

Comments
 (0)