-
Notifications
You must be signed in to change notification settings - Fork 570
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
i#7113 decode cache: Add analyzer library for decode_cache_t #7114
base: master
Are you sure you want to change the base?
Conversation
Adds a new library to cache information about decoded instructions. This can be used by analysis tools that need to decode the instr encodings in the trace. The library allows the tools to specify what information they need to cache. Refactors the invariant checker tool to use this library. Issue: #7113
Decided to try out an alternate way to support module-mapper-decoding in instr_decode_cache_t that came out of offline discussion. Okay to hold off on the re-review until then (Cannot undo re-request review) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank review to reset the requested review state.
…upport to instr_decode_cache_t
…rs (#7193) Skips the unnecessary munmap before the subsequent mmap to the same region in elf_loader_map_phdrs. To mmap all the loadable segments of a file, elf_loader_map_phdrs first gets a large anonymous map. Then for each loadable segment, it munmaps a portion of the anonymous map and mmaps the segment to it. There is potential for a race with other threads that may mmap some memory between the munmap and mmap, which will then get stolen from that other thread because our mmap uses MAP_FIXED. This manifests as crashes when the other thread munmaps that region eventually, and the module mapper cannot access the mapped segment suddenly. This PR mitigates such a race by simply skipping the munmap call, since the following mmap call uses MAP_FIXED anyway which causes the overlapping address range in the initial map to atomically get unmapped. Note that MAP_FIXED documents that the only safe way to use it is with a range that was previously reserved using another mapping, otherwise it may end up forcibly removing someone else's existing mappings. This race manifested in #7114 during module_mapper_t initialization which loads all app binaries in the process address space. #7114 is moving this init into the analyzer worker threads whereas previously it was done before launching the workers, which hid the race. There were ~20/1000 analyzer run crashes upon testing #7114 on a small internal test trace, which are fixed with this. There are some other cases where the unmap call must be made, like when the initial address range was obtained from the vmm to honor the loaded library's preferred address (in this case there's no real race due to how os_unmap_file is implemented; see comment in code), or when d_r_(un)map_file is used (in some non-analyzer cases) which needs to perform other book-keeping besides the actual mmap/munmap (left as future TODO). Most uses of the elf_loader_map_phdrs mapping code (private library load, early injection) are during DR initialization where there's no race with other threads. Issue: #7192
Could you point at which piece needs a new review: the New Changes button showed just 2 tiny tweaks (+virtual, and copyright change). |
Since your last review: https://github.com/DynamoRIO/dynamorio/pull/7114/files/268e5a7d20c98433b97a971b3b1b69eaa394ae5f..31f41466c896760c7b995be302b1a93499512f03 |
That seems to have unrelated changes from merging: e.g. the very first patch in release.dox. |
That view does yes. But it's only that file that overlaps a bit. Alternatively you could look at individual commits between two above-mentioned two hashes. |
Adds a new drmemtrace_decode_cache library to cache information about decoded instructions using decode_cache_t. This can be used by analysis tools that need to decode the instr encodings in the trace, to avoid overhead of redundant decodings which can get expensive.
The library allows the tools to specify what information they need to cache. Also, it uses instr_noalloc_t when possible to reduce heap usage and allocation/deallocation overhead.
If the trace does not include embedded encodings or if the user wants to get encodings from the app binaries using module_mapper_t instead, they can provide the module file path to the init API on the decode_cache_t object. decode_cache_t keeps a single initialized module_mapper_t at any time, which is shared between all decode_cache_t objects (even the ones of different template types); this is done by tracking the count of active objects using the module mapper.
decode_cache_t provides the clear_cache() API which can be used in parallel_shard_exit() to keep memory consumption in check by free-ing up cached decoding info that may not be needed for result computation in later print_results() which has to wait until all shards are done.
Refactors the invariant checker and opcode mix tools to use this library.
Modifies add_encodings_to_memrefs to support a mode where encodings are not set in the generated test memref but only the instr addr and size fields are set.
Makes the opcode cache in opcode_mix_t per-shard instead of per-worker. Decodings must not be cached per-worker as that may cause stale encodings for non-first shards processed by the worker. This means the worker init and worker exit APIs can be removed now from opcode_mix_t.
Adds decode_cache_test and opcode_mix_test unit tests that verify operation of the decode_cache_t.
Issue: #7113