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][debugger] Support symbol server on mobile devices #82555

Merged
merged 7 commits into from
Feb 24, 2023

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Feb 23, 2023

I need these informations to correct create the URL to download symbols from symbol server.

I created a new message to avoid passing all the assembly content over the protocol and read it using PEReader which would be easier, but it would spend more time and more memory on client side also.

Related to PRs on debugger-libs and debugger-vs repo:
https://github.com/xamarin/debugger-vs/pull/315
mono/debugger-libs#382

JustMyCode_SylbolServer_SourceLink_maui.mp4

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM. But we should probably check the bounds of the PDB data. And also I'm not sure if it's ok to reuse a command number even if you bump the protocol version.

src/mono/mono/component/debugger-protocol.h Outdated Show resolved Hide resolved
{
data = (guint8 *) (image->raw_data + debug_dir.pointer);
char* alg_name = (char*)data;
guint8* checksum = (guint8 *) (data + strlen(alg_name)+ 1);
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a bounds-check here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, this data will be used on buffer_add_string which will access until find a \0, right?

Copy link
Member Author

@thaystg thaystg Feb 23, 2023

Choose a reason for hiding this comment

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

Do you mean a case where there isn't a \0 and buffer_add_string will access it and pass the end of the debug info?
I'm not sure if it's possible but I can check if you think we should :)

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean a case where there isn't a \0 and buffer_add_string will access it and pass the end of the debug info?

Yes, that's what I mean.

I'm not sure if it's possible but I can check if you think we should :)

I'd feel better if we had a bounds check :)

@@ -96,6 +105,8 @@ get_pe_debug_info (MonoImage *image, guint8 *out_guid, gint32 *out_age, gint32 *
if (dir.signature == 0x53445352) {
memcpy (out_guid, data + 4, 16);
*out_age = read32(data + 20);
if (pdb_path)
*pdb_path = (char*) data + 24;
Copy link
Member

Choose a reason for hiding this comment

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

Also here - should we try to find the end of the string and make sure it doesn't go past the end of the debug info

@@ -74,4 +74,7 @@ mono_ppdb_is_embedded (MonoPPDBFile *ppdb);
MONO_COMPONENT_API MonoPPDBFile*
mono_create_ppdb_file (MonoImage *ppdb_image, gboolean is_embedded_ppdb);

MONO_COMPONENT_API gboolean
get_pe_debug_info_full (MonoImage *image, guint8 *out_guid, gint32 *out_age, gint32 *out_timestamp, guint8 **ppdb_data,
Copy link
Member

@akoeplinger akoeplinger Feb 24, 2023

Choose a reason for hiding this comment

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

does this need to be MONO_COMPONENT_API ? ah yeah it does. we usually prefix Mono APIs with mono_

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Addressed in this PR: #82587

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants