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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

I'm not sure if the current revision of the patch reflects the long-term testing strategy. But if so, I'm quite concerned about the proliferation of large binary files in the repo (like ld.so, or the raw trace binary itself). These are opaque blobs that are hard to understand. Also, each time we add one, we're imposing a sizeable tax on everyone working with the llvm-project monorepo.

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
- Check in assembly, and use llvm-mc/clang to generate executables during testing



================
Comment at: lldb/tools/intel-features/intel-pt/Decoder.cpp:251
 
+#include <fstream>
+
----------------
Please group your includes.


================
Comment at: lldb/tools/intel-features/intel-pt/Decoder.cpp:260
   Buffer &pt_buffer = threadTraceInfo.GetPTBuffer();
+
   lldb::SBError error;
----------------
unintentional whitespace diff?


================
Comment at: lldb/tools/intel-features/intel-pt/Decoder.cpp:301
 
+  std::ofstream of("/tmp/multi-file.trace", std::ios::binary | std::ios::out);
+  for (auto bait : pt_buffer)
----------------
Not sure what this is in aid of?


================
Comment at: llvm/include/llvm/Support/Error.h:1016
+/// expected-producer might be more clearly refactored to return an Optional<T>.
+template <typename T> inline void consumeExpected(Expected<T> E) {
+  if (!E)
----------------
Probably best to not add another escape-hatch to permit fast and loose error handling. This seems to be used in a lambda passed to TraverseInstructions. There might be a way to avoid invoking the callback in the case where the expected value is thrown away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283