Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono][interp] Add a bitset with IL jump targets #80136

Merged
merged 7 commits into from
Jan 11, 2023

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Jan 3, 2023

This PR introduces a bitset that keeps track of IL jump targets and throws an InvalidProgramException if a jump target is in the middle of an opcode or out of range. Function get_basic_blocks marks all jump targets in the bitset, while function generate_code checks if an opcode offset is marked as a jump target.

Fix for #54396.

@ghost
Copy link

ghost commented Jan 3, 2023

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces a bitset that keeps track of IL jump targets and throws an InvalidProgramException if a jump target is in the middle of an opcode or out of range. Function get_basic_blocks marks all jump targets in the bitset, while function generate_code checks if an opcode offset is marked as a jump target.

Fix for #54396

Draft PR should confirm if the runtime works as expected.

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@build-analysis build-analysis bot mentioned this pull request Jan 4, 2023
@kotlarmilos kotlarmilos marked this pull request as ready for review January 4, 2023 07:44
@kotlarmilos kotlarmilos changed the title [WIP][mono][interp] Add a bitset with IL jump targets [mono][interp] Add a bitset with IL jump targets Jan 4, 2023
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the method execution continues here if we detected error ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should check error status here

Copy link
Member Author

@kotlarmilos kotlarmilos Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here is not guaranteed to run, it should reside at the exit lable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to exit label.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@kotlarmilos kotlarmilos Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

get_bb (td, start + c->data.filter_offset, make_list);
mono_bitset_set(il_targets, c->data.filter_offset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

get_bb (td, target, make_list);
ip += 4;
mono_bitset_set(il_targets, target - start);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before (, maybe we coud split this sausage

@kotlarmilos
Copy link
Member Author

Builds canceled after timeout don't look related to the changes.

@kotlarmilos kotlarmilos merged commit 50b79a6 into dotnet:main Jan 11, 2023
@kotlarmilos kotlarmilos deleted the issue-54396 branch January 11, 2023 20:06
@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants