[hot_reload] Remove pass1 (#95130)

* [hot_reload] Remove pass1

The job of pass1 was to validate that the EnC delta had the expected
structure.
This was particularly useful before Roslyn added the capabilities API
and it could potentially send us changes that we weren't able to
handle yet.
In some cases we were actually too strict and disallowed benign
edits (that sometimes happen because it is easier for roslyn to send
an update that has no effect than to compute how to leave it out).
But now Mono is in lock-step with all supported edits, so pass1 is not
necessary.  Pass2 still has some structural checks to validate some of our
assumptions, but we don't expect them to be violated in any supported edits.

Fixes #90153

Author: lambdageek

src/mono/mono/component/hot_reload.c

@@ -1463,245 +1463,27 @@ prepare_mutated_rows (const MonoTableInfo *table_enclog, MonoImage *image_base,
 	}
 }
 
-/* Run some sanity checks first. If we detect unsupported scenarios, this
- * function will fail and the metadata update should be aborted. This should
- * run before anything in the metadata world is updated. */
+/*
+ * Returns TRUE if modifications or additions to the table imply that the execution engine should
+ * invalidate existing methods. It's better to be pessimistic (say TRUE more often) - most EnC
+ * deltas will lead to a change in behavior.
+ */
 static gboolean
-apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, DeltaInfo *delta_info, gconstpointer dil_data, uint32_t dil_length, gboolean *should_invalidate_transformed_code, MonoError *error)
+table_should_invalidate_transformed_code (int token_table)
 {
-	*should_invalidate_transformed_code = false;
-	MonoTableInfo *table_enclog = &image_dmeta->tables [MONO_TABLE_ENCLOG];
-	guint32 rows = table_info_get_rows (table_enclog);
-
-	gboolean unsupported_edits = FALSE;
-
-	/* hack: make a pass over it, looking only for table method updates, in
-	 * order to give more meaningful error messages first */
-
-	for (guint32 i = 0; i < rows ; ++i) {
-		guint32 cols [MONO_ENCLOG_SIZE];
-		mono_metadata_decode_row (table_enclog, i, cols, MONO_ENCLOG_SIZE);
-
-		// FIXME: the top bit 0x8000000 of log_token is some kind of
-		// indicator see IsRecId in metamodelrw.cpp and
-		// MDInternalRW::EnumDeltaTokensInit which skips over those
-		// records when EditAndContinueModule::ApplyEditAndContinue is
-		// iterating.
-		int log_token = cols [MONO_ENCLOG_TOKEN];
-		int func_code = cols [MONO_ENCLOG_FUNC_CODE];
-
-		int token_table = mono_metadata_token_table (log_token);
-		guint32 token_index = mono_metadata_token_index (log_token);
-
-		gboolean is_addition = token_index-1 >= delta_info->count[token_table].prev_gen_rows ;
-
-
-		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x (%s idx=0x%02x) (base table has 0x%04x rows; prev gen had 0x%04x rows)\t%s\tfunc=0x%02x (\"%s\")\n", i, log_token, mono_meta_table_name (token_table), token_index, table_info_get_rows (&image_base->tables [token_table]), delta_info->count[token_table].prev_gen_rows, (is_addition ? "ADD" : "UPDATE"), func_code, funccode_to_str (func_code));
-
-
-		if (token_table != MONO_TABLE_METHOD)
-			continue;
-
-		/* adding a new parameter to a new method is ok */
-		if (func_code == ENC_FUNC_ADD_PARAM && is_addition)
-			continue;
-
-		g_assert (func_code == 0); /* anything else doesn't make sense here */
-	}
-
-	for (guint32 i = 0; i < rows ; ++i) {
-		guint32 cols [MONO_ENCLOG_SIZE];
-		mono_metadata_decode_row (table_enclog, i, cols, MONO_ENCLOG_SIZE);
-
-		int log_token = cols [MONO_ENCLOG_TOKEN];
-		int func_code = cols [MONO_ENCLOG_FUNC_CODE];
-
-		int token_table = mono_metadata_token_table (log_token);
-		guint32 token_index = mono_metadata_token_index (log_token);
-
-		gboolean is_addition = token_index-1 >= delta_info->count[token_table].prev_gen_rows ;
-
-		switch (token_table) {
-		case MONO_TABLE_ASSEMBLYREF:
-			/* okay, supported */
-			break;
-		case MONO_TABLE_METHOD:
-			*should_invalidate_transformed_code = true;
-			if (func_code == ENC_FUNC_ADD_PARAM)
-				continue; /* ok, allowed */
-			/* handled above */
-			break;
-		case MONO_TABLE_FIELD:
-			/* ok */
-			g_assert (func_code == ENC_FUNC_DEFAULT);
-			break;
-		case MONO_TABLE_PROPERTYMAP: {
-			*should_invalidate_transformed_code = true;
-			if (func_code == ENC_FUNC_ADD_PROPERTY) {
-				g_assert (i + 1 < rows);
-				i++; /* skip the next record */
-				continue;
-			}
-			if (!is_addition) {
-				mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token);
-				mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token);
-				unsupported_edits = TRUE;
-				continue;
-			}
-			/* new rows, ok */
-			break;
-		}
-		case MONO_TABLE_PROPERTY: {
-			/* ok */
-			*should_invalidate_transformed_code = true;
-			g_assert (func_code == ENC_FUNC_DEFAULT);
-			break;
-		}
-		case MONO_TABLE_EVENTMAP: {
-			*should_invalidate_transformed_code = true;
-			if (func_code == ENC_FUNC_ADD_EVENT) {
-				g_assert (i + 1 < rows);
-				i++; /* skip the next record */
-				continue;
-			}
-			if (!is_addition) {
-				mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token);
-				mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token);
-				unsupported_edits = TRUE;
-				continue;
-			}
-			/* new rows, ok */
-			break;
-		}
-		case MONO_TABLE_METHODSEMANTICS: {
-			*should_invalidate_transformed_code = true;
-			if (is_addition) {
-				/* new rows are fine */
-				break;
-			} else {
-				mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token);
-				mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token);
-				unsupported_edits = TRUE;
-				continue;
-			}
-		}
-		case MONO_TABLE_CUSTOMATTRIBUTE: {
-			*should_invalidate_transformed_code = true;
-			if (!is_addition) {
-				/* modifying existing rows is ok, as long as the parent and ctor are the same */
-				guint32 ca_upd_cols [MONO_CUSTOM_ATTR_SIZE];
-				guint32 ca_base_cols [MONO_CUSTOM_ATTR_SIZE];
-				int mapped_token = hot_reload_relative_delta_index (image_dmeta, delta_info, mono_metadata_make_token (token_table, token_index));
-				g_assert (mapped_token != -1);
-				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x CUSTOM_ATTR update.  mapped index = 0x%08x\n", i, log_token, mapped_token);
-
-				mono_metadata_decode_row (&image_dmeta->tables [MONO_TABLE_CUSTOMATTRIBUTE], mapped_token - 1, ca_upd_cols, MONO_CUSTOM_ATTR_SIZE);
-				mono_metadata_decode_row (&image_base->tables [MONO_TABLE_CUSTOMATTRIBUTE], token_index - 1, ca_base_cols, MONO_CUSTOM_ATTR_SIZE);
-
-				/* compare the ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] to ca_base_cols [MONO_CUSTOM_ATTR_PARENT]. */
-				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x CUSTOM_ATTR update. Old Parent 0x%08x New Parent 0x%08x\n", i, log_token, ca_base_cols [MONO_CUSTOM_ATTR_PARENT], ca_upd_cols [MONO_CUSTOM_ATTR_PARENT]);
-				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x CUSTOM_ATTR update. Old ctor 0x%08x New ctor 0x%08x\n", i, log_token, ca_base_cols [MONO_CUSTOM_ATTR_TYPE], ca_upd_cols [MONO_CUSTOM_ATTR_TYPE]);
-
-				/* TODO: when we support the ChangeCustomAttribute capability, the
-				 * parent might become 0 to delete attributes.  It may also be the
-				 * case that the MONO_CUSTOM_ATTR_TYPE will change.  Without that
-				 * capability, we trust that if the TYPE is not the same token, it
-				 * still resolves to the same MonoMethod* (but we can't check it in
-				 * pass1 because we haven't added the new AssemblyRefs yet.
-				 */
-				/* 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.
-				 */
-				/* FIXME: https://github.com/dotnet/roslyn/issues/60125
-				 * Roslyn seems to emit CA rows out of order which puts rows with unexpected Parent values in the wrong place.
-				 */
-#ifdef DISALLOW_BROKEN_PARENT
-				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;
-				}
-#endif
-				break;
-			} else  {
-				/* Added a row. ok */
-				break;
-			}
-		}
-		case MONO_TABLE_PARAM: {
-			*should_invalidate_transformed_code = true;
-			break;
-		}
-		case MONO_TABLE_TYPEDEF: {
-			*should_invalidate_transformed_code = true;
-			if (func_code == ENC_FUNC_ADD_METHOD) {
-				/* next record should be a MONO_TABLE_METHOD addition (func == default) */
-				g_assert (i + 1 < rows);
-				guint32 next_cols [MONO_ENCLOG_SIZE];
-				mono_metadata_decode_row (table_enclog, i + 1, next_cols, MONO_ENCLOG_SIZE);
-				g_assert (next_cols [MONO_ENCLOG_FUNC_CODE] == ENC_FUNC_DEFAULT);
-				int next_token = next_cols [MONO_ENCLOG_TOKEN];
-				int next_table = mono_metadata_token_table (next_token);
-				guint32 next_index = mono_metadata_token_index (next_token);
-				g_assert (next_table == MONO_TABLE_METHOD);
-				/* expecting an added method */
-				g_assert (next_index-1 >= delta_info->count[next_table].prev_gen_rows);
-				i++; /* skip the next record */
-				continue;
-			}
-
-			if (func_code == ENC_FUNC_ADD_FIELD) {
-				mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x AddField to klass 0x%08x, skipping next EnClog record", i, log_token, token_index);
-				g_assert (i + 1 < rows);
-				guint32 next_cols [MONO_ENCLOG_SIZE];
-				mono_metadata_decode_row (table_enclog, i + 1, next_cols, MONO_ENCLOG_SIZE);
-				g_assert (next_cols [MONO_ENCLOG_FUNC_CODE] == ENC_FUNC_DEFAULT);
-				int next_token = next_cols [MONO_ENCLOG_TOKEN];
-				int next_table = mono_metadata_token_table (next_token);
-				guint32 next_index = mono_metadata_token_index (next_token);
-				g_assert (next_table == MONO_TABLE_FIELD);
-				/* expecting an added field */
-				g_assert (next_index-1 >= delta_info->count[next_table].prev_gen_rows);
-				i++; /* skip the next record */
-				continue;
-			}
-			// don't expect to see any other func codes with this table
-			g_assert (func_code == ENC_FUNC_DEFAULT);
-			// If it's an addition, it's an added type definition, continue.
-
-			// If it's a modification, Roslyn sometimes sends this when a custom
-			// attribute is deleted from a type definition.
-			break;
-		}
-		default:
-			if (!is_addition) {
-				mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token);
-				mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token);
-				unsupported_edits = TRUE;
-				continue;
-			}
-		}
-
-
-		/*
-		 * So the way a non-default func_code works is that it's attached to the EnCLog
-		 * record preceding the new member definition (so e.g. an addMethod code will be on
-		 * the preceding MONO_TABLE_TYPEDEF enc record that identifies the parent type).
-		 */
-		switch (func_code) {
-			case ENC_FUNC_DEFAULT: /* default */
-				break;
-			default:
-				mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x FunCode %d (%s) not supported (token=0x%08x)", i, log_token, func_code, funccode_to_str (func_code), log_token);
-				mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: FuncCode %d (%s) not supported (token=0x%08x)", func_code, funccode_to_str (func_code), log_token);
-				unsupported_edits = TRUE;
-				continue;
-		}
+	switch (token_table) {
+	case MONO_TABLE_METHOD:
+	case MONO_TABLE_PROPERTYMAP:
+	case MONO_TABLE_PROPERTY:
+	case MONO_TABLE_EVENTMAP:
+	case MONO_TABLE_METHODSEMANTICS:
+	case MONO_TABLE_CUSTOMATTRIBUTE:
+	case MONO_TABLE_PARAM:
+	case MONO_TABLE_TYPEDEF:
+		return TRUE;
+	default:
+		return FALSE;
 	}
-	return !unsupported_edits;
 }
 
 static void
@@ -2052,9 +1834,14 @@ pass2_update_nested_classes (Pass2Context *ctx, MonoImage *image_base, MonoError
 
 }
 
-/* do actual enclog application */
+/*
+ * Apply the entries from the EnC log to the runtime type system (as opposed to just mutating the
+ * metadata table contents).  This is "pass2" because historically there was a "pass1" that just
+ * validated the EnC log to ensure that Mono didn't see any changes that it was not prepared to
+ * handle yet.  Pass1 was non-destructive, while Pass2 actually makes changes that are hard to undo.
+ */
 static gboolean
-apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base_info, uint32_t generation, MonoImage *image_dmeta, DeltaInfo *delta_info, gconstpointer dil_data, uint32_t dil_length, MonoError *error)
+apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base_info, uint32_t generation, MonoImage *image_dmeta, DeltaInfo *delta_info, gconstpointer dil_data, uint32_t dil_length, gboolean *should_invalidate_transformed_code, MonoError *error)
 {
 	MonoTableInfo *table_enclog = &image_dmeta->tables [MONO_TABLE_ENCLOG];
 	guint32 rows = table_info_get_rows (table_enclog);
@@ -2102,6 +1889,8 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 
 		gboolean is_addition = token_index-1 >= delta_info->count[token_table].prev_gen_rows ;
 
+		*should_invalidate_transformed_code |= table_should_invalidate_transformed_code (token_table);
+		
 		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "enclog i=%d: token=0x%08x (table=%s): %d:\t%s", i, log_token, mono_meta_table_name (token_table), func_code, (is_addition ? "ADD" : "UPDATE"));
 
 
@@ -2140,7 +1929,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 		}
 
 		default:
-			g_error ("EnC: unsupported FuncCode, should be caught by pass1");
+			g_error ("EnC: unsupported FuncCode");
 			break;
 		}
 
@@ -2199,7 +1988,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 				delta_info->method_ppdb_table_update = g_hash_table_new (g_direct_hash, g_direct_equal);
 
 			if (is_addition) {
-				g_assertf (add_member_typedef, "EnC: new method added but I don't know the class, should be caught by pass1");
+				g_assertf (add_member_typedef, "EnC: malformed EnC delta, new method added but I don't know the class");
 				if (pass2_context_is_skeleton (ctx, add_member_typedef)) {
 					mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "Adding new method 0x%08x to new class 0x%08x", log_token, add_member_typedef);
 					pass2_context_add_skeleton_member (ctx, add_member_typedef, log_token);
@@ -2438,11 +2227,9 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 			break;
 		}
 		case MONO_TABLE_CUSTOMATTRIBUTE: {
-			/* ok, pass1 checked for disallowed modifications */
 			break;
 		}
 		case MONO_TABLE_PARAM: {
-			/* ok, pass1 checked for disallowed modifications */
 			/* if there were multiple added methods, this comes in as several method
 			 * additions, followed by the parameter additions.
 			 *
@@ -2483,7 +2270,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 		}
 		case MONO_TABLE_METHODSEMANTICS: {
 			g_assert (is_addition);
-			/* added rows ok (for new or existing classes). modifications rejected by pass1 */
+			/* added rows ok (for new or existing classes). don't expect modifications */
 			uint32_t sema_cols[MONO_METHOD_SEMA_SIZE];
 
 			uint32_t mapped_token = hot_reload_relative_delta_index (image_dmeta, delta_info, log_token);
@@ -2659,8 +2446,6 @@ hot_reload_apply_changes (int origin, MonoImage *image_base, gconstpointer dmeta
 
 	/* Process EnCMap and compute number of added/modified rows from this
 	 * delta.  This enables computing row indexes relative to the delta.
-	 * We use it in pass1 to bail out early if the EnCLog has unsupported
-	 * edits.
 	 */
 	if (!delta_info_compute_table_records (image_dmeta, image_base, base_info, delta_info)) {
 		mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "Error on computing delta table info (base=%s)", basename);
@@ -2674,13 +2459,6 @@ hot_reload_apply_changes (int origin, MonoImage *image_base, gconstpointer dmeta
 
 	mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "Populated mutated tables for delta image %p", image_dmeta);
 
-	gboolean should_invalidate_transformed_code = false;
-	if (!apply_enclog_pass1 (image_base, image_dmeta, delta_info, dil_bytes, dil_length, &should_invalidate_transformed_code, error)) {
-		mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "Error on sanity-checking delta image to base=%s, due to: %s", basename, mono_error_get_message (error));
-		hot_reload_update_cancel (generation);
-		return;
-	}
-
 	/* if there are updates, start tracking the tables of the base image, if we weren't already. */
 	if (table_info_get_rows (table_enclog))
 		table_to_image_add (image_base);
@@ -2691,9 +2469,10 @@ hot_reload_apply_changes (int origin, MonoImage *image_base, gconstpointer dmeta
 	if (mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE))
 		dump_update_summary (image_base, image_dmeta);
 
+	gboolean should_invalidate_transformed_code = FALSE;
 	Pass2Context pass2ctx = {0,};
 	pass2_context_init (&pass2ctx);
-	if (!apply_enclog_pass2 (&pass2ctx, image_base, base_info, generation, image_dmeta, delta_info, dil_bytes, dil_length, error)) {
+	if (!apply_enclog_pass2 (&pass2ctx, image_base, base_info, generation, image_dmeta, delta_info, dil_bytes, dil_length, &should_invalidate_transformed_code, error)) {
 		pass2_context_destroy (&pass2ctx);
 		mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "Error applying delta image to base=%s, due to: %s", basename, mono_error_get_message (error));
 		hot_reload_update_cancel (generation);