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

Fix incorrect lru_cache usage #668

Merged
merged 1 commit into from
Jun 27, 2021

Conversation

borismol
Copy link
Contributor

@borismol borismol commented Jun 25, 2021

Fix incorrect lru_cache usage

Description/Motivation/Screenshots

  • Removed @lru_cache for gef_disassemble since it returns generator and repetitive context_code shows empty code
  • Added get_register_for_selected_frame to correctly cache register values depending on selected frame.

How Has This Been Tested?

Architecture Yes/No Comments
x86-32 ✖️
x86-64 ✔️
ARM ✖️
AARCH64 ✖️
MIPS ✖️
POWERPC ✖️
SPARC ✖️
RISC-V ✖️
make tests ✔️

Checklist

  • My PR was done against the dev branch, not master.
  • My code follows the code style of this project.
  • [] My change includes a change to the documentation, if required.
  • [] My change adds tests as appropriate.
  • I have read and agree to the CONTRIBUTING document.

@borismol borismol force-pushed the fix-incorrect-lrucache-usage branch from 7d8df11 to 43be1d7 Compare June 25, 2021 22:37
@borismol borismol marked this pull request as ready for review June 25, 2021 22:37
@borismol borismol force-pushed the fix-incorrect-lrucache-usage branch from 43be1d7 to 0749012 Compare June 26, 2021 08:29
@borismol borismol changed the title Fix uncorrect lru_cache usage Fix incorrect lru_cache usage Jun 26, 2021
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Good find.

def get_register(regname):
"""Return a register's value."""
return get_register_for_selected_frame(id(gdb.selected_frame()), regname)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you confirmed that gdb.selected_frame(), if called more than once, returns the same object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for review. Unfortunately, no. It changes.
Maybe just remove lru_cache for get_register in such case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would slow things down a lot. Maybe we can use gdb.selected_frame().pc()? Although that could break on recursive functions. We should play with the frame object to see if there something we can use.

@hugsy
Copy link
Owner

hugsy commented Jun 27, 2021

The bug is confirmed and can be reproduced as such

void f10(){ __asm__("int3"); }
void f9(){ f10(); }
void f8(){ f9(); }
void f7(){ f8(); }
void f6(){ f7(); }
void f5(){ f6(); }
void f4(){ f5(); }
void f3(){ f4(); }
void f2(){ f3(); }
void f1(){ f2(); }
void main(){ f1(); }

Using the following command

gdb -ex run -ex 'frame 5' -ex reg -ex 'pi print(get_register.cache_info())' -ex reg -ex 'pi print(get_register.cache_info())' ./nested.out

I get

  • without patch
0:000 ➤  pi print(get_register.cache_info())
CacheInfo(hits=184, misses=26, maxsize=128, currsize=26)
[...]
CacheInfo(hits=191, misses=26, maxsize=128, currsize=26)
  • with patch
CacheInfo(hits=184, misses=26, maxsize=128, currsize=26)
[...]
CacheInfo(hits=191, misses=26, maxsize=128, currsize=26)

To the exception that now the context code pane is properly filled. Thanks for the PR @borismol

@hugsy hugsy merged commit 076bc6e into hugsy:dev Jun 27, 2021
hugsy pushed a commit that referenced this pull request Jul 1, 2021
hugsy pushed a commit that referenced this pull request Jul 1, 2021
@hugsy hugsy added this to the Release 2021.07 milestone Jul 6, 2021
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