angr-management icon indicating copy to clipboard operation
angr-management copied to clipboard

Remove the cache for FunctionTableModel

Open mahaloz opened this issue 2 years ago • 9 comments

This reverts commit b7b7bf7184ce3340191e493a673e3d1eb9b6b2b9.

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 avatar Jun 16 '23 04:06 mahaloz

Test Results

19 tests  ±0   19 :heavy_check_mark: ±0   1m 44s :stopwatch: +11s 10 suites ±0     0 :zzz: ±0  10 files   ±0     0 :x: ±0 

Results for commit 8a4e073f. ± Comparison against base commit 7b1960e9.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jun 16 '23 04:06 github-actions[bot]

The user experience without the cache is not great, see comparisons here https://github.com/angr/angr-management/pull/853

How about triggering a cache update?

mborgerson avatar Jun 27 '23 01:06 mborgerson

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

mahaloz avatar Jun 27 '23 05:06 mahaloz

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

ltfish avatar Jun 27 '23 05:06 ltfish

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

ltfish avatar Jun 27 '23 05:06 ltfish

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

mahaloz avatar Jul 04 '23 21:07 mahaloz

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

ltfish avatar Jul 05 '23 10:07 ltfish