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

Memory map provider #1003

Merged
merged 11 commits into from Dec 18, 2023
Merged

Memory map provider #1003

merged 11 commits into from Dec 18, 2023

Conversation

Grazfather
Copy link
Collaborator

@Grazfather Grazfather commented Sep 1, 2023

Description

This change allows architectures to provide their own memory map by defining maps in the architecture. If set, then GefMemoryManager will defer to it when building maps.

This can be used for e.g. architectures that are built for use with gdbserver.

It also adds parse_info_mem (Renaming the old one to parse_monitor_info_mem) which parses the maps from this command. The Black Magic Probe provides memory maps of its targets via this command.

Checklist

  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • If my change adds new code, adequate tests have been added.
  • I have read and agree to the CONTRIBUTING document.

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

🤖 Coverage Update

  • Commit: 6a6e2a0
  • Current Coverage: 71.6614%
  • New Coverage: 71.6614%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

gef.py Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

🤖 Coverage Update

  • Commit: 6a6e2a0
  • Current Coverage: 71.6614%
  • New Coverage: 71.6614%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

1 similar comment
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

🤖 Coverage Update

  • Commit: 6a6e2a0
  • Current Coverage: 71.6614%
  • New Coverage: 71.6614%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

I like the idea of abstraction the memory map provider. But there's potential to break tons of things considering how much we rely on the mem map, so it'll need to be test the s**t out of before merging.

Copy link

stale bot commented Nov 11, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You can reopen it by adding a comment to this issue.

@stale stale bot added the stale label Nov 11, 2023
Copy link

stale bot commented Dec 12, 2023

This issue has been automatically closed because it has not had recent activity. If you are the owner of this issue, you can either re-open it and provide a more complete description; or create a new issue. Thank you for your contributions.

@stale stale bot closed this Dec 12, 2023
@Grazfather Grazfather reopened this Dec 13, 2023
Copy link

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: caa641b
  • Current Coverage: 71.4300%
  • New Coverage: 71.2815%
  • Diff: -0.14850000000001273

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 6924019
  • Current Coverage: 71.4300%
  • New Coverage: 71.2814%
  • Diff: -0.14860000000000184

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: e6a8663
  • Current Coverage: 71.4511%
  • New Coverage: 71.3027%
  • Diff: -0.1483999999999952

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Some things to change before merge.

Also it's a good time to add some testing to those functions, gdb output string parsing can blow up in our face at any next release.

gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
Copy link

🤖 Coverage Update

  • Commit: c6e05f4
  • Current Coverage: 71.4511%
  • New Coverage: 71.3097%
  • Diff: -0.1413999999999902

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 8525c7b
  • Current Coverage: 71.4511%
  • New Coverage: 71.3376%
  • Diff: -0.11350000000000193

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 38579be
  • Current Coverage: 71.4511%
  • New Coverage: 71.4579%
  • Diff: 0.006799999999998363

To this point, this PR:

  • includes changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • includes changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: f7a2105
  • Current Coverage: 71.4511%
  • New Coverage: 71.4511%
  • Diff: 0.0

To this point, this PR:

  • includes changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link

🤖 Coverage Update

  • Commit: 65e918e
  • Current Coverage: 71.4511%
  • New Coverage: 71.3767%
  • Diff: -0.07439999999999714

To this point, this PR:

  • includes changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

👍

@Grazfather Grazfather merged commit 4f20983 into main Dec 18, 2023
6 checks passed
@Grazfather Grazfather deleted the memory_map_provider branch December 18, 2023 16:59
@hugsy hugsy added this to the 2024.01 milestone Dec 22, 2023
@blasty
Copy link

blasty commented Feb 21, 2024

Hi, is there any reason _parse_maps does not actually attempt to populate the maps using parse_info_mem? I'm currently working on a target for which I'm providing my own barebones GDB stub remote support. Memory map information is provided by the remote to GDB as a reply to the q packet which requests Xfer:memory-map:read::. I had to hack GEF to invoke parse_info_mem to correctly populate the memory map. But perhaps there's a (good) reason I'm missing as to why this functionality is not invoked at all currently?

EDIT: I guess @Grazfather' rationale was that the right way to do it is to rely on the output of monitor info mem, which emits a Rcmd to the remote stub requesting the memory map. Though I guess parse_info_mem could still be used as a fallback if the remote stub doesn't support that?

@Grazfather
Copy link
Collaborator Author

It's not used because it's not reliable on normal linux targets. You could create a new Architecture that subtypes the real architecture, and defines a maps method that calls it, then manually set the architecture to that and you're done.

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.

None yet

3 participants