Uh oh!
There was an error while loading. Please reload this page.
[mono][debugger] Debugger method invokes dont' handle ref fields in ref structs - #76332
Conversation
ghost
commented
Sep 28, 2022
Tagging subscribers to this area: @thaystg |
lambdageek
commented
Sep 29, 2022
I'll take a look in more detail later today, but overall this looks good. |
lambdageek
left a comment
There was a problem hiding this comment.
I'm not very confident about the size calculation. I'd like to see a comment somewhere that summarizes the rules.
Uh oh!
There was an error while loading. Please reload this page.
| /* We send these as vtypes, so we get them back as such */ | ||
| g_assert (type == MONO_TYPE_VALUETYPE); | ||
| /* Fall through */ | ||
| handle_vtype: |
| *(guint8**)addr = *extra_space; | ||
| guint8 *buf_int = buf; | ||
| addr = *(guint8**)addr; |
There was a problem hiding this comment.
This is confusing. (Maybe a comment is enough).
This is what I think it's doing:
- We're going to decode a value
- The value will be placed into the extra space we allocated starting at
*extra_space. - At the beginning,
addris a pointer to some storage. We will write into that storage the address of the value we're decoding. - Then we update
addrto also point at the beginning of the extra space, because this is where the fields of the value will be stored.
Finally, we bump *extra_space so that if we need additional extra storage, it will use the extra space after the current value.
| *(guint8**)addr = *extra_space; | ||
| guint8 *buf_int = buf; | ||
| addr = *(guint8**)addr; | ||
| *extra_space += decode_value_compute_size (t, type, domain, buf_int, &buf_int, limit, from_by_ref_value_type); |
There was a problem hiding this comment.
why isn't the last argument TRUE? isn't the whole current value going into extra space?
| if (!m_type_is_byref (f->type) && !m_class_is_byreflike (mono_class_from_mono_type_internal (f->type)) && !from_by_ref_value_type) | ||
| decode_value_compute_size (f->type, 0, domain, buf, &buf, limit, FALSE); | ||
| else | ||
| ret += decode_value_compute_size (f->type, 0, domain, buf, &buf, limit, m_type_is_byref (f->type) || m_class_is_byreflike (mono_class_from_mono_type_internal (f->type))); |
There was a problem hiding this comment.
I was really confused what from_by_ref_value_type is used for.
But I think this is the only place where it actually makes a difference, right?
And its purpose is to decide if the current field needs extra space.
So we're saying - a value will need to be stored into extra space if:
- It is byref,
- or it is a
ref struct - or the current field is part of a value type that was byref in some outer struct.
It might be clearer to do something like:
/* byval members of the current field will need to be in extra_space */gbooleanmembers_in_extra_space=m_type_is_byref (f->type) ||m_class_is_byreflike (...);
/* does the current field need to go in extra_space? Yes if it's recursively in a by_ref_value_type, or if it is itself byref or a ref struct */gbooleancur_field_in_extra_space=from_by_ref_value_type||members_in_extra_space;
intfield_size=decode_value_compute_size (f->type, 0, domain, buf, &buf, limit, members_in_extra_space);
if (cur_field_in_extra_space)
ret+=field_size;And actually I think I don't understand why it's like this.
Shouldn't it be somthing like:
gbooleancur_field_in_extra_space=from_by_ref_value_type;
gbooleanmembers_in_extra_space=cur_field_in_extra_space||m_type_is_byref (f->type);
gsizefield_size=decode_value_compute_size (f->type, ..., members_in_extra_space);
if (cur_field_in_extra_space)
ret+=field_size;ie once you're in extra_space, you're stuck in extra space for all the other structs you recurse into. And additionallly if any of them have byref fields, those values need to be in extra_space too.
There was a problem hiding this comment.
Also if f->type is byref, don't you also needif (cur_field_in_extra_space) ret += sizeof(gpointer); ?
If the current field is ref double you need to store the double in extra_space and you also need to store a pointer in the current structure, I think.
thaystg
commented
Jan 2, 2023
@lambdageek when you have time can you review it again, I addressed all your comments I think :) |
lambdageek
left a comment
There was a problem hiding this comment.
is the extra condition looking at the byval_arg needed somewhere, or is it dead code from a previous revision? might be clearer to remove it for now
| #define GET_EXTRA_SPACE_FOR_REF_FIELDS(klass) \ | ||
| extra_space_size = 0; \ | ||
| extra_space = NULL; \ | ||
| if (m_class_is_valuetype (klass) && (m_type_is_byref (m_class_get_byval_arg (klass)) || m_class_is_byreflike (klass))) { \ |
There was a problem hiding this comment.
m_class_is_valuetype (klass) && (m_type_is_byref (m_class_get_byval_arg (klass)) can't be true in mono right now. the byval_arg of every MonoClass is not a ref type.
(In my ref ref C prototype from hack week a couple years ago, we added a MonoClass for byref types, so this might be ok for future-proofing, but right now it won't do anything).
Probably you need a MonoType here
| Console.WriteLine(typeof(object).Assembly.FullName); | ||
| Console.WriteLine(System.Reflection.Assembly.GetEntryAssembly ()); | ||
| Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription); | ||
| Run(); |
There was a problem hiding this comment.
@thaystg oops. I missed this during code review - I thought this was in a debugger test, not in the HelloWorld sample. Can you revert it
#75774 (comment)