flush cache after mempatch #5295
Conversation
This adds code to flush cache after applying a hotpatch. While it's unlikely that a patched location will be in cache at the time we apply a patch, it is better to be safe than sorry Also fixed the spelling error in `setPermissions` while I was here
|
side comment: i did not implement this for MacOS |
|
sys/cachectl.h: No such file or directory |
any idea what it should be? that's what the documentation for nvm, found it (i think) |
apparently the documentation i found was... incomplete
library/Process-linux.cpp
Outdated
| { | ||
| return cacheflush(target, count, BCACHE); | ||
| __builtin___clear_cache((char*)target, (char*)target + count - 1); | ||
| return false; /* assume always succeeds, as the builtin has no return type */ |
There was a problem hiding this comment.
Just verifying, you return false for success?
There was a problem hiding this comment.
yeah, following the unix standard of 0 (false) for success and non-zero for failure. this is used elsewhere in DFHack for OS API calls (see, e.g. PlugLoad)
There was a problem hiding this comment.
let me be clear that i don't have a problem with returning true for success :)
There was a problem hiding this comment.
It seems to me that returning 0 for success makes sense when the return value is an int, but if it's a bool, then it should return true for success. If this matches the semantics of the surrounding functions and is consistent across the different implementations in Process-linux and Process-windows, though, I'm fine with it.
There was a problem hiding this comment.
switched, since tbh i prefer returning true on success as well
2 participants
This adds code to flush cache after applying a hotpatch. While it's unlikely that a patched location will be in cache at the time we apply a patch, it is better to be safe than sorry
Also fixed the spelling error in
setPermissionswhile I was hereNo changelog; this is a fix to existing functionality to avoid a potential future bug instead of fixing a known existing one