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

GdbServer: Support 32-bit context definitions #4205

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

Sonicadvance1
Copy link
Member

Requires restructuring a couple of things, but nothing too crazy here.

First commit is from #4203

@Sonicadvance1 Sonicadvance1 force-pushed the gdbserver_support_32bit branch from 1c35755 to af0b1af Compare December 10, 2024 00:47
Comment on lines 133 to 134
GPRType gregs[GPRCount];
GPRType rip;
Copy link
Member

Choose a reason for hiding this comment

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

Making all this compile-time-based increases the code complexity here a lot. Have you considered using a runtime-based approach via std::span instead?

bool is_64_bit;
GPRType gregs_storage[NUM_GPRS];
std::span gregs(gregs_storage, is_64_bit ? NUM_GPRS : NUM_GPRS / 2);

AFAICS we only need binary serialization in one place. We can do a little more work manually there, at the benefit of saving all the template ugliness this change currently adds (most notably GenerateAndFetchContextDefinition.template operator()<GDBContextDefinition>...).

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 don't see what you are intending here? We give the connected gdb instance a layout that is defined via BuildTargetXML and we need to ensure that the resulting binary that we encode and send over the pipe matches that exact struct layout, which differs between x86-64 and i386.

This is probably just me being daft since I don't quite understand what the intended change does.

Copy link
Member

Choose a reason for hiding this comment

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

we need to ensure that the resulting binary that we encode and send over the pipe matches that exact struct layout, which differs between x86-64 and i386.

Yes, but we aren't required to use the exact same binary structure internally. IIUC the binary layout of GDBContextDefinition(_32) only matters in CommandReadRegisters, where we serialize using encodeHex. If we do this serialization manually instead, we can merge GDBContextDefinition and GDBContextDefinition_32 into a single, non-template struct.

This would require boilerplate similar to what's in CommandReadReg now, so it could even make sense to go one step further and drop the use of structs altogether and describe the GDBContextDefinition data as a raw memory block with a fextl::vector-based array that dynamically describes each member.

I'm not strongly attached to any particular way of going about this, but the template approach seems overly clever when runtime-based approaches would produce code that's simpler to reason about.

Copy link
Member Author

Choose a reason for hiding this comment

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

The struct definition is technically used in both CommandReadRegisters and CommandReadReg in different ways.
But I've removed any amount of being "clever".

@Sonicadvance1 Sonicadvance1 force-pushed the gdbserver_support_32bit branch 2 times, most recently from 33c7efa to 0d5646b Compare December 12, 2024 20:28
Requires restructuring a couple of things, but nothing too crazy here.
@Sonicadvance1 Sonicadvance1 force-pushed the gdbserver_support_32bit branch from 0d5646b to 82d7f9f Compare December 12, 2024 20:36
@Sonicadvance1 Sonicadvance1 merged commit 072cf4c into FEX-Emu:main Dec 12, 2024
12 checks passed
@Sonicadvance1 Sonicadvance1 deleted the gdbserver_support_32bit branch December 12, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants