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

Feature/gdb packet optimization #2014

Merged

Conversation

perigoso
Copy link
Contributor

@perigoso perigoso commented Dec 10, 2024

Detailed description

This tries to improve GDB packet transmission by removing dynamic allocations thus reduncing memory fragmentation potential.

Proper testing is still required.

Your checklist for this pull request

Closing issues

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

This is looking awesome - really excited to see how this runs and performs. We've got just a handful of things we noticed reading through this PR and with them addressed and some testing done, we'll be happy to merge this PR. This solves some mighty nasty issues with the GDB interface logic and with its stack and heap usage!

src/gdb_main.c Outdated Show resolved Hide resolved
src/gdb_main.c Outdated Show resolved Hide resolved
src/gdb_main.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
@dragonmux dragonmux added Enhancement General project improvement GDB Issue/PR related to GDB labels Dec 11, 2024
@perigoso perigoso force-pushed the feature/gdb-packet-optimization branch 4 times, most recently from 306504a to 90190cf Compare December 11, 2024 12:11
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

Couple of things we spotted in the new code from the last review.. we'll get this compiled and thrown on a probe in a bit though and give it a spin with a known good target or two and see what happens 😄

src/gdb_main.c Outdated Show resolved Hide resolved
src/gdb_main.c Outdated Show resolved Hide resolved
@perigoso perigoso force-pushed the feature/gdb-packet-optimization branch from f16ec17 to f4b1210 Compare December 11, 2024 23:12
@dragonmux dragonmux added this to the v2.0 release milestone Dec 12, 2024
dragonmux
dragonmux previously approved these changes Dec 12, 2024
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

This all LGTM. We'll test this tomorrow and let you know if anything falls out that needs fixing. If nothing does, we'll just merge it. Thank you for the contribution!

@perigoso perigoso force-pushed the feature/gdb-packet-optimization branch 2 times, most recently from 34173e9 to 3bb39b4 Compare December 13, 2024 15:28
The tm4c platform should need the same flush workaround logic as stm32 but it was not implemented
This cleans up and reduces code duplication of gdb_putpacket2 and gdb_putpacket
This also integrates data hexifing to simplify the usage on common use cases
After the streamlining of gdb_putpacket the packet is not and does not need to be modified
In a effort to reduce memory fragmentation this removes heap allocations by reusing the existing packet buffer where possible
There is a large stack allocation in a particular code path, this is hopefully better for memory fragmentation though there is a chance of stack overflows, in the event it is a problem it is possible to remove allocations completely
This also introduces a struct to group packet buffer data and further streamlines packet transmission
Simplify packet transmission and reception with dedicated checksum calculation function
This also adds a gdb_put_packet_ok and gdb_put_packet_error for common standard packet responses
…ailable

When reading registers, the stub may also return a string of literal ‘x’’s in place of the register data digits, to indicate that the corresponding register’s value is unavailable.
This indicates that the stub has no means to access the register contents, even though the corresponding register is known to exist.

If a register truly does not exist on the target, then it is better to not include it in the target description in the first place.
This assumed the buffer was large enough to account for the NUL byte, this was an unsafe assumption, we shall limit writes to the size explicitly given to us

Note, none of the current usages of this function required it to handle NUL termination
vasprintf declaration

The last usage of vasprintf as been removed thus this is no longer required
@perigoso perigoso force-pushed the feature/gdb-packet-optimization branch from 3bb39b4 to 8bcdc48 Compare December 13, 2024 16:59
@dragonmux
Copy link
Member

Just tested these changes and they appear to be working wonderfully. Merging - thank you for this wonderful contribution!

@dragonmux dragonmux merged commit 8bcdc48 into blackmagic-debug:main Dec 14, 2024
36 checks passed
@perigoso perigoso deleted the feature/gdb-packet-optimization branch December 14, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement GDB Issue/PR related to GDB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants