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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to