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

Remove the cache for FunctionTableModel #1023

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mahaloz
Copy link
Member

@mahaloz mahaloz commented Jun 16, 2023

This reverts commit b7b7bf7.

Bug

Since this commit, if you rename a function in angr then the functions table does not refresh.
To reproduce:

  1. Rename a function through the Pseudocode view
  2. Notice the functions table does not refresh

Open to changes to this PR.

@mahaloz mahaloz requested a review from mborgerson June 16, 2023 04:01
@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2023

Test Results

19 tests  ±0   19 ✔️ ±0   1m 44s ⏱️ +11s
10 suites ±0     0 💤 ±0 
10 files   ±0     0 ±0 

Results for commit 8a4e073. ± Comparison against base commit 7b1960e.

♻️ This comment has been updated with latest results.

@mborgerson
Copy link
Member

The user experience without the cache is not great, see comparisons here #853

How about triggering a cache update?

@mahaloz
Copy link
Member Author

mahaloz commented Jun 27, 2023

@mborgerson I would if I knew how. When I first went for this fix, I triggered the cache to refresh whenever a Rename event happened. This will not work because there are many ways to change the function name. You can even do it programmatically as binsync does by just writing directly to the kb's functions (which may not be am containers).

I am not sure I understand your mental model when you designed this cache since I don't know what you want to pipeline to get this event to trigger. If it's just AM containers being updated, we still need to catch all the programmatic ways to update names. I see removing the cache as the best alternative right now, even compared to lag.

@ltfish
Copy link
Member

ltfish commented Jun 27, 2023

I think the best way is to manually trigger the refresh-cache event after programmatically setting function names.

@ltfish
Copy link
Member

ltfish commented Jun 27, 2023

We can also wrap each Function object within an ObjectContainer class.

@mahaloz
Copy link
Member Author

mahaloz commented Jul 4, 2023

@ltfish I think this change will require things to change in angr, since, again, things can be accessed from the kb, which will not trigger any known handler we have now. I don't have time currently to reimplement this cache with the correct handlers everywhere. However, I would say it is critical that the decompiler actually shows renamed function in the function tab. This is still on master where anyone using the decompiler sees no change to the function in the function tab.

I suggest this PR is merged, and we open an issue for #853 as a separate task.

@ltfish
Copy link
Member

ltfish commented Jul 5, 2023

I think we should fix it in the right way. I'll give it a try tomorrow.

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