[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 16 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:188
+  if (int error = pt_image_set_callback(image, ReadProcessMemory, &process))
+    return CreateLibiptError(error);
+
----------------
labath wrote:
> 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).
good catch!


================
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()))
----------------
clayborg wrote:
> labath wrote:
> > `i>=0` is always true. You'll have to do this trick with signed numbers (ssize_t?)
> Yes, switch to ssize_t, your delta is already signed. Also switch "delta" to ssize_t as well.
TIL!


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109
+  else {
+    consumeError(decoded_thread.takeError());
+    return 0;
----------------
labath wrote:
> 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).
> Maybe that representation should be used all the way down? 

I'll follow that path. This will create consistency through the code


> 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).
 
I wish I could do that, but decoding is very expensive and should be done lazily. According to Intel and my tests, if a thread was traced during T seconds, then decoding takes around 10T, which is a big amount of time if you were tracing 10 threads for 5 seconds, which would take 500 seconds to decode. At least for know we are not doing parallel decoding. I imagine at some point we'll have to work on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89283/new/

https://reviews.llvm.org/D89283