-
Notifications
You must be signed in to change notification settings - Fork 566
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
Add new cache management instruction support to drmemtrace #7111
Comments
We may also want to mark other cache ops such as CLWB and CLDEMOTE. |
Adds handling to drmemtrace for the x86 CLFLUSHOPT opcode to treat it just like CLFLUSH; the two are similar enough for most uses. Augments the existing CLFLUSH invariant_checker test as a sanity test of the new opcode handling. Issue: #7111
Adds handling to drmemtrace for the x86 CLFLUSHOPT opcode to treat it just like CLFLUSH; the two are similar enough for most uses. Augments the existing CLFLUSH invariant_checker test as a sanity test of the new opcode handling. Issue: #7111
In fact, e24dbb0 has CLWB marked as a load: which it is not. If we do nothing, the trace will have it labeled as a read==load record. That highlights that this affects all analyzers and not just those that want to model cache behavior: those looking at loads will do the wrong thing. So this is not just about augmenting the trace but correcting the trace. Today a functional cache simulator like dr$sim doesn't need any architecture-specific code: everything that affects the caches is a first-class record type. If we don't do the same with these operations, we're forcing dr$sim and others to add decoding and architecture-specific handling of the various cache operations. The complexity of figuring out what these operations do has to live somewhere: the question is, does it live in the trace itself and the consumers like dr$sim remain arch-agnostic and simple, or does it live in the consumers and they become complicated? It could live in a library to share among analyzers. We added the whole series of TRACE_TYPE_PREFETCH_INSTR_L1, TRACE_TYPE_PREFETCH_INSTR_L1_NT, etc. record types, along with cache flush types, back when we didn't have embedded encodings: so there was no simple way to get that info (though you could map the binaries and decode it was rarely done). So is there less of an argument to continue in the pre-existing approach of having all cache-related operations as first-class record types because it is now a little easier to decode? Since we have to correct the marking of this and related operations as loads, we need some kind of new record type. Trying to boil this down to these choices:
|
fwiw I have a similar issue. Our use of DR is confined to the decoding/encoding components. We examine blocks of instructions and capture their memory inputs so that the block can later be re-executed (with appropriate rewriting) outside of the original program by providing the initial register states and said memory inputs. We end up special casing the prefetch/cache flush instructions to avoid unnecessarily capturing their input memory operands precisely because they are not architectural loads. |
Should there be an IR API routine to query whether a load actually issues a regular load? A different version of |
Yeah that might be nice. I looked at adding an equivalent of |
Some of us have lobbied to separate the DR versions of some Arm opcodes to make these things easier. See the discussion at |
My pending comment in #7109 which adds CLFLUSHOPT opcode decoding support:
drmemtrace needs to be udpated as well: it looks like CLFLUSHOPT also invalidates all cache levels and so should be treated like CLFLUSH in memtraces (or would a simulator want to know about the subtle differences?). So just adding to 2 spots in instru.cpp https://github.com/DynamoRIO/dynamorio/blob/master/clients/drcachesim/tracer/instru.cpp#L227 and https://github.com/DynamoRIO/dynamorio/blob/master/clients/drcachesim/tracer/instru.cpp#L244 should be enough (optionally extend invariant_checker_test). Hmm. Maybe enough questions this should be separated out.
The text was updated successfully, but these errors were encountered: