[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Oct 19 06:29:02 PDT 2020
labath added a comment.
In D89283#2336280 <https://reviews.llvm.org/D89283#2336280>, @vsk wrote:
> In D89283#2336120 <https://reviews.llvm.org/D89283#2336120>, @wallace wrote:
>
>> @vsk, I agree with you regarding the files. At the moment our implementation of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. Then, we'll be able to generate these trace files on the fly as the tests run, so I imagine I'll be deleting these binary files. For the time being I doubt I'll include any new binary, as what is included is more than enough to test the basic decoding functionalities.
>
> That seems promising. Deleting those binary files after the fact doesn't address the issue, though, as they'd be part of the history. I have a question about that ld-2.17.so file in particular: is there no way to decoder/traverse a trace of a process that loads a dylib, without copying all of ld.so into the source tree? That seems very surprising -- I'd expect the decoder API to allow you to skip right over PC ranges that have nothing to do with the binary you want to debug.
I would very much like to keep some tests which use pre-baked traces (binaries are a different matter). I have a couple of reasons for that:
- tests exercising the tracing code path require appropriate hardware. Checked in traces can run everywhere (assuming one can build the pt library there)
- due to their systemic nature, it will be hard to test various edge conditions (missing binaries, corrupt traces, ...) with a end-to-end test
- we had some issues with end-to-end tests being flaky due to the fact that cpu writes the traces asynchronously -- I am not sure we ever fully figured that out
Now, we're definitely going to need tests which check the tracing functionality, but I think that having these kinds of traces is definitely good. As for ld.so, while it's not the end of the world (we have much larger binaries), it would definitely be nice to avoid it. What functionality is that test exercising? If you want traces that cross multiple modules, maybe you could capture a trace from the middle of an application, after ld.so is done, and show how the application is ping-ponging between some functions in different shared libraries.
In D89283#2336090 <https://reviews.llvm.org/D89283#2336090>, @vsk wrote:
> One possible alternative:
>
> - Design a textual description for the raw trace contents, and possibly a way to convert an existing trace file into this textual format
I think that converting the textual trace description would essentially mean reimplementing the intel-pt library. That might be nice (llvm is definitely fond of reimplementing things), but I'm not sure if that's what these guys have signed up for.
> - Check in assembly, and use llvm-mc/clang to generate executables during testing
This is definitely doable, though I'd probably go for yaml2obj, as llvm-mc&clang cannot guarantee the exact placement of instructions in memory. It looks like obj2yaml has grown program header support since last time I checked this (previously you had to write them by hand), so it may be that a plain `obj2yaml | yaml2obj` would just work now. (It probably won't produce a fully functional binary, but it might be close enough.)
================
Comment at: lldb/include/lldb/Target/Trace.h:160-179
+ /// Determine whether the trace plug-in supports the \a GetInstructionCount
+ /// method.
+ ///
+ /// Some trace plug-ins might prefer to provide instructions lazily on the
+ /// fly, being unable to provide the total number of instructions in a single
+ /// call.
+ ///
----------------
How about `Optional<size_t> GetInstructionCount`? That makes it less likely to develop an accidental dependancy on this interface (though I fear that might still happen without an subclass which actually returns None here).
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:31
+ if (m_custom_error_message.empty()) {
+ std::ostringstream os;
+ os << pt_errstr(pt_errcode(GetErrorCode())) << ": 0x" << std::hex
----------------
`raw_string_ostream` would be more llvm-y (the std::hex part in particular is very non-idiomatic)
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
+
+std::vector<IntelPTInstruction> &DecodedThread::GetInstructions() {
+ return m_instructions;
----------------
Do you want anyone to modify the vector? Return ArrayRef<IntelPTInstruction>
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:32
+/// returned.
+int FindNextSynchronizationPoint(pt_insn_decoder &decoder) {
+ // Try to sync the decoder. If it fails, then get
----------------
static
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:80
+/// error code in case of errors.
+int ProcessPTEvents(pt_insn_decoder &decoder, int errcode) {
+ while (errcode & pts_event_pending) {
----------------
static
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:158
+ void *context) {
+ Process *process = reinterpret_cast<Process *>(context);
+
----------------
static_cast is enough here
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:178
+ config.begin =
+ reinterpret_cast<uint8_t *>(const_cast<char *>(trace.getBufferStart()));
+ config.end =
----------------
I presume that the pt library does not actually modify this data. Maybe a short note saying that.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:188
+ if (int error = pt_image_set_callback(image, ReadProcessMemory, &process))
+ return CreateLibiptError(error);
+
----------------
It looks like this can only fail if the image argument is null (which can only happen if the decoder is null, which is checked). An assert would be enough for that. (For a proper error handling you should have also freed the decoder object on the error path, which is how i came to thing about this).
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:69
+ std::forward_as_tuple(
+ std::make_pair(thread->GetProcess()->GetID(), thread->GetID())),
+ std::forward_as_tuple(thread, pt_cpu));
----------------
drop std::make_pair, it's cleaner.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:94
+
+ std::vector<IntelPTInstruction> instructions =
+ decoded_thread->GetInstructions();
----------------
this makes a copy, which you probably did not want.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:98
+ int delta = direction == TraceDirection::Forwards ? -1 : 1;
+ for (uint64_t i = position; i < instructions.size() && i >= 0; i += delta)
+ if (!callback(i, instructions[i].GetLoadAddress()))
----------------
`i>=0` is always true. You'll have to do this trick with signed numbers (ssize_t?)
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109
+ else {
+ consumeError(decoded_thread.takeError());
+ return 0;
----------------
I'm having doubts about this "I have an thread but was not able to decode _anything_ about it" state is worth it. Having many different ways to report errors just increases the chance of something going wrong, and in `TraverseInstructions` you're already treating this state as a pseudo-instruction.
Maybe that representation should be used all the way down? Or (and this may be even better) we avoid creating such Threads in the first place (print an error/warning when the target is created).
================
Comment at: lldb/source/Target/Trace.cpp:81
+static int GetNumberOfDigits(size_t num) {
+ int digits_count = 0;
+ do {
----------------
vsk wrote:
> Just 'assert(num); return ceill(log10(num));'?
that would have to be `log10(num+1)`, though I'm not sure what to thing of the floating point arithmetic...
================
Comment at: lldb/source/Target/Trace.cpp:106
+
+ size_t last_index = (int64_t)start_position + count - 1;
+ TraverseInstructions(
----------------
The cast to int64_t won't change the actual value of the result (though it may invoke UB due to signed wraparound). What exactly are you trying to achieve here?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:43-48
+ [ 0] 0x40052d
+ [ 1] 0x400529
+ [ 2] 0x400525
+ [ 3] 0x400521
+ [ 4] 0x40052d
+ [ 5] 0x400529
----------------
Are you sure that printing this backwards is the best way to display this? The resulting disassembly is going to look quite weird. I think that printing this in the "normal" direction would make it easier to figure out what the program was doing. For people who are only interested in the final PC value it should not be a problem to skip to the last line of the output (the last line is also more likely to remain visible if the dump produces lots of data).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89283/new/
https://reviews.llvm.org/D89283
More information about the lldb-commits
mailing list