Skip to content
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

Open
wants to merge 68 commits into
base: master
Choose a base branch
from

Conversation

abhinav92003
Copy link
Contributor

@abhinav92003 abhinav92003 commented Dec 9, 2024

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

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
@abhinav92003 abhinav92003 changed the title i#7113: Add library to cache information about decoded instructions i#7113: Add analyzer library to cache instr decode info Dec 10, 2024
clients/drcachesim/tools/instr_decode_cache.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tests/instr_decode_cache_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tests/instr_decode_cache_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/instr_decode_cache.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/instr_decode_cache.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/invariant_checker.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/invariant_checker.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/invariant_checker.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/invariant_checker.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/instr_decode_cache.h Outdated Show resolved Hide resolved
@abhinav92003
Copy link
Contributor Author

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)

Copy link
Contributor

@derekbruening derekbruening left a 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.

@abhinav92003 abhinav92003 changed the title i#7113: Add analyzer library to cache instr decode info i#7113 decode cache: Add analyzer library for decode_cache_t Dec 16, 2024
api/docs/release.dox Outdated Show resolved Hide resolved
clients/drcachesim/docs/drcachesim.dox.in Outdated Show resolved Hide resolved
clients/drcachesim/docs/drcachesim.dox.in Outdated Show resolved Hide resolved
abhinav92003 added a commit that referenced this pull request Jan 22, 2025
…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
@derekbruening
Copy link
Contributor

@abhinav92003 abhinav92003 requested a review from derekbruening 22 minutes ago

Could you point at which piece needs a new review: the New Changes button showed just 2 tiny tweaks (+virtual, and copyright change).

@abhinav92003
Copy link
Contributor Author

@abhinav92003 abhinav92003 requested a review from derekbruening 22 minutes ago

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

@derekbruening
Copy link
Contributor

@abhinav92003 abhinav92003 requested a review from derekbruening 22 minutes ago

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.

@abhinav92003
Copy link
Contributor Author

@abhinav92003 abhinav92003 requested a review from derekbruening 22 minutes ago

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.

api/docs/release.dox Outdated Show resolved Hide resolved
clients/drcachesim/tools/common/decode_cache.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/common/decode_cache.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants