[SharedCache] Reconsider use of `BeginUndoActions` / `ForgetUndoActions` during shared cache loading / analysis
Version and Platform (required):
- Binary Ninja Version: 4.3-dev (current HEAD)
- OS: macOS
- OS Version: 15.1.1
- CPU Architecture: arm64
Bug Description:
The dyld shared cache plug-in contains a couple of locations where it does work that it brackets with BeginUndoActions / ForgetUndoActions, such as:
| auto id = m_dscView->BeginUndoActions(); | |
| if (auto loadedSymbol = m_dscView->GetSymbolByAddress(symbolLocation)) | |
| { | |
| if (m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) | |
| m_dscView->DefineUserSymbol(new Symbol(FunctionSymbol, prefix + loadedSymbol->GetFullName(), targetLocation)); | |
| else | |
| m_dscView->DefineUserSymbol(new Symbol(loadedSymbol->GetType(), prefix + loadedSymbol->GetFullName(), targetLocation)); | |
| } | |
| else if (auto sym = m_dscView->GetSymbolByAddress(symbolLocation)) | |
| { | |
| if (m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation)) | |
| m_dscView->DefineUserSymbol(new Symbol(FunctionSymbol, prefix + sym->GetFullName(), targetLocation)); | |
| else | |
| m_dscView->DefineUserSymbol(new Symbol(sym->GetType(), prefix + sym->GetFullName(), targetLocation)); | |
| } | |
| m_dscView->ForgetUndoActions(id); |
and:
| auto id = m_data->BeginUndoActions(); | |
| m_symbolQueue->Process(); | |
| m_data->EndBulkModifySymbols(); | |
| delete m_symbolQueue; | |
| m_data->ForgetUndoActions(id); |
From discussion on Slack, undo actions are tracked at the BinaryView level. Forgetting undo actions will forget actions performed outside of the local code, such as simultaneous operations performed by the user or by code running on other threads.
Additionally, BeginUndoActions posts work to the main thread and waits for it to complete. This can pose a performance issue if the main thread is not idle.
It would be worth revisiting what the use of BeginUndoActions / ForgetUndoActions is trying to accomplish and seeing if there's an alternative solution that doesn't have these drawbacks (perhaps auto sections / symbols rather than user?).
Slack discussions that touch on this:
https://binaryninja.slack.com/archives/C1E616ZSQ/p1734975321394819
https://binaryninja.slack.com/archives/C1E616ZSQ/p1735840209591709