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

[walter erquinigo via Phabricator via llvm-commits]

wallace updated this revision to Diff 299236.
wallace marked 3 inline comments as done.
wallace added a comment.

Addressed almost all changes.

What's left to discuss:

- I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the binaries. Besides, I removed ld.so, and the binaries that are left are tiny. I imagine that the binaries that will be committed in the future will also be tiny ones depicting specific edge cases.
- Showing the disassembly of the instructions, requested by Greg, is not yet done.
- I'm representing now all decoding errors as instructions, which in fact simplifies the API a good deal. However, I'm still not happy with the API, right now we have

class Trace {

  void TraverseInstructions(Thread, callback, ...);
  int GetInstructionsCount(Thread...);

}

Right now these methods are not doing anything (or returning 0) if the provided Thread is not traced by the Trace instance. Which is kind of what ThreadList::GetThreadAtIndex does, as well as many other APIs. To keep consistency with most of LLDB core APIs, we could leave the code as it is, which simplifies a lot the API.

class Trace {

  TracedThreadSP GetTraceForThread(Thread ...);

}

class TracedThread {

  void TraverseInstructions(callback, ...);
  int GetInstructionsCount()

}

This creates a level of indirection that forces the caller to check if the returned TracedTrace is a valid pointer or not. If the pointer is invalid, the caller can show an error message or fail silenty, otherwise, TraverseInstructions and GetInstructionsCount will perform valid operations.

This might become useful to avoid errors in complex scenarios as the API grows. But then, how often will someone invoke these methods with the wrong thread?

The first form of the API looks simpler without that level of indirection, which is cool.

What do you guys think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283

Files:
  lldb/include/lldb/Target/ProcessTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/ProcessTrace.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/intelpt-trace-multi-file/a.out
  lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.h
  lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.h
  lldb/test/API/commands/trace/intelpt-trace-multi-file/libbar.so
  lldb/test/API/commands/trace/intelpt-trace-multi-file/libfoo.so
  lldb/test/API/commands/trace/intelpt-trace-multi-file/main.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file-no-ld.json
  lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file.trace
  lldb/test/API/commands/trace/intelpt-trace/trace_bad_image.json
  lldb/test/API/commands/trace/intelpt-trace/trace_wrong_cpu.json

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89283.299236.patch
Type: text/x-patch
Size: 37824 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201020/4d4f506d/attachment.bin>