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

correct parsing in gdb_get_location_from_symbol #1037

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Conversation

Angelo942
Copy link
Contributor

@Angelo942 Angelo942 commented Jan 2, 2024

Description

As stated in the comments info symbol returns "<symbol_name> + <offset>" with spaces around the +. The current split leaves a space in front of the offset which makes .isdigit() return False.

@r12f, do you think we still need the .strip() for sym[0] ?

  • 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.

@r12f
Copy link
Contributor

r12f commented Jan 2, 2024

Hi @Angelo942 , with this, I believe the .strip() could be removed. I never saw symbols with leading space, the strip is mostly used for removing the trailing ones.

@r12f
Copy link
Contributor

r12f commented Jan 2, 2024

although this means we are missing test case.....

do you mind to help update the test case: test_cmd_xinfo_on_class. Instead of running xinfo $pc, we run xinfo $pc+4, and the output should have "Symbol: B<TraitA, TraitB>::Run+4" in my understanding.

@Angelo942
Copy link
Contributor Author

I'll look into it

@Angelo942
Copy link
Contributor Author

@r12f, I wrote it, but I have a problem. If I do b B<TraitA, TraitB>::Run() the breakpoint is put on 0x0000555555555320 <B<TraitA, TraitB>::Run()+8> instead. I have the same result without GEF...

@hugsy
Copy link
Owner

hugsy commented Jan 3, 2024

It could be worth checking for a more native way to handle symbols than string parsing. The gdb_get_location_from_symbol was written a very long time ago, when the Python API was much less mature than it is now.

@Angelo942 Angelo942 marked this pull request as draft January 3, 2024 00:29
@Angelo942
Copy link
Contributor Author

Right now I'm worried about this part from the tests: Symbol: B<TraitA, TraitB>::Run()+20 instead of Symbol: B<TraitA, TraitB>::Run()+4. Is there really a problem with how gdb puts the breakpoints ?

@r12f
Copy link
Contributor

r12f commented Jan 3, 2024

Hi @Angelo942 , for your question below:

If I do b B<TraitA, TraitB>::Run() the breakpoint is put on 0x0000555555555320 <B<TraitA, TraitB>::Run()+8> instead.

how $pc is set is platform dependent. why you are seeing it, is very likely because you are running on an ARM64-based machine, which $pc will be set to 8 bytes after the breakpoint.

"+4" would work on CISC arch machines, like x64. I did a quick change on this branch and looks like it works for me: https://github.com/r12f/gef/tree/user/r12f/xinfo-offset.

image

so instead of checking "+4", we can assert against "Symbol: B<TraitA, TraitB>::Run()+", just to ensure offset is indeed parsed. (still with xinfo $pc+4, to ensure there will be an offset there.)

@Angelo942 Angelo942 marked this pull request as ready for review January 3, 2024 01:52
@Angelo942
Copy link
Contributor Author

@r12f, I found the problem. "The difference is that b *main breaks on the first instruction of main, while b main breaks on the first instruction after the function prologue."

I changed the command we use to set the breakpoint so that now it is indeed on the first instruction of the function and $pc+4 works.

But regarding ARM, are you sure that the check would fail ?

@r12f
Copy link
Contributor

r12f commented Jan 3, 2024

I see. That makes sense.

for the ARM question, although I don't have a working arm board at hand to validate right now, but at least that's what ARM doc says: Register-relative and PC-relative expressions.

In ARM code, the value of the PC is the address of the current instruction plus 8 bytes.

@r12f
Copy link
Contributor

r12f commented Jan 3, 2024

ok, I have got a working arm64 board now, so I can confirm that +4 should work!

Here is the output, which shows $pc is pointing to the start of the function when breakpoint is hit:

image

The breakpoint is set by b *'B<TraitA, TraitB>::Run'

@r12f
Copy link
Contributor

r12f commented Jan 3, 2024

Test is also passed on arm64:

image

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.

LGTM

self.assertNoException(res)
self.assertIn("Symbol: B<TraitA, TraitB>::Run", res)
self.assertIn("Symbol: B<TraitA, TraitB>::Run()+4", res)
Copy link
Owner

Choose a reason for hiding this comment

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

🔥

@hugsy hugsy merged commit deeab2f into hugsy:main Jan 4, 2024
4 of 5 checks passed
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