[mono][interp] Add a bitset with IL jump targets #80136
Conversation
|
Tagging subscribers to this area: @BrzVlad Issue DetailsThis PR introduces a bitset that keeps track of IL jump targets and throws an Fix for #54396 Draft PR should confirm if the runtime works as expected.
|
kotlarmilos
changed the title
| @@ -3737,12 +3737,29 @@ get_basic_blocks (TransformData *td, MonoMethodHeader *header, gboolean make_lis | |||
|
|
|||
| for (guint i = 0; i < header->num_clauses; i++) { | |||
| MonoExceptionClause *c = header->clauses + i; | |||
| if (start + c->try_offset <= 0) { | |||
There was a problem hiding this comment.
The pointer arithmetic around here looks redundant. If you really want to do checks for these offsets, it would make more sense to check them against the code_size directly. Note that all offsets in MonoExceptionClause are unsigned. This comparison that a pointer is negative doesn't make sense to me, I don't think this code should compile.
There was a problem hiding this comment.
Make sense. As offsets are unsigned, only checks against the code_size are in place.
| get_bb (td, target, make_list); | ||
| ip += 2; | ||
| get_bb (td, ip, make_list); | ||
| mono_bitset_set(il_targets, target - start); |
| @@ -3770,28 +3787,43 @@ get_basic_blocks (TransformData *td, MonoMethodHeader *header, gboolean make_lis | |||
| break; | |||
| case MonoShortInlineBrTarget: | |||
| target = start + cli_addr + 2 + (signed char)ip [1]; | |||
| if (target >= end) { | |||
| mono_error_set_generic_error (error, "System", "InvalidProgramException", "Jump target out of range at offset %04tx", cli_addr); | |||
There was a problem hiding this comment.
Why does the method execution continues here if we detected error ?
There was a problem hiding this comment.
get_basic_blocks function is updated to return gboolean. It terminates the execution if invalid code is detected.
| @@ -4554,7 +4587,8 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, | |||
| exit_bb->stack_height = -1; | |||
| } | |||
|
|
|||
| get_basic_blocks (td, header, td->gen_sdb_seq_points); | |||
| il_targets = mono_bitset_mem_new(mono_mempool_alloc0(td->mempool, mono_bitset_alloc_size(header->code_size, 0)), header->code_size, 0); | |||
| get_basic_blocks (td, header, td->gen_sdb_seq_points, il_targets, error); | |||
There was a problem hiding this comment.
we should check error status here
There was a problem hiding this comment.
if get_basic_blocks return FALSE, td->has_invalid_code is set to TRUE and execution is terminated.
| @@ -7841,6 +7882,7 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, | |||
| } | |||
|
|
|||
| g_assert (td->ip == end); | |||
| mono_bitset_free(il_targets); | |||
There was a problem hiding this comment.
The code here is not guaranteed to run, it should reside at the exit lable.
| break; | ||
| case MonoInlineBrTarget: | ||
| target = start + cli_addr + 5 + (gint32)read32 (ip + 1); | ||
| if (target >= end) { | ||
| mono_error_set_generic_error (error, "System", "InvalidProgramException", "Jump target out of range at offset %04tx", cli_addr); |
There was a problem hiding this comment.
These exceptions are super uncommon. Detailed exception information about them seems like a waste of code size. We can just set td->has_invalid_code instead.
There was a problem hiding this comment.
td->has_invalid_code is used instead of mono_error_set_generic_error on all occasions.
| /* Checks that a jump target isn't in the middle of opcode offset */ | ||
| int op_size = mono_opcode_size (td->ip, end); | ||
| for (int i = 1; i < op_size; i++) | ||
| { |
There was a problem hiding this comment.
{ should be on the same line as for
…is detected. Use td->has_invalid_code instead of mono_error_set_generic_error.
| @@ -7869,6 +7905,7 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, | |||
| exit_ret: | |||
| g_free (arg_locals); | |||
| g_free (local_locals); | |||
| mono_bitset_free(il_targets); | |||
| get_bb (td, start + c->try_offset, make_list); | ||
| mono_bitset_set(il_targets, c->try_offset); | ||
| mono_bitset_set(il_targets, c->try_offset + c->try_len); |
| get_bb (td, start + c->handler_offset, make_list); | ||
| if (c->flags == MONO_EXCEPTION_CLAUSE_FILTER) | ||
| mono_bitset_set(il_targets, c->handler_offset); | ||
| mono_bitset_set(il_targets, c->handler_offset + c->handler_len); |
| get_bb (td, start + c->data.filter_offset, make_list); | ||
| mono_bitset_set(il_targets, c->data.filter_offset); |
| get_bb (td, target, make_list); | ||
| ip += 4; | ||
| mono_bitset_set(il_targets, target - start); |
| @@ -4554,7 +4579,11 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, | |||
| exit_bb->stack_height = -1; | |||
| } | |||
|
|
|||
| get_basic_blocks (td, header, td->gen_sdb_seq_points); | |||
| il_targets = mono_bitset_mem_new(mono_mempool_alloc0(td->mempool, mono_bitset_alloc_size(header->code_size, 0)), header->code_size, 0); | |||
There was a problem hiding this comment.
space before (, maybe we coud split this sausage
|
Builds canceled after timeout don't look related to the changes. |
ghost
locked as resolved and limited conversation to collaborators
This PR introduces a bitset that keeps track of IL jump targets and throws an
InvalidProgramExceptionif a jump target is in the middle of an opcode or out of range. Functionget_basic_blocksmarks all jump targets in the bitset, while functiongenerate_codechecks if an opcode offset is marked as a jump target.Fix for #54396.