Skip to content

Commit

Permalink
[SharedCache] Fix some bugs and add the ability to bulk load images
Browse files Browse the repository at this point in the history
This is a massive and messy commit that started out as trying to add bulk loading of images (the ability to load them in parallel via a single API call) but then required a number of changes that unearthed some fundamental bugs with the current design of `SharedCache`. Untangle it all to create nice clean commits is more work than its worth.

The main bug fix is the fact that `State()` was not actually thread safe because `m_state` which it was returning a copy of could be modified concurrently during the copying. Now a lock is held when accessing `m_state` via functions like `State()` and a `shared_ptr` is returned. This is a thread safe operation and the returned `shared_ptr` guarantees the `State` struct remains allocated whilst that `shared_ptr` exists. This has also required changes to some of the code to create local variables to ensure things remain allocated after they are read from `State()`.

The other big change is introducing an API call to load a number of images concurrently. This is done using worker threads and required some changes to `SharedCache::LoadImageWithInstallName` and `SharedCache::LoadSectionAtAddress` so that they could work in parallel. Previously they just took locks for extended periods, this is now reduced as much as possible. Other work had to be done to make parts of the loading process thread safe.

Something else this commit adds is the commenting out of calls to the undo actions API; `BeginUndoActions`, `ForgetUndoActions` and `CommitUndoActions`. Tbh I don't know if this actually works at all when being used in parallel as, according to Vector35, due to the design of the undo actions system, it can't distinguish undo actions across threads. So when you've got loads of threads making calls to this API and then calling `ForgetUndoActions` or `CommitUndoActions`, is what you expect actually getting forgotten or committed? The main reason for commenting these out though is issues like Vector35#6289.

There might be some other minor changes in this commit I've forgotten about but I think this is the majority of it.
  • Loading branch information
WeiN76LQh authored and WeiN76LQh committed Jan 2, 2025
1 parent 206c8a1 commit 7e97d51
Show file tree
Hide file tree
Showing 7 changed files with 992 additions and 529 deletions.
25 changes: 25 additions & 0 deletions view/sharedcache/api/python/_sharedcachecore.py
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,31 @@ def BNDSCViewLoadImageWithInstallName(
return _BNDSCViewLoadImageWithInstallName(cache, cstr(name), skipObjC)


# -------------------------------------------------------
# _BNDSCViewLoadImagesWithInstallNames

_BNDSCViewLoadImagesWithInstallNames = core.BNDSCViewLoadImagesWithInstallNames
_BNDSCViewLoadImagesWithInstallNames.restype = ctypes.c_bool
_BNDSCViewLoadImagesWithInstallNames.argtypes = [
ctypes.POINTER(BNSharedCache),
ctypes.POINTER(ctypes.c_char_p),
ctypes.c_ulonglong,
ctypes.c_bool,
ctypes.c_bool,
]


# noinspection PyPep8Naming
def BNDSCViewLoadImagesWithInstallNames(
cache: ctypes.POINTER(BNSharedCache),
names: ctypes.POINTER(ctypes.c_char_p),
namesCount: int,
freeNames: bool,
skipObjC: bool
) -> bool:
return _BNDSCViewLoadImagesWithInstallNames(cache, names, namesCount, freeNames, skipObjC)


# -------------------------------------------------------
# _BNDSCViewLoadSectionAtAddress

Expand Down
5 changes: 5 additions & 0 deletions view/sharedcache/api/python/sharedcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ def __init__(self, view):
def load_image_with_install_name(self, installName, skipObjC = False):
return sccore.BNDSCViewLoadImageWithInstallName(self.handle, installName, skipObjC)

def load_images_with_install_names(self, installNames, skipObjC = False):
installNamesArrayType = ctypes.c_char_p * len(installNames)
installNamesCArray = installNamesArrayType(*[s.encode('utf-8') for s in installNames])
return sccore.BNDSCViewLoadImagesWithInstallNames(self.handle, installNamesCArray, len(installNames), False, skipObjC)

def load_section_at_address(self, addr):
return sccore.BNDSCViewLoadSectionAtAddress(self.handle, addr)

Expand Down
11 changes: 11 additions & 0 deletions view/sharedcache/api/sharedcache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,17 @@ namespace SharedCacheAPI {
return BNDSCViewLoadImageWithInstallName(m_object, str, skipObjC);
}

bool SharedCache::LoadImagesWithInstallNames(std::vector<std::string_view> installNames, bool skipObjC)
{
std::vector<const char*> installNamesPtrs;
for (auto& installName : installNames)
{
installNamesPtrs.push_back(installName.data());
}
char** strs = BNAllocStringList(installNamesPtrs.data(), installNamesPtrs.size());
return BNDSCViewLoadImagesWithInstallNames(m_object, strs, installNamesPtrs.size(), true, skipObjC);
}

bool SharedCache::LoadSectionAtAddress(uint64_t addr)
{
return BNDSCViewLoadSectionAtAddress(m_object, addr);
Expand Down
1 change: 1 addition & 0 deletions view/sharedcache/api/sharedcacheapi.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ namespace SharedCacheAPI {
static uint64_t FastGetBackingCacheCount(Ref<BinaryView> view);

bool LoadImageWithInstallName(std::string_view installName, bool skipObjC = false);
bool LoadImagesWithInstallNames(std::vector<std::string_view> installNames, bool skipObjC = false);
bool LoadSectionAtAddress(uint64_t addr);
bool LoadImageContainingAddress(uint64_t addr, bool skipObjC = false);
std::vector<std::string> GetAvailableImages();
Expand Down
1 change: 1 addition & 0 deletions view/sharedcache/api/sharedcachecore.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ extern "C"
SHAREDCACHE_FFI_API char** BNDSCViewGetInstallNames(BNSharedCache* cache, size_t* count);

SHAREDCACHE_FFI_API bool BNDSCViewLoadImageWithInstallName(BNSharedCache* cache, char* name, bool skipObjC);
SHAREDCACHE_FFI_API bool BNDSCViewLoadImagesWithInstallNames(BNSharedCache* cache, char** names, size_t namesCount, bool freeNames, bool skipObjC);
SHAREDCACHE_FFI_API bool BNDSCViewLoadSectionAtAddress(BNSharedCache* cache, uint64_t name);
SHAREDCACHE_FFI_API bool BNDSCViewLoadImageContainingAddress(BNSharedCache* cache, uint64_t address, bool skipObjC);

Expand Down
Loading

0 comments on commit 7e97d51

Please sign in to comment.