[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418154.
zrthxn added a comment.

Resolved many runtime errors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -104,9 +104,12 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread =
+  std::make_shared(DecodedThread(
+  thread_sp, std::vector(), raw_trace_size));
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(make_error(errcode));
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(make_error(errcode));
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(make_error(errcode, insn.ip));
 break;
   }
 
@@ -142,22 +145,23 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time method incorrectly,
 // but the instruction is good though.
-instructions.emplace_back(
+decoded_thread->AppendError(
 make_error(time_error, insn.ip));
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(IntelPTInstruction(insn));
 break;
   }
+
   if (time_error == -pte_no_time) {
 // We simply don't have time information, i.e. None of TSC, MTC or CYC
 // was enabled.
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(IntelPTInstruction(insn));
   } else {
-instructions.emplace_back(insn, time);
+decoded_thread->AppendInstruction(IntelPTInstruction(insn, time));
   }
 }
   }
 
-  return instructions;
+  return decoded_thread;
 }
 
 /// Callback used by libipt for reading the process memory.
@@ -176,8 +180,9 @@
   return bytes_read;
 }
 
-static Expected>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, Process &process,
+TraceIntelPT &trace_intel_pt,
 MutableArrayRef buffer) {
   Expected cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
@@ -203,77 +208,74 @@
   assert(errcode == 0);
   (void)errcode;
 
-  std::vector instructions = DecodeInstructions(*decoder);
+  DecodedThreadSP decoded_thread =
+  DecodeInstructions(*decoder, thread_sp, buffer.size());
 
   pt_insn_free_decoder(decoder);
-  return instructions;
+  return decoded_thread;
 }
+// ---
 
-static Expected>
-DecodeTraceFile(Process &process, TraceIntelPT &trace_intel_pt,
-const FileSpec &trace_file, size_t &raw_trace_size) {
-  ErrorOr> trace_or_error =
-  MemoryBuffer::getFile(trace_file.GetPath());
-  if (std::error_code err = trace_or_error.getError())
-return errorCodeToError(err);
-
-  MemoryBuffer &trace = **trace_or_error;
-  MutableArrayRef trace_data(
-  // The libipt library does not modify the trace buffer, hence the
-  // following cast is safe.
-  reinterpret_cast(const_cast(trace.getBufferStart())),
-  trace.getBufferSize());
-  raw_trace_size = trace_data.size();
-  return DecodeInMemory

[Lldb-commits] [PATCH] D122461: [lldb] Add a fuzzer for target create

[Pavel Labath via Phabricator via lldb-commits]

labath added subscribers: cmtice, labath.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

I like this.




Comment at: lldb/tools/lldb-fuzzer/lldb-fuzzer-target.cpp:24
+extern "C" int LLVMFuzzerTestOneInput(uint8_t *data, size_t size) {
+  auto file = TempFile::Create(data, size);
+  if (!file)

Not an ideal use of auto -- it's clear that this returns some form of TempFile, 
but if this were say Expected, then the code would be incorrect (but 
still compile).


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

https://reviews.llvm.org/D122461

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

[Lldb-commits] [PATCH] D122411: [lldb][AArch64] Fix corefile memory reads when there are non-address bits

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Can we make the core file smaller? Since it's essentially static data, do you 
really need all the mmap, debug info and everything? Could you just take an 
address that is known to be in the core file (smallest you can make), and then 
do a `memory read 0xwhatever` ?




Comment at: 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py:182
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+

even for a core file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122411

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

[Lldb-commits] [PATCH] D122411: [lldb][AArch64] Fix corefile memory reads when there are non-address bits

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 418221.
DavidSpickett added a comment.

- Don't require pointer auth hardware for the corefile test.
- Remove the program file since all we need to do is read memory from the 
corefile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122411

Files:
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/corefile
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c


Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
===
--- lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
@@ -13,6 +13,13 @@
   if (buf == MAP_FAILED)
 return 1;
 
+  // Some known values to go in the corefile, since we cannot
+  // write to corefile memory.
+  buf[0] = 'L';
+  buf[1] = 'L';
+  buf[2] = 'D';
+  buf[3] = 'B';
+
 #define sign_ptr(ptr) __asm__ __volatile__("pacdza %0" : "=r"(ptr) : "r"(ptr))
 
   // Set top byte to something.
@@ -21,5 +28,8 @@
   // Address is now:
   // <8 bit top byte tag>
 
+  // Uncomment this line to crash and generate a corefile.
+  //*(char*)0 = 0;
+
   return 0; // Set break point at this line.
 }
Index: 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
===
--- 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
+++ 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
@@ -175,3 +175,19 @@
 
 if not found_read_buf:
 self.fail("Did not find any reads of buf.")
+
+@skipIfLLVMTargetMissing("AArch64")
+def test_non_address_bit_memory_corefile(self):
+self.runCmd("target create --core corefile")
+
+self.expect("thread list", substrs=['stopped',
+'stop reason = signal SIGSEGV'])
+
+# No caching (the program/corefile are the cache) and no writing
+# to memory. So just check that tagged/untagged addresses read
+# the same location.
+
+expected = ["4c 4c 44 42", "LLDB"]
+# These are known addresses in the corefile.
+self.expect("memory read 0x90027000", substrs=expected)
+self.expect("memory read 0xff7790027000", substrs=expected)
Index: lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
===
--- lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
@@ -794,14 +794,20 @@
 // Reads code or data address mask for the current Linux process.
 static lldb::addr_t ReadLinuxProcessAddressMask(lldb::ProcessSP process_sp,
 llvm::StringRef reg_name) {
-  // Linux configures user-space virtual addresses with top byte ignored.
-  // We set default value of mask such that top byte is masked out.
-  uint64_t address_mask = ~((1ULL << 56) - 1);
-  // If Pointer Authentication feature is enabled then Linux exposes
-  // PAC data and code mask register. Try reading relevant register
-  // below and merge it with default address mask calculated above.
+  // 0 means there isn't a mask or it has not been read yet.
+  // We do not return the top byte mask unless thread_sp is valid.
+  // This prevents calls to this function before the thread is setup locking
+  // in the value to just the top byte mask, in cases where pointer 
authentication
+  // might also be active.
+  uint64_t address_mask = 0;
   lldb::ThreadSP thread_sp = process_sp->GetThreadList().GetSelectedThread();
   if (thread_sp) {
+// Linux configures user-space virtual addresses with top byte ignored.
+// We set default value of mask such that top byte is masked out.
+address_mask = ~((1ULL << 56) - 1);
+// If Pointer Authentication feature is enabled then Linux exposes
+// PAC data and code mask register. Try reading relevant register
+// below and merge it with default address mask calculated above.
 lldb::RegisterContextSP reg_ctx_sp = thread_sp->GetRegisterContext();
 if (reg_ctx_sp) {
   const RegisterInfo *reg_info =


Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
===
--- lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
@@ -13,6 +13,13 @@
   if (buf == MAP_FAILED)
 return 1;
 
+  // Some known values to go in the corefile, since we cannot
+  // wri

[Lldb-commits] [PATCH] D122411: [lldb][AArch64] Fix corefile memory reads when there are non-address bits

[David Spickett via Phabricator via lldb-commits]

DavidSpickett marked an inline comment as done.
DavidSpickett added a comment.

> Can we make the core file smaller?

Yes we can.

I need to find a good way to do that, my first instinct was to strip segments 
from it but perhaps I'm barking up the wrong tree there. Is there a usual way 
to make a minimal core?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122411

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

[Lldb-commits] [PATCH] D122486: [lldb] Fix building for mingw after changes to sigtstp_handler

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo created this revision.
mstorsjo added a reviewer: labath.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

Some signal handlers are set up within an !_MSC_VER condition,
i.e. omitted in MSVC builds but included in mingw builds. Previously
sigtstp_handler was defined in all builds, but since
4bcadd66864bf4af929ac8753a51d7ad408cdef0 
 / D120320 
 it's only
defined non platforms other than Windows.

This applies a matching ifdef around the usage of sigtstp_handler.

Alternatively the ifdef condition could be changed from !_MSC_VER
to !_WIN32 - I'm unsure whether the other ones that are hooked up
in mingw builds but not in MSVC builds actually ever make a difference
in mingw builds.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122486

Files:
  lldb/tools/driver/Driver.cpp


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -832,7 +832,9 @@
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
+#if !defined(_WIN32)
   signal(SIGTSTP, sigtstp_handler);
+#endif
 #endif
 
   int exit_code = 0;


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -832,7 +832,9 @@
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
+#if !defined(_WIN32)
   signal(SIGTSTP, sigtstp_handler);
+#endif
 #endif
 
   int exit_code = 0;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122486: [lldb] Fix building for mingw after changes to sigtstp_handler

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

> Alternatively the ifdef condition could be changed from !_MSC_VER to !_WIN32 
> - I'm unsure whether the other ones that are hooked up in mingw builds but 
> not in MSVC builds actually ever make a difference in mingw builds.

On second thought, maybe it'd be best to just go down this route, to reduce 
unnecessary differences between the MSVC and mingw builds. If the (more 
officially supported) MSVC build can do without these signals, the mingw build 
should too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122486

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

[Lldb-commits] [PATCH] D122486: [lldb] Fix building for mingw after changes to sigtstp_handler

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo updated this revision to Diff 418244.
mstorsjo added a comment.

Changed the existing ifdef around the signals that were omitted in the MSVC 
build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122486

Files:
  lldb/tools/driver/Driver.cpp


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -829,7 +829,7 @@
   SBHostOS::ThreadCreated("");
 
   signal(SIGINT, sigint_handler);
-#if !defined(_MSC_VER)
+#if !defined(_WIN32)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -829,7 +829,7 @@
   SBHostOS::ThreadCreated("");
 
   signal(SIGINT, sigint_handler);
-#if !defined(_MSC_VER)
+#if !defined(_WIN32)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122411: [lldb][AArch64] Fix corefile memory reads when there are non-address bits

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 418246.
DavidSpickett added a comment.

- Corefile down to 24k with some tweaking of coredump_filter
- Add some prints so when you generate the corefile you know what addresses to 
test

I also found another bug testing this where memory read works but printing
doesnt, like:

  (lldb) p (char*)0x90027000
  (char *) $1 = 0x90027000 "LLDB"
  (lldb) p (char*)0xff0af7ff9000
  (char *) $2 = 0xff0af7ff9000 ""

I think this may use the number of addressable bits setting, that could be 
missing
in a corefile. Planning changes to work on that and add tests for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122411

Files:
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/corefile
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c


Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
===
--- lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
@@ -13,6 +13,13 @@
   if (buf == MAP_FAILED)
 return 1;
 
+  // Some known values to go in the corefile, since we cannot
+  // write to corefile memory.
+  buf[0] = 'L';
+  buf[1] = 'L';
+  buf[2] = 'D';
+  buf[3] = 'B';
+
 #define sign_ptr(ptr) __asm__ __volatile__("pacdza %0" : "=r"(ptr) : "r"(ptr))
 
   // Set top byte to something.
@@ -21,5 +28,11 @@
   // Address is now:
   // <8 bit top byte tag>
 
+  // Uncomment this line to crash and generate a corefile.
+  // Prints so we know what fixed address to look for in testing.
+  // printf("buf: %p\n", buf);
+  // printf("buf_with_non_address: %p\n", buf_with_non_address);
+  // *(char*)0 = 0;
+
   return 0; // Set break point at this line.
 }
Index: 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
===
--- 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
+++ 
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
@@ -175,3 +175,19 @@
 
 if not found_read_buf:
 self.fail("Did not find any reads of buf.")
+
+@skipIfLLVMTargetMissing("AArch64")
+def test_non_address_bit_memory_corefile(self):
+self.runCmd("target create --core corefile")
+
+self.expect("thread list", substrs=['stopped',
+'stop reason = signal SIGSEGV'])
+
+# No caching (the program/corefile are the cache) and no writing
+# to memory. So just check that tagged/untagged addresses read
+# the same location.
+
+expected = ["4c 4c 44 42", "LLDB"]
+# These are known addresses in the corefile.
+self.expect("memory read 0xa75a5000", substrs=expected)
+self.expect("memory read 0xff0ba75a5000", substrs=expected)
Index: lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
===
--- lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
@@ -794,14 +794,20 @@
 // Reads code or data address mask for the current Linux process.
 static lldb::addr_t ReadLinuxProcessAddressMask(lldb::ProcessSP process_sp,
 llvm::StringRef reg_name) {
-  // Linux configures user-space virtual addresses with top byte ignored.
-  // We set default value of mask such that top byte is masked out.
-  uint64_t address_mask = ~((1ULL << 56) - 1);
-  // If Pointer Authentication feature is enabled then Linux exposes
-  // PAC data and code mask register. Try reading relevant register
-  // below and merge it with default address mask calculated above.
+  // 0 means there isn't a mask or it has not been read yet.
+  // We do not return the top byte mask unless thread_sp is valid.
+  // This prevents calls to this function before the thread is setup locking
+  // in the value to just the top byte mask, in cases where pointer 
authentication
+  // might also be active.
+  uint64_t address_mask = 0;
   lldb::ThreadSP thread_sp = process_sp->GetThreadList().GetSelectedThread();
   if (thread_sp) {
+// Linux configures user-space virtual addresses with top byte ignored.
+// We set default value of mask such that top byte is masked out.
+address_mask = ~((1ULL << 56) - 1);
+// If Pointer Authentication feature is enabled then Linux exposes
+// PAC data and code mask register. Try reading relevant register
+// below and merge it with default address mask c

[Lldb-commits] [PATCH] D122461: [lldb] Add a fuzzer for target create

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.



Comment at: lldb/tools/lldb-fuzzer/lldb-fuzzer-target.cpp:24
+extern "C" int LLVMFuzzerTestOneInput(uint8_t *data, size_t size) {
+  auto file = TempFile::Create(data, size);
+  if (!file)

labath wrote:
> Not an ideal use of auto -- it's clear that this returns some form of 
> TempFile, but if this were say Expected, then the code would be 
> incorrect (but still compile).
Agreed. Originally I was returning a 
`llvm::Expected>` but the inability to create a temp 
file isn't all that interesting and without the expected the logic can be a lot 
simpler.


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

https://reviews.llvm.org/D122461

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418249.
zrthxn added a comment.

Refactor to use more templates and param packs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -9,6 +9,7 @@
 
 #include "llvm/Support/MemoryBuffer.h"
 
+#include 
 #include "../common/ThreadPostMortemTrace.h"
 #include "DecodedThread.h"
 #include "TraceIntelPT.h"
@@ -104,9 +105,12 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread =
+  std::make_shared(DecodedThread(
+  thread_sp, std::vector(), raw_trace_size));
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +118,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +127,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +146,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time method incorrectly,
 // but the instruction is good though.
-instructions.emplace_back(
-make_error(time_error, insn.ip));
-instructions.emplace_back(insn);
+decoded_thread->AppendError(time_error, insn.ip);
+decoded_thread->AppendInstruction(insn);
 break;
   }
+
   if (time_error == -pte_no_time) {
 // We simply don't have time information, i.e. None of TSC, MTC or CYC
 // was enabled.
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(insn);
   } else {
-instructions.emplace_back(insn, time);
+decoded_thread->AppendInstruction(insn, time);
   }
 }
   }
 
-  return instructions;
+  return decoded_thread;
 }
 
 /// Callback used by libipt for reading the process memory.
@@ -176,8 +180,9 @@
   return bytes_read;
 }
 
-static Expected>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, Process &process,
+TraceIntelPT &trace_intel_pt,
 MutableArrayRef buffer) {
   Expected cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
@@ -203,77 +208,72 @@
   assert(errcode == 0);
   (void)errcode;
 
-  std::vector instructions = DecodeInstructions(*decoder);
+  DecodedThreadSP decoded_thread =
+  DecodeInstructions(*decoder, thread_sp, buffer.size());
 
   pt_insn_free_decoder(decoder);
-  return instructions;
+  return decoded_thread;
 }
+// ---
 
-static Expected>
-DecodeTraceFile(Process &process, TraceIntelPT &trace_intel_pt,
-const FileSpec &trace_file, size_t &raw_trace_size) {
-  ErrorOr> trace_or_error =
-  MemoryBuffer::getFile(trace_file.GetPath());
-  if (std::error_code err = trace_or_error.getError())
-return errorCodeToError(err);
-
-  MemoryBuffer &trace = **trace_or_error;
-  MutableArrayRef trace_data(
-  // The libipt library does not modify the trace buffer, hence the
-  // following cast is safe.
-  reinterpret_cast(const_cast(trace

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:320
+
+  std::vector sorted_keys(keys->GetSize());
+  auto sort_keys = [&sorted_keys](StructuredData::Object *item) -> bool {

JDevlieghere wrote:
> Won't this overflow when you return the example from the summary?
> 
> ```
> {1: "one", 2: "two", 10: "ten"}
> ```
> 
> Would it be easier to populate our own `map` and iterate 
> over that below? 
I don't see why it would overflow 🧐


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [lldb] 61efe14 - [lldb] Add a fuzzer for target creation

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-03-25T09:34:00-07:00
New Revision: 61efe14e21b2c47a848f6d7500ed05af17c64a9a

URL: 
https://github.com/llvm/llvm-project/commit/61efe14e21b2c47a848f6d7500ed05af17c64a9a
DIFF: 
https://github.com/llvm/llvm-project/commit/61efe14e21b2c47a848f6d7500ed05af17c64a9a.diff

LOG: [lldb] Add a fuzzer for target creation

This patch adds a generic fuzzer that interprets inputs as object files
and uses them to create a target in lldb. It is very similar to the
llvm-dwarfdump fuzzer which found a bunch of issues in libObject.

Differential revision: https://reviews.llvm.org/D122461

Added: 
lldb/tools/lldb-fuzzer/CMakeLists.txt
lldb/tools/lldb-fuzzer/lldb-target-fuzzer.cpp
lldb/tools/lldb-fuzzer/utils/CMakeLists.txt
lldb/tools/lldb-fuzzer/utils/TempFile.cpp
lldb/tools/lldb-fuzzer/utils/TempFile.h

Modified: 
lldb/tools/CMakeLists.txt
llvm/docs/FuzzingLLVM.rst

Removed: 




diff  --git a/lldb/tools/CMakeLists.txt b/lldb/tools/CMakeLists.txt
index 1585fd4dc4b9e..16a2c7956aeff 100644
--- a/lldb/tools/CMakeLists.txt
+++ b/lldb/tools/CMakeLists.txt
@@ -6,6 +6,7 @@ add_subdirectory(intel-features)
 # i.e. if a target requires it as dependency. The typical
 # example is `check-lldb`. So, we pass EXCLUDE_FROM_ALL here.
 add_subdirectory(lldb-test EXCLUDE_FROM_ALL)
+add_subdirectory(lldb-fuzzer EXCLUDE_FROM_ALL)
 
 add_lldb_tool_subdirectory(lldb-instr)
 add_lldb_tool_subdirectory(lldb-vscode)

diff  --git a/lldb/tools/lldb-fuzzer/CMakeLists.txt 
b/lldb/tools/lldb-fuzzer/CMakeLists.txt
new file mode 100644
index 0..44df5f193b44a
--- /dev/null
+++ b/lldb/tools/lldb-fuzzer/CMakeLists.txt
@@ -0,0 +1,17 @@
+add_subdirectory(utils)
+
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_llvm_fuzzer(lldb-target-fuzzer
+  EXCLUDE_FROM_ALL
+  lldb-target-fuzzer.cpp
+  )
+
+target_link_libraries(lldb-target-fuzzer
+  PRIVATE
+  liblldb
+  lldbFuzzerUtils
+  )
+

diff  --git a/lldb/tools/lldb-fuzzer/lldb-target-fuzzer.cpp 
b/lldb/tools/lldb-fuzzer/lldb-target-fuzzer.cpp
new file mode 100644
index 0..82487bb224f09
--- /dev/null
+++ b/lldb/tools/lldb-fuzzer/lldb-target-fuzzer.cpp
@@ -0,0 +1,35 @@
+//===-- lldb-target-fuzzer.cpp - Fuzz target creation 
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include 
+
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBTarget.h"
+
+using namespace lldb;
+using namespace lldb_fuzzer;
+using namespace llvm;
+
+extern "C" int LLVMFuzzerInitialize(int *argc, char ***argv) {
+  SBDebugger::Initialize();
+  return 0;
+}
+
+extern "C" int LLVMFuzzerTestOneInput(uint8_t *data, size_t size) {
+  std::unique_ptr file = TempFile::Create(data, size);
+  if (!file)
+return 1;
+
+  SBDebugger debugger = SBDebugger::Create(false);
+  SBTarget target = debugger.CreateTarget(file->GetPath().data());
+  debugger.DeleteTarget(target);
+  SBDebugger::Destroy(debugger);
+  SBModule::GarbageCollectAllocatedModules();
+
+  return 0;
+}

diff  --git a/lldb/tools/lldb-fuzzer/utils/CMakeLists.txt 
b/lldb/tools/lldb-fuzzer/utils/CMakeLists.txt
new file mode 100644
index 0..2c99c79e8aefe
--- /dev/null
+++ b/lldb/tools/lldb-fuzzer/utils/CMakeLists.txt
@@ -0,0 +1,6 @@
+add_lldb_library(lldbFuzzerUtils
+  TempFile.cpp
+
+  LINK_COMPONENTS
+Support
+  )

diff  --git a/lldb/tools/lldb-fuzzer/utils/TempFile.cpp 
b/lldb/tools/lldb-fuzzer/utils/TempFile.cpp
new file mode 100644
index 0..c5c16ec19df7a
--- /dev/null
+++ b/lldb/tools/lldb-fuzzer/utils/TempFile.cpp
@@ -0,0 +1,33 @@
+//===-- TempFile.cpp 
--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Support/FileSystem.h"
+#include 
+
+using namespace lldb_fuzzer;
+using namespace llvm;
+
+TempFile::~TempFile() {
+  if (!m_path.empty())
+sys::fs::remove(m_path.str(), true);
+}
+
+std::unique_ptr TempFile::Create(uint8_t *data, size_t size) {
+  int fd;
+  std::unique_ptr temp_file = std::make_unique();
+  std::error_code ec = sys::fs::createTemporaryFile("lldb-fuzzer", "input", fd,
+temp_file->m_path);
+  if (ec)
+return nullptr;
+
+  raw_fd_ostream os(fd, true);
+  os.write(reinterpret_cast(data), size);
+  os.close();
+
+  return temp_file;
+}

diff  --git a/lldb/tools/lldb-fuzzer/utils/TempFile.h 
b/lldb/tools/lldb-fuzzer/utils/Temp

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:320
+
+  std::vector sorted_keys(keys->GetSize());
+  auto sort_keys = [&sorted_keys](StructuredData::Object *item) -> bool {

mib wrote:
> JDevlieghere wrote:
> > Won't this overflow when you return the example from the summary?
> > 
> > ```
> > {1: "one", 2: "two", 10: "ten"}
> > ```
> > 
> > Would it be easier to populate our own `map` and iterate 
> > over that below? 
> I don't see why it would overflow 🧐
You create a vector with 3 elements:

```
std::vector sorted_keys(keys->GetSize());
```

But you use the key from the dictionary to index the vector:

```
sorted_keys[0] = "";
sorted_keys[1] = "one";
sorted_keys[2] = "two";
-- end of vector --
sorted_keys[10] = "ten"; 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

much closer! I'm glad you are starting to understand the patterns we use for 
this kind of code




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:13
 #include 
+#include 
+#include 

delete this



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:14
+#include 
+#include 
 

you don't need to import it. Maybe your c++ vscode extension has been 
autoimporting this one, in which case it's better to disable that feature.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:134-137
+  m_errors.insert(std::pair>(
+  m_instructions.size(), 
+  std::move(info)
+  ));

emplace will prevent unnecessary copies and also doesn't need you to pass a pair



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:142-144
+  return m_raw_trace_size +
+ IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() +
+ sizeof(DecodedThread);

in order to have correct formatting all along, you need to use git 
clang-format: https://llvm.org/docs/Contributing.html#format-patches

follow that guide. Whenever you are going to submit a patch, first run git 
clang-format and it will format your code correctly, so that you never again 
have to lose time doing that. It can even format comments



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:143-144
+  return m_raw_trace_size +
+ IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() +
+ sizeof(DecodedThread);
 }

here you also need to ask for the size of the DenseMap



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:42-45
+  // /// \param[in] string_error
+  // /// StringError to copy from
+  // IntelPTError(StringError string_error);
+

delete commented code



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:76
   /// libipt errors should use the underlying \a IntelPTError class.
-  IntelPTInstruction(llvm::Error err);
+  IntelPTInstruction(llvm::Error &err);
 

Pass the error by const reference, because we don't modify it



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:141
 
-  /// Get the instructions from the decoded trace. Some of them might indicate
-  /// errors (i.e. gaps) in the trace.
+  /// Get the instructions from the decoded trace.
   ///





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:150
+  /// \return
+  ///   The error or \a llvm::Error::success if the given index 
+  ///   points to a valid instruction.





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:155
+  /// Append a successfully decoded instruction.
+  void AppendInstruction(IntelPTInstruction ins);
+

remove this one. We need the version that accepts a parameter pack, as we 
discussed offline



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157-158
+
+  /// Append a decoding error (i.e. an instruction that failed to be decoded).
+  void AppendError(llvm::Error err);
+

same here



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:111-112
+  DecodedThreadSP decoded_thread =
+  std::make_shared(DecodedThread(
+  thread_sp, std::vector(), raw_trace_size));
 

the magic of make_shared is that it uses parameter packs, so that it only 
constructs the object right where it'll store it, thus preventing unnecessary 
copies



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:184
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, Process &process,
+TraceIntelPT &trace_intel_pt,

don't pass the process, because we can get it from the thread_sp object



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:207
 
   int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
   assert(errcode == 0);

if the compiler complains mentioning that the Process pointer you are passing 
is const, you can do an unsafe cast that removes the const qualifier and write 
a comment here. I hope you don't need it anyway



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:235
   if (!buffer)
-return buffer.takeError();
-  raw_trace_size = buffer->size();
-  if (Expected cpu_info = trace.GetCPUInfo())
-return DecodeInMemoryTrace(*thread.GetProcess(), trace,
-   MutableArrayRef(*buffer));
+return DecodedThread(m_thread_sp, buffer.takeError()).shared_fr

[Lldb-commits] [lldb] 8f7db76 - [lldb] Conditionalize target_link_libraries on the target

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-03-25T09:50:34-07:00
New Revision: 8f7db763ef7f30ec952bdb00adb6f99f980cb427

URL: 
https://github.com/llvm/llvm-project/commit/8f7db763ef7f30ec952bdb00adb6f99f980cb427
DIFF: 
https://github.com/llvm/llvm-project/commit/8f7db763ef7f30ec952bdb00adb6f99f980cb427.diff

LOG: [lldb] Conditionalize target_link_libraries on the target

Fixes "Cannot specify link libraries for target "lldb-target-fuzzer"
which is not built by this project." Normally that's taken care of by
add_llvm_fuzzer but we need target_link_libraries for liblldb and our
utility library.

Added: 


Modified: 
lldb/tools/lldb-fuzzer/CMakeLists.txt

Removed: 




diff  --git a/lldb/tools/lldb-fuzzer/CMakeLists.txt 
b/lldb/tools/lldb-fuzzer/CMakeLists.txt
index 44df5f193b44a..b87e08720b59b 100644
--- a/lldb/tools/lldb-fuzzer/CMakeLists.txt
+++ b/lldb/tools/lldb-fuzzer/CMakeLists.txt
@@ -9,9 +9,10 @@ add_llvm_fuzzer(lldb-target-fuzzer
   lldb-target-fuzzer.cpp
   )
 
-target_link_libraries(lldb-target-fuzzer
-  PRIVATE
-  liblldb
-  lldbFuzzerUtils
-  )
-
+if(TARGET lldb-target-fuzzer)
+  target_link_libraries(lldb-target-fuzzer
+PRIVATE
+liblldb
+lldbFuzzerUtils
+)
+endif()



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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn marked 15 inline comments as done.
zrthxn added a comment.

Before refactor

  thread #1: tid = 37275
Raw trace size: 4 KiB
Total number of instructions: 21
Total approximate memory usage: 5.38 KiB

After refactor

  (lldb) thread trace dump info
  Trace technology: intel-pt
  
  thread #1: tid = 13690
Raw trace size: 4 KiB
Total number of instructions: 20
Total approximate memory usage: 5.34 KiB


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:320
+
+  std::vector sorted_keys(keys->GetSize());
+  auto sort_keys = [&sorted_keys](StructuredData::Object *item) -> bool {

JDevlieghere wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > Won't this overflow when you return the example from the summary?
> > > 
> > > ```
> > > {1: "one", 2: "two", 10: "ten"}
> > > ```
> > > 
> > > Would it be easier to populate our own `map` and 
> > > iterate over that below? 
> > I don't see why it would overflow 🧐
> You create a vector with 3 elements:
> 
> ```
> std::vector sorted_keys(keys->GetSize());
> ```
> 
> But you use the key from the dictionary to index the vector:
> 
> ```
> sorted_keys[0] = "";
> sorted_keys[1] = "one";
> sorted_keys[2] = "two";
> -- end of vector --
> sorted_keys[10] = "ten"; 
> ```
You're right! I didn't hit this because in the Scripted Process case, indices 
are 0 based, so that works perfectly but I guess I could use a map to prevent a 
potential overflow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418299.
zrthxn added a comment.

Incoporate more feedback,
Only parameter pack isnt done yet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -104,9 +105,11 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread = std::make_shared(
+  thread_sp, std::vector(), raw_trace_size);
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +145,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time method incorrectly,
 // but the instruction is good though.
-instructions.emplace_back(
-make_error(time_error, insn.ip));
-instructions.emplace_back(insn);
+decoded_thread->AppendError(time_error, insn.ip);
+decoded_thread->AppendInstruction(insn);
 break;
   }
+
   if (time_error == -pte_no_time) {
 // We simply don't have time information, i.e. None of TSC, MTC or CYC
 // was enabled.
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(insn);
   } else {
-instructions.emplace_back(insn, time);
+decoded_thread->AppendInstruction(insn, time);
   }
 }
   }
 
-  return instructions;
+  return decoded_thread;
 }
 
 /// Callback used by libipt for reading the process memory.
@@ -176,8 +179,8 @@
   return bytes_read;
 }
 
-static Expected>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, TraceIntelPT &trace_intel_pt,
 MutableArrayRef buffer) {
   Expected cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
@@ -199,81 +202,71 @@
 
   pt_image *image = pt_insn_get_image(decoder);
 
-  int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
+  // ProcessSP process = thread_sp->GetProcess();
+  int errcode = pt_image_set_callback(image, ReadProcessMemory,
+  thread_sp->GetProcess().get());
   assert(errcode == 0);
   (void)errcode;
 
-  std::vector instructions = DecodeInstructions(*decoder);
+  DecodedThreadSP decoded_thread =
+  DecodeInstructions(*decoder, thread_sp, buffer.size());
 
   pt_insn_free_decoder(decoder);
-  return instructions;
+  return decoded_thread;
 }
+// ---
 
-static Expected>
-DecodeTraceFile(Process &process, TraceIntelPT &trace_intel_pt,
-const FileSpec &trace_file, size_t &raw_trace_size) {
-  ErrorOr> trace_or_error =
-  MemoryBuffer::getFile(trace_file.GetPath());
-  if (std::error_code err = 

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418306.
zrthxn added a comment.

Finalize diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -104,9 +105,11 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread = std::make_shared(
+  thread_sp, std::vector(), raw_trace_size);
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +145,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time method incorrectly,
 // but the instruction is good though.
-instructions.emplace_back(
-make_error(time_error, insn.ip));
-instructions.emplace_back(insn);
+decoded_thread->AppendError(time_error, insn.ip);
+decoded_thread->AppendInstruction(insn);
 break;
   }
+
   if (time_error == -pte_no_time) {
 // We simply don't have time information, i.e. None of TSC, MTC or CYC
 // was enabled.
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(insn);
   } else {
-instructions.emplace_back(insn, time);
+decoded_thread->AppendInstruction(insn, time);
   }
 }
   }
 
-  return instructions;
+  return decoded_thread;
 }
 
 /// Callback used by libipt for reading the process memory.
@@ -176,8 +179,8 @@
   return bytes_read;
 }
 
-static Expected>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, TraceIntelPT &trace_intel_pt,
 MutableArrayRef buffer) {
   Expected cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
@@ -199,81 +202,71 @@
 
   pt_image *image = pt_insn_get_image(decoder);
 
-  int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
+  // ProcessSP process = thread_sp->GetProcess();
+  int errcode = pt_image_set_callback(image, ReadProcessMemory,
+  thread_sp->GetProcess().get());
   assert(errcode == 0);
   (void)errcode;
 
-  std::vector instructions = DecodeInstructions(*decoder);
+  DecodedThreadSP decoded_thread =
+  DecodeInstructions(*decoder, thread_sp, buffer.size());
 
   pt_insn_free_decoder(decoder);
-  return instructions;
+  return decoded_thread;
 }
+// ---
 
-static Expected>
-DecodeTraceFile(Process &process, TraceIntelPT &trace_intel_pt,
-const FileSpec &trace_file, size_t &raw_trace_size) {
-  ErrorOr> trace_or_error =
-  MemoryBuffer::getFile(trace_file.GetPath());
-  if (std::error_code err = trace_or_error.getError())
-return errorCo

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418309.
zrthxn added a comment.

Added average memory per instruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -112,12 +112,15 @@
   }
   s.Printf("\n");
 
+  size_t insn_len = Decode(thread)->GetInstructions().size();
   size_t mem_used = Decode(thread)->CalculateApproximateMemoryUsage();
+  
   s.Printf("  Raw trace size: %zu KiB\n", *raw_size / 1024);
-  s.Printf("  Total number of instructions: %zu\n",
-   Decode(thread)->GetInstructions().size());
+  s.Printf("  Total number of instructions: %zu\n", insn_len);
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
(double)mem_used / 1024);
+  s.Printf("  Average memory usage per instruction: %0.2lf KiB\n",
+   ((double)mem_used/insn_len) / 1024);
   return;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -104,9 +105,11 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread = std::make_shared(
+  thread_sp, std::vector(), raw_trace_size);
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +145,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time method incorrectly,
 // but the instruction is good though.
-instructions.emplace_back(
-make_error(time_error, insn.ip));
-instructions.emplace_back(insn);
+decoded_thread->AppendError(time_error, insn.ip);
+decoded_thread->AppendInstruction(insn);
 break;
   }
+
   if (time_error == -pte_no_time) {
 // We simply don't have time information, i.e. None of TSC, MTC or CYC
 // was enabled.
-instructions.emplace_back(insn);
+decoded_thread->AppendInstruction(insn);
   } else {
-instructions.emplace_back(insn, time);
+decoded_thread->AppendInstruction(insn, time);
   }
 }
   }
 
-  return instructions;
+  return decoded_thread;
 }
 
 /// Callback used by libipt for reading the process memory.
@@ -176,8 +179,8 @@
   return bytes_read;
 }
 
-static Expected>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static Expected
+DecodeInMemoryTrace(const ThreadSP &thread_sp, TraceIntelPT &trace_intel_pt,
 MutableArrayRef buffer) {
   Expected cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
@@ -1

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
   m_pt_insn.iclass = ptic_error;
+  m_is_error = true;
 }

Is this boolean necessary? In the case of an error, the other two fields also 
indicate an error so this doesn't seem to add much information.
If we remove it, you can just update IsError to check the other two fields 
accordingly.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:40
 
-IntelPTInstruction::IntelPTInstruction(llvm::Error err) {
-  llvm::handleAllErrors(std::move(err),
-[&](std::unique_ptr info) {
-  m_error = std::move(info);
-});
+IntelPTInstruction::IntelPTInstruction(Error &err) {
   m_pt_insn.ip = LLDB_INVALID_ADDRESS;

Feels kinda weird that the `err` param isn't being used? Not sure if it would 
be preferred but another option would be to make the default constructor 
construct an error instruction.
Passing the error is more clear that this constructor will create an error 
instruction, but then it feels a bit unnecessary to pass it since it's now 
unused. I'm a little torn so would be curious to hear others opinions (:



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156
+  template  void AppendInstruction(Ts... __args) {
+m_instructions.emplace_back(std::move(IntelPTInstruction(__args...)));
+  }

Should we be using `std::forward()` here? Same question for the `AppendError` 
function



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

nit: should we update this to use the error map? I don't think there's a 
significant difference performance wise, but the code would be a little cleaner 
imo and consistent with how `GetError()` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

jj10306 wrote:
> nit: should we update this to use the error map? I don't think there's a 
> significant difference performance wise, but the code would be a little 
> cleaner imo and consistent with how `GetError()` works.
That would sort of look like this I think

```
  if 
(m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA())
return false;
  else true;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 418325.
mib added a comment.

Implement @JDevlieghere suggestions:

- Use a `std::map` to sort the thread info dictionary while 
preventing potential overflow


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

https://reviews.llvm.org/D122429

Files:
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@
 
   StructuredData::DictionarySP thread_info_sp = 
GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match 
the
-  // thread indices.
-
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
 "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key element,
+  // thread info comes ordered alphabetically, instead of numerically, so we
+  // need to sort the thread indices before creating thread.
+
+  StructuredData::ArraySP keys = thread_info_sp->GetKeys();
+
+  std::map sorted_threads;
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;
+
+// Make sure the provided index is actually an integer
+if (!llvm::to_integer(key, idx))
+  return false;
+
+sorted_threads[idx] = thread_info_sp->GetValueForKey(key);
+return true;
+  };
+
+  size_t thread_count = thread_info_sp->GetSize();
+
+  if (!keys->ForEach(sort_keys) || sorted_threads.size() != thread_count)
+// Might be worth showing the unsorted thread list instead of return early.
+return ScriptedInterface::ErrorWithMessage(
+LLVM_PRETTY_FUNCTION, "Couldn't sort thread list.", error);
+
   auto create_scripted_thread =
-  [this, &old_thread_list, &error,
-   &new_thread_list](ConstString key, StructuredData::Object *val) -> bool 
{
-if (!val)
-  return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
+  [this, &error, &new_thread_list](
+  const std::pair pair) -> bool {
+size_t idx = pair.first;
+StructuredData::ObjectSP object_sp = pair.second;
 
-lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-if (!llvm::to_integer(key.AsCString(), tid))
+if (!object_sp)
   return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread id", error);
-
-if (ThreadSP thread_sp =
-old_thread_list.FindThreadByID(tid, false /*=can_update*/)) {
-  // If the thread was already in the old_thread_list,
-  // just add it back to the new_thread_list.
-  new_thread_list.AddThread(thread_sp);
-  return true;
-}
+  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
 
-auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
+auto thread_or_error =
+ScriptedThread::Create(*this, object_sp->GetAsGeneric());
 
 if (!thread_or_error)
   return ScriptedInterface::ErrorWithMessage(
@@ -345,8 +365,7 @@
 if (!reg_ctx_sp)
   return ScriptedInterface::ErrorWithMessage(
   LLVM_PRETTY_FUNCTION,
-  llvm::Twine("Invalid Register Context for thread " +
-  llvm::Twine(key.AsCString()))
+  llvm::Twine("Invalid Register Context for thread " + 
llvm::Twine(idx))
   .str(),
   error);
 
@@ -355,7 +374,7 @@
 return true;
   };
 
-  thread_info_sp->ForEach(create_scripted_thread);
+  llvm::for_each(sorted_threads, create_scripted_thread);
 
   return new_thread_list.GetSize(false) > 0;
 }


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@
 
   StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match the
-  // thread indices.
-
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
 "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key element,
+  // thread info comes ordered alphabeti

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418327.
zrthxn added a comment.

Updated tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -37,8 +37,9 @@
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
-  Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total number of instructions: 22
+  Total approximate memory usage: 6.38 KiB
+  Average memory usage per instruction: 296 bytes'''])
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInfo.py
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -39,5 +39,6 @@
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
-  Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total number of instructions: 22
+  Total approximate memory usage: 6.38 KiB
+  Average memory usage per instruction: 296 bytes'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -112,12 +112,15 @@
   }
   s.Printf("\n");
 
+  size_t insn_len = Decode(thread)->GetInstructions().size();
   size_t mem_used = Decode(thread)->CalculateApproximateMemoryUsage();
+
   s.Printf("  Raw trace size: %zu KiB\n", *raw_size / 1024);
-  s.Printf("  Total number of instructions: %zu\n",
-   Decode(thread)->GetInstructions().size());
+  s.Printf("  Total number of instructions: %zu\n", insn_len);
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
(double)mem_used / 1024);
+  s.Printf("  Average memory usage per instruction: %zu bytes\n",
+   mem_used / insn_len);
   return;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -104,9 +105,11 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread = std::make_shared(
+  thread_sp, std::vector(), raw_trace_size);
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +145,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_insn_time m

[Lldb-commits] [PATCH] D122426: [lldb/Utility] Make `StructuredData::Dictionary::GetKeys` return an `Array`

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Makes sense. LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122426

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:321-342
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;

Instead of this callback, can we simplify the code by iterating over the keys 
and populate the map that way?


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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:321-342
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;

JDevlieghere wrote:
> Instead of this callback, can we simplify the code by iterating over the keys 
> and populate the map that way?
We don't have iterators to iterate over `StructuredData` containers with a 
range-based loop, so we can only iterate over them using indices ...

I find it ungracious to do it that way because we have to call 
`GetItemAtIndex(idx)` to get the `StructuredData::Object` element from the 
array, that's why I used the `ForEach` method instead.

I think we can improve StructuredData containers to be more customizable and 
operate better with the `std`/`adl` utilities, it should be done separately.


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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:321-342
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;

mib wrote:
> JDevlieghere wrote:
> > Instead of this callback, can we simplify the code by iterating over the 
> > keys and populate the map that way?
> We don't have iterators to iterate over `StructuredData` containers with a 
> range-based loop, so we can only iterate over them using indices ...
> 
> I find it ungracious to do it that way because we have to call 
> `GetItemAtIndex(idx)` to get the `StructuredData::Object` element from the 
> array, that's why I used the `ForEach` method instead.
> 
> I think we can improve StructuredData containers to be more customizable and 
> operate better with the `std`/`adl` utilities, it should be done separately.
I think we've discussed this offline, but if we made the 
`StructuredData::Array` templated (with a default template parameter), we could 
prevent having to sort ourself the thread dictionary, and loop twice over it 
(which annoys me quite a bit 😅😅)


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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGMT




Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:321-342
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;

mib wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > Instead of this callback, can we simplify the code by iterating over the 
> > > keys and populate the map that way?
> > We don't have iterators to iterate over `StructuredData` containers with a 
> > range-based loop, so we can only iterate over them using indices ...
> > 
> > I find it ungracious to do it that way because we have to call 
> > `GetItemAtIndex(idx)` to get the `StructuredData::Object` element from the 
> > array, that's why I used the `ForEach` method instead.
> > 
> > I think we can improve StructuredData containers to be more customizable 
> > and operate better with the `std`/`adl` utilities, it should be done 
> > separately.
> I think we've discussed this offline, but if we made the 
> `StructuredData::Array` templated (with a default template parameter), we 
> could prevent having to sort ourself the thread dictionary, and loop twice 
> over it (which annoys me quite a bit 😅😅)
Okay, that's unfortunate. Thanks for the explanation!


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

https://reviews.llvm.org/D122429

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

[Lldb-commits] [lldb] 12301d6 - [lldb/crashlog] Parse thread fields and pass it to crashlog scripted process

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-03-25T14:59:50-07:00
New Revision: 12301d616fbcd3bbc78664221256404123a0935f

URL: 
https://github.com/llvm/llvm-project/commit/12301d616fbcd3bbc78664221256404123a0935f
DIFF: 
https://github.com/llvm/llvm-project/commit/12301d616fbcd3bbc78664221256404123a0935f.diff

LOG: [lldb/crashlog] Parse thread fields and pass it to crashlog scripted 
process

Previously, the ScriptedThread used the thread index as the thread id.

This patch parses the crashlog json to extract the actual thread "id" value,
and passes this information to the Crashlog ScriptedProcess blueprint,
to create a higher fidelity ScriptedThreaad.

It also updates the blueprint to show the thread name and thread queue.

Finally, this patch updates the interactive crashlog test to reflect
these changes.

rdar://90327854

Differential Revision: https://reviews.llvm.org/D122422

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/examples/python/scripted_process/crashlog_scripted_process.py
lldb/examples/python/scripted_process/scripted_process.py

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index aad58c57d58f2..e0bd52d8711ef 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -76,10 +76,12 @@ class Thread:
 
 def __init__(self, index, app_specific_backtrace):
 self.index = index
+self.id = index
 self.frames = list()
 self.idents = list()
 self.registers = dict()
 self.reason = None
+self.name = None
 self.queue = None
 self.crashed = False
 self.app_specific_backtrace = app_specific_backtrace
@@ -521,14 +523,18 @@ def parse_threads(self, json_threads):
 for json_thread in json_threads:
 thread = self.crashlog.Thread(idx, False)
 if 'name' in json_thread:
+thread.name = json_thread['name']
 thread.reason = json_thread['name']
+if 'id' in json_thread:
+thread.id = int(json_thread['id'])
 if json_thread.get('triggered', False):
 self.crashlog.crashed_thread_idx = idx
 thread.crashed = True
 if 'threadState' in json_thread:
 thread.registers = self.parse_thread_registers(
 json_thread['threadState'])
-thread.queue = json_thread.get('queue')
+if 'queue' in json_thread:
+thread.queue = json_thread.get('queue')
 self.parse_frames(thread, json_thread.get('frames', []))
 self.crashlog.threads.append(thread)
 idx += 1

diff  --git 
a/lldb/examples/python/scripted_process/crashlog_scripted_process.py 
b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
index 770fb52b8c990..f3b11e3699d98 100644
--- a/lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -135,15 +135,12 @@ def __init__(self, process, args, crashlog_thread):
 
 self.backing_thread = crashlog_thread
 self.idx = self.backing_thread.index
+self.tid = self.backing_thread.id
+self.name = self.backing_thread.name
+self.queue = self.backing_thread.queue
 self.has_crashed = (self.scripted_process.crashed_thread_idx == 
self.idx)
 self.create_stackframes()
 
-def get_thread_id(self) -> int:
-return self.idx
-
-def get_name(self) -> str:
-return CrashLogScriptedThread.__name__ + ".thread-" + str(self.idx)
-
 def get_state(self):
 if not self.has_crashed:
 return lldb.eStateStopped

diff  --git a/lldb/examples/python/scripted_process/scripted_process.py 
b/lldb/examples/python/scripted_process/scripted_process.py
index 5969737c8b4bd..b9388ae25e466 100644
--- a/lldb/examples/python/scripted_process/scripted_process.py
+++ b/lldb/examples/python/scripted_process/scripted_process.py
@@ -219,8 +219,8 @@ def __init__(self, scripted_process, args):
 self.scripted_process = None
 self.process = None
 self.args = None
-
-self.id = None
+self.idx = 0
+self.tid = 0
 self.idx = None
 self.name = None
 self.queue = None
@@ -236,24 +236,29 @@ def __init__(self, scripted_process, args):
 self.process = self.target.GetProcess()
 self.get_register_info()
 
+def get_thread_idx(self):
+""" Get the scripted thread index.
+
+Returns:
+int: The index of the scripted thread in the scripted process.
+"""
+return self.idx
 
-@abstractmeth

[Lldb-commits] [lldb] 29f3636 - [lldb/Utility] Make StructuredData::Dictionary::GetKeys return an Array

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-03-25T14:59:50-07:00
New Revision: 29f363611dd4b9b898230cde60bacf71f9581343

URL: 
https://github.com/llvm/llvm-project/commit/29f363611dd4b9b898230cde60bacf71f9581343
DIFF: 
https://github.com/llvm/llvm-project/commit/29f363611dd4b9b898230cde60bacf71f9581343.diff

LOG: [lldb/Utility] Make StructuredData::Dictionary::GetKeys return an Array

This patch changes `StructuredData::Dictionary::GetKeys` return type
from an `StructuredData::ObjectSP` to a `StructuredData::ArraySP`.

The function already stored the keys in an array but implicitely upcasted
it to an `ObjectSP`, which required the user to convert it again to a
Array object to access each element.

Since we know the keys should be held by an iterable container, it makes
more sense to return the allocated ArraySP as-is.

Differential Revision: https://reviews.llvm.org/D122426

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/include/lldb/Utility/StructuredData.h

Removed: 




diff  --git a/lldb/include/lldb/Utility/StructuredData.h 
b/lldb/include/lldb/Utility/StructuredData.h
index 11eee92f8c3b8..9f6300f4f115b 100644
--- a/lldb/include/lldb/Utility/StructuredData.h
+++ b/lldb/include/lldb/Utility/StructuredData.h
@@ -375,15 +375,15 @@ class StructuredData {
   }
 }
 
-ObjectSP GetKeys() const {
-  auto object_sp = std::make_shared();
+ArraySP GetKeys() const {
+  auto array_sp = std::make_shared();
   collection::const_iterator iter;
   for (iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
 auto key_object_sp = std::make_shared();
 key_object_sp->SetValue(iter->first.AsCString());
-object_sp->Push(key_object_sp);
+array_sp->Push(key_object_sp);
   }
-  return object_sp;
+  return array_sp;
 }
 
 ObjectSP GetValueForKey(llvm::StringRef key) const {



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

[Lldb-commits] [lldb] 150db43 - [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-03-25T14:59:50-07:00
New Revision: 150db43e412efba0f95ebde81aabd93e7535b909

URL: 
https://github.com/llvm/llvm-project/commit/150db43e412efba0f95ebde81aabd93e7535b909
DIFF: 
https://github.com/llvm/llvm-project/commit/150db43e412efba0f95ebde81aabd93e7535b909.diff

LOG: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

With Scripted Processes, in order to create scripted threads, the blueprint
provides a dictionary that have each thread index as the key with the respective
thread instance as the pair value.

In Python, this is fine because a dictionary key can be of any type including
integer types:

```
>>> {1: "one", 2: "two", 10: "ten"}
{1: 'one', 2: 'two', 10: 'ten'}
```

However, when the python dictionary gets bridged to C++ we convert it to a
`StructuredData::Dictionary` that uses a `std::map`
for storage.

Because `std::map` is an ordered container and ours uses the `ConstString`
type for keys, the thread indices gets converted to strings which makes the
dictionary sorted alphabetically, instead of numerically.

If the ScriptedProcess has 10 threads or more, it causes thread “10”
(and higher) to be after thread “1”, but before thread “2”.

In order to solve this, this sorts the thread info dictionary keys
numerically, before iterating over them to create ScriptedThreads.

rdar://90327854

Differential Revision: https://reviews.llvm.org/D122429

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp 
b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index e36b29db0feb5..cd2d4fe4fd234 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@ bool ScriptedProcess::DoUpdateThreadList(ThreadList 
&old_thread_list,
 
   StructuredData::DictionarySP thread_info_sp = 
GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match 
the
-  // thread indices.
-
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
 "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key element,
+  // thread info comes ordered alphabetically, instead of numerically, so we
+  // need to sort the thread indices before creating thread.
+
+  StructuredData::ArraySP keys = thread_info_sp->GetKeys();
+
+  std::map sorted_threads;
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;
+
+// Make sure the provided index is actually an integer
+if (!llvm::to_integer(key, idx))
+  return false;
+
+sorted_threads[idx] = thread_info_sp->GetValueForKey(key);
+return true;
+  };
+
+  size_t thread_count = thread_info_sp->GetSize();
+
+  if (!keys->ForEach(sort_keys) || sorted_threads.size() != thread_count)
+// Might be worth showing the unsorted thread list instead of return early.
+return ScriptedInterface::ErrorWithMessage(
+LLVM_PRETTY_FUNCTION, "Couldn't sort thread list.", error);
+
   auto create_scripted_thread =
-  [this, &old_thread_list, &error,
-   &new_thread_list](ConstString key, StructuredData::Object *val) -> bool 
{
-if (!val)
-  return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
+  [this, &error, &new_thread_list](
+  const std::pair pair) -> bool {
+size_t idx = pair.first;
+StructuredData::ObjectSP object_sp = pair.second;
 
-lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-if (!llvm::to_integer(key.AsCString(), tid))
+if (!object_sp)
   return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread id", error);
-
-if (ThreadSP thread_sp =
-old_thread_list.FindThreadByID(tid, false /*=can_update*/)) {
-  // If the thread was already in the old_thread_list,
-  // just add it back to the new_thread_list.
-  new_thread_list.AddThread(thread_sp);
-  return true;
-}
+  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
 
-auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
+auto thread_or_error =
+ScriptedThread::Create(*this, object_sp->GetAsGeneric());
 
 if (!thread_or_error)
   return ScriptedInterface::ErrorWithMessage(
@@ -345,8 +365,7 @@ bool ScriptedProcess::DoUpdateThrea

[Lldb-commits] [PATCH] D122422: [lldb/crashlog] Parse thread fields and pass it to crashlog scripted process

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG12301d616fbc: [lldb/crashlog] Parse thread fields and pass 
it to crashlog scripted process (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122422

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test

Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
@@ -4,17 +4,17 @@
 
 # RUN: cp %S/Inputs/scripted_crashlog.ips %t.crash
 # RUN: %python %S/patch-crashlog.py --binary %t.out --crashlog %t.crash --offsets '{"main":160, "bar":20, "foo":24}' --json
-# RUN: %lldb %t.out -o 'command script import lldb.macosx.crashlog' -o 'crashlog -a -i %t.crash' 2>&1 -o "bt all" | FileCheck %s
+# RUN: %lldb %t.out -o 'command script import lldb.macosx.crashlog' -o 'crashlog -a -i %t.crash' 2>&1 -o "thread list" -o "bt all" | FileCheck %s
 
 # CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" options on these commands
 
 # CHECK: (lldb) process status
 # CHECK-NEXT: Process 24991 stopped
-# CHECK-NEXT: * thread #3, name = 'CrashLogScriptedThread.thread-2', stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
 # CHECK-NEXT: frame #0: 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar
 
 # CHECK: (lldb) thread backtrace
-# CHECK-NEXT: * thread #3, name = 'CrashLogScriptedThread.thread-2', stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
 # CHECK-NEXT:   * frame #0: 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar
 # CHECK-NEXT: frame #1: 0x0001047f5998 scripted_crashlog_json.test.tmp.out`foo
 # CHECK-NEXT: frame #2: 0x0001047f5b04 scripted_crashlog_json.test.tmp.out`compute_pow
@@ -24,14 +24,21 @@
 # CHECK-NEXT: frame #6: 0x00018bf5326c libsystem_pthread.dylib`_pthread_start
 # CHECK-NEXT: frame #7: 0x00018bf4e08c libsystem_pthread.dylib`thread_start
 
+# CHECK: (lldb) thread list
+# CHECK-NEXT: Process 24991 stopped
+# CHECK-NEXT:  thread #1: tid = 0x4ea840, 0x00018bf17854 libsystem_kernel.dylib`__ulock_wait{{.*}}, queue = 'com.apple.main-thread'
+# CHECK-NEXT:  thread #2: tid = 0x4ea850, 0x0001047f59e8 scripted_crashlog_json.test.tmp.out`call_and_wait
+# CHECK-NEXT: * thread #3: tid = 0x4ea851, 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar{{.*}}, stop reason = EXC_BAD_ACCESS
+
+
 # CHECK: (lldb) bt all
-# CHECK-NEXT:   thread #1, name = 'CrashLogScriptedThread.thread-0'
+# CHECK-NEXT:   thread #1
 # CHECK-NEXT: frame #0: 0x00018bf17854 libsystem_kernel.dylib`__ulock_wait
 # CHECK-NEXT: frame #1: 0x00018bf555a0 libsystem_pthread.dylib`_pthread_join
 # CHECK-NEXT: frame #2: 0x00018beae9c0 libc++.1.dylib`std::__1::thread::join
 # CHECK-NEXT: frame #3: 0x0001047f5bb8 scripted_crashlog_json.test.tmp.out`main
 # CHECK-NEXT: frame #4: 0x000104ae5088 dyld`start
-# CHECK-NEXT:   thread #2, name = 'CrashLogScriptedThread.thread-1'
+# CHECK-NEXT:   thread #2
 # CHECK-NEXT: frame #0: 0x0001047f59e8 scripted_crashlog_json.test.tmp.out`call_and_wait
 # CHECK-NEXT: frame #1: 0x0001047f59d4 scripted_crashlog_json.test.tmp.out`call_and_wait
 # CHECK-NEXT: frame #2: 0x0001047f7690 scripted_crashlog_json.test.tmp.out`decltype
@@ -39,7 +46,7 @@
 # CHECK-NEXT: frame #4: 0x0001047f6d58 scripted_crashlog_json.test.tmp.out`void* std::__1::__thread_proxy
 # CHECK-NEXT: frame #5: 0x00018bf5326c libsystem_pthread.dylib`_pthread_start
 # CHECK-NEXT: frame #6: 0x00018bf4e08c libsystem_pthread.dylib`thread_start
-# CHECK-NEXT: * thread #3, name = 'CrashLogScriptedThread.thread-2', stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
 # CHECK-NEXT:   * frame #0: 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar
 # CHECK-NEXT: frame #1: 0x0001047f5998 scripted_crashlog_json.test.tmp.out`foo
 # CHECK-NEXT: frame #2: 0x0001047f5b04 scripted_crashlog_json.test.tmp.out`compute_pow
Index: lldb/examples/python/scripted_process/scripted_process.py
===
--- lldb/examples/python/scripted_process/scripted_process.py
+++ lldb/examples/python/scripted_process/scripted_process.py
@@ -219,8 +219,8 @@
 self.scripted_process = None
 self.process = None
 self.args = None
-
-self.id = None
+self.idx = 0
+self.tid

[Lldb-commits] [PATCH] D122426: [lldb/Utility] Make `StructuredData::Dictionary::GetKeys` return an `Array`

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG29f363611dd4: [lldb/Utility] Make 
StructuredData::Dictionary::GetKeys return an Array (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122426

Files:
  lldb/include/lldb/Utility/StructuredData.h


Index: lldb/include/lldb/Utility/StructuredData.h
===
--- lldb/include/lldb/Utility/StructuredData.h
+++ lldb/include/lldb/Utility/StructuredData.h
@@ -375,15 +375,15 @@
   }
 }
 
-ObjectSP GetKeys() const {
-  auto object_sp = std::make_shared();
+ArraySP GetKeys() const {
+  auto array_sp = std::make_shared();
   collection::const_iterator iter;
   for (iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
 auto key_object_sp = std::make_shared();
 key_object_sp->SetValue(iter->first.AsCString());
-object_sp->Push(key_object_sp);
+array_sp->Push(key_object_sp);
   }
-  return object_sp;
+  return array_sp;
 }
 
 ObjectSP GetValueForKey(llvm::StringRef key) const {


Index: lldb/include/lldb/Utility/StructuredData.h
===
--- lldb/include/lldb/Utility/StructuredData.h
+++ lldb/include/lldb/Utility/StructuredData.h
@@ -375,15 +375,15 @@
   }
 }
 
-ObjectSP GetKeys() const {
-  auto object_sp = std::make_shared();
+ArraySP GetKeys() const {
+  auto array_sp = std::make_shared();
   collection::const_iterator iter;
   for (iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
 auto key_object_sp = std::make_shared();
 key_object_sp->SetValue(iter->first.AsCString());
-object_sp->Push(key_object_sp);
+array_sp->Push(key_object_sp);
   }
-  return object_sp;
+  return array_sp;
 }
 
 ObjectSP GetValueForKey(llvm::StringRef key) const {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG150db43e412e: [lldb/Plugin] Sort the ScriptedProcess' 
thread list before creating threads (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122429

Files:
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@
 
   StructuredData::DictionarySP thread_info_sp = 
GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match 
the
-  // thread indices.
-
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
 "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key element,
+  // thread info comes ordered alphabetically, instead of numerically, so we
+  // need to sort the thread indices before creating thread.
+
+  StructuredData::ArraySP keys = thread_info_sp->GetKeys();
+
+  std::map sorted_threads;
+  auto sort_keys = [&sorted_threads,
+&thread_info_sp](StructuredData::Object *item) -> bool {
+if (!item)
+  return false;
+
+llvm::StringRef key = item->GetStringValue();
+size_t idx = 0;
+
+// Make sure the provided index is actually an integer
+if (!llvm::to_integer(key, idx))
+  return false;
+
+sorted_threads[idx] = thread_info_sp->GetValueForKey(key);
+return true;
+  };
+
+  size_t thread_count = thread_info_sp->GetSize();
+
+  if (!keys->ForEach(sort_keys) || sorted_threads.size() != thread_count)
+// Might be worth showing the unsorted thread list instead of return early.
+return ScriptedInterface::ErrorWithMessage(
+LLVM_PRETTY_FUNCTION, "Couldn't sort thread list.", error);
+
   auto create_scripted_thread =
-  [this, &old_thread_list, &error,
-   &new_thread_list](ConstString key, StructuredData::Object *val) -> bool 
{
-if (!val)
-  return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
+  [this, &error, &new_thread_list](
+  const std::pair pair) -> bool {
+size_t idx = pair.first;
+StructuredData::ObjectSP object_sp = pair.second;
 
-lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-if (!llvm::to_integer(key.AsCString(), tid))
+if (!object_sp)
   return ScriptedInterface::ErrorWithMessage(
-  LLVM_PRETTY_FUNCTION, "Invalid thread id", error);
-
-if (ThreadSP thread_sp =
-old_thread_list.FindThreadByID(tid, false /*=can_update*/)) {
-  // If the thread was already in the old_thread_list,
-  // just add it back to the new_thread_list.
-  new_thread_list.AddThread(thread_sp);
-  return true;
-}
+  LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
 
-auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
+auto thread_or_error =
+ScriptedThread::Create(*this, object_sp->GetAsGeneric());
 
 if (!thread_or_error)
   return ScriptedInterface::ErrorWithMessage(
@@ -345,8 +365,7 @@
 if (!reg_ctx_sp)
   return ScriptedInterface::ErrorWithMessage(
   LLVM_PRETTY_FUNCTION,
-  llvm::Twine("Invalid Register Context for thread " +
-  llvm::Twine(key.AsCString()))
+  llvm::Twine("Invalid Register Context for thread " + 
llvm::Twine(idx))
   .str(),
   error);
 
@@ -355,7 +374,7 @@
 return true;
   };
 
-  thread_info_sp->ForEach(create_scripted_thread);
+  llvm::for_each(sorted_threads, create_scripted_thread);
 
   return new_thread_list.GetSize(false) > 0;
 }


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@
 
   StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match the
-  // thread indices.
-
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
 "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key el

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

zrthxn wrote:
> jj10306 wrote:
> > nit: should we update this to use the error map? I don't think there's a 
> > significant difference performance wise, but the code would be a little 
> > cleaner imo and consistent with how `GetError()` works.
> That would sort of look like this I think
> 
> ```
>   if 
> (m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA())
> return false;
>   else true;
> ```
What about 
`return (bool)m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);`
Another idea is to just remove the `IsError()` function entirely since calling 
`GetError()` tells you if it's an error. iirc all error checks actually use 
`GetError` except for the checks inside of `TraceHtr` which is soon going to be 
deleted by @wallace in new patches, so you could just change those couple 
instances of `IsError` and remove it all together. Definitely not necessary, 
just spitballing ideas (:
@wallace what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:25
 IntelPTError::IntelPTError(int libipt_error_code, lldb::addr_t address)
 : m_libipt_error_code(libipt_error_code), m_address(address) {
   assert(libipt_error_code < 0);





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:105-107
 DecodedThread::DecodedThread(ThreadSP thread_sp, Error error)
 : m_thread_sp(thread_sp) {
+  m_instructions.emplace_back(error);

let's improve this method a bit



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:86-87
 
-  /// \return
-  /// An \a llvm::Error object if this class corresponds to an Error, or an
-  /// \a llvm::Error::success otherwise.
-  llvm::Error ToError() const;
+  /// Get the size in bytes of an instance of this class
+  static size_t GetMemoryUsage();
 

good



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:155
+  /// Append a successfully decoded instruction.
+  template  void AppendInstruction(Ts... __args) {
+m_instructions.emplace_back(std::move(IntelPTInstruction(__args...)));

just call it args or instruction_args instead of __args



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156
+  template  void AppendInstruction(Ts... __args) {
+m_instructions.emplace_back(std::move(IntelPTInstruction(__args...)));
+  }

the goal is to avoid creating explicitly a  IntelPTInstruction, so you should 
be able to achieve something like
  m_instructions.emplace_back(args...);




Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:205
 
-  int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
+  // ProcessSP process = thread_sp->GetProcess();
+  int errcode = pt_image_set_callback(image, ReadProcessMemory,

delete this



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:122-123
(double)mem_used / 1024);
+  s.Printf("  Average memory usage per instruction: %0.2lf KiB\n",
+   ((double)mem_used/insn_len) / 1024);
   return;

print it in bytes instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

jj10306 wrote:
> zrthxn wrote:
> > jj10306 wrote:
> > > nit: should we update this to use the error map? I don't think there's a 
> > > significant difference performance wise, but the code would be a little 
> > > cleaner imo and consistent with how `GetError()` works.
> > That would sort of look like this I think
> > 
> > ```
> >   if 
> > (m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA())
> > return false;
> >   else true;
> > ```
> What about 
> `return (bool)m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);`
> Another idea is to just remove the `IsError()` function entirely since 
> calling `GetError()` tells you if it's an error. iirc all error checks 
> actually use `GetError` except for the checks inside of `TraceHtr` which is 
> soon going to be deleted by @wallace in new patches, so you could just change 
> those couple instances of `IsError` and remove it all together. Definitely 
> not necessary, just spitballing ideas (:
> @wallace what do you think?
I don't think that's a good idea. The problem is calling 
`GetErrorByInstructionIndex` is that you then have an Error object that you 
need to consume. There's also the cost of creating this object even if you just 
want to know if there's an error or not and you don't want to do anything with 
the actual error message. It's better then to create the Error object only when 
needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [lldb] 3427edd - Adopt new dyld SPIs to introspect the shared cache.

[Jonas Devlieghere via lldb-commits]

Author: Fred Riss
Date: 2022-03-25T18:02:15-07:00
New Revision: 3427eddd9aabcb1505ffe16adfcba7d6e8b28bf8

URL: 
https://github.com/llvm/llvm-project/commit/3427eddd9aabcb1505ffe16adfcba7d6e8b28bf8
DIFF: 
https://github.com/llvm/llvm-project/commit/3427eddd9aabcb1505ffe16adfcba7d6e8b28bf8.diff

LOG: Adopt new dyld SPIs to introspect the shared cache.

With the shared cache getting split into multiple files, the current
way we created ObjectFileMachO objects for shared cache dylib images
will break.

This patch conditionally adopts new SPIs which will do the right
thing in the new world of multi-file caches.

Added: 


Modified: 
lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

Removed: 




diff  --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm 
b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
index 3d68fcdb653b9..e038b7abb6d78 100644
--- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -35,6 +35,10 @@
 #include 
 #include 
 #include 
+#if __has_include()
+#include 
+#define SDK_HAS_NEW_DYLD_INTROSPECTION_SPIS
+#endif
 #include 
 
 // These are needed when compiling on systems
@@ -525,6 +529,41 @@ static void ParseOSVersion(llvm::VersionTuple &version, 
NSString *Key) {
 }
 
 SharedCacheInfo::SharedCacheInfo() {
+#if defined(SDK_HAS_NEW_DYLD_INTROSPECTION_SPIS)
+  if (__builtin_available(macOS 12, *)) {
+if (dyld_process_create_for_current_task) {
+  auto dyld_process = dyld_process_create_for_current_task();
+  auto snapshot =
+  dyld_process_snapshot_create_for_process(dyld_process, nullptr);
+  auto shared_cache = dyld_process_snapshot_get_shared_cache(snapshot);
+  assert(dyld_process && snapshot && shared_cache);
+
+  dyld_shared_cache_for_each_image(shared_cache, ^(dyld_image_t image) {
+__block uint64_t minVmAddr = UINT64_MAX;
+__block uint64_t maxVmAddr = 0;
+uuid_t uuidStore;
+__block uuid_t *uuid = &uuidStore;
+
+dyld_image_for_each_segment_info(image, ^(const char *segmentName,
+  uint64_t vmAddr,
+  uint64_t vmSize, int perm) {
+  minVmAddr = std::min(minVmAddr, vmAddr);
+  maxVmAddr = std::max(maxVmAddr, vmAddr + vmSize);
+  dyld_image_copy_uuid(image, uuid);
+});
+assert(minVmAddr != UINT_MAX);
+assert(maxVmAddr != 0);
+m_images[dyld_image_get_installname(image)] = SharedCacheImageInfo{
+UUID::fromData(uuid, 16),
+std::make_shared((uint8_t *)minVmAddr,
+maxVmAddr - minVmAddr)};
+  });
+  dyld_process_snapshot_dispose(snapshot);
+  return;
+}
+  }
+#endif
+
   size_t shared_cache_size;
   uint8_t *shared_cache_start =
   _dyld_get_shared_cache_range(&shared_cache_size);



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

[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 418361.
wallace marked 2 inline comments as done.
wallace added a comment.

Some updates:

- Modified `thread trace dump instructions` to accept one single thread instead 
of many. The reason is that, with the new --id argument, traversing multiple 
threads doesn't make sense anymore, as the id might only exist in one single 
thread. Besides that, it's really not very useful to analyze multiple threads 
in parallel by chunks. You normally want to analyze one of them at a time.
- I couldn't find a simple way to create the repeat command directly from 
`GetRepeatCommand` because it's not easy to quickly find what the next 
instruction is going to be in the repeat command. In fact, finding the id of 
that future instruction requires to actually traverse the instructions up to 
that point, but the traversal only happens in DoExecute, which is invoked after 
GetRepeatCommand. As a simple workarou nd, I'm adding a " repeat" positional 
argument that won't be parsed by CommandOptions but will still indicate that a 
repeat is happening. I was able to remove the --continue flag from Options.td, 
thus reducing the amount of code needed to handle the repeat. Not only that, I 
was able to get rid of the map that I had in the 
CommandObject that I was using to continue the iteration in the repeat 
commands. Now I'm using the last iterated id to computed where the next one 
will be, thus creating a new brand new dumper with each command making this 
class simpler.
- I can't pass the Stream to TraceInstructionDumperOptions because when that 
object is crea ted the Stream is not yet available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn marked 13 inline comments as done.
zrthxn added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:25
 IntelPTError::IntelPTError(int libipt_error_code, lldb::addr_t address)
 : m_libipt_error_code(libipt_error_code), m_address(address) {
   assert(libipt_error_code < 0);

wallace wrote:
> 
This is needed at DecodedThread.h:66, not here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn marked an inline comment as done.
zrthxn added a comment.

Test program info, 
(llvm-project/lldb/test/API/commands/trace/intelpt-trace/a.out)

  (lldb) thread trace dump info
  Trace technology: intel-pt
  
  thread #1: tid = 3842849
Raw trace size: 4 KiB
Total number of instructions: 22
Total approximate memory usage: 6.38 KiB
Average memory usage per instruction: 296 bytes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

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

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn updated this revision to Diff 418376.
zrthxn added a comment.

Clean up and finalize


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

Files:
  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/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -37,8 +37,9 @@
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
-  Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total number of instructions: 22
+  Total approximate memory usage: 6.38 KiB
+  Average memory usage per instruction: 296 bytes'''])
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInfo.py
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -39,5 +39,6 @@
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
-  Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total number of instructions: 22
+  Total approximate memory usage: 6.38 KiB
+  Average memory usage per instruction: 296 bytes'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -112,12 +112,15 @@
   }
   s.Printf("\n");
 
+  size_t insn_len = Decode(thread)->GetInstructions().size();
   size_t mem_used = Decode(thread)->CalculateApproximateMemoryUsage();
+
   s.Printf("  Raw trace size: %zu KiB\n", *raw_size / 1024);
-  s.Printf("  Total number of instructions: %zu\n",
-   Decode(thread)->GetInstructions().size());
+  s.Printf("  Total number of instructions: %zu\n", insn_len);
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
(double)mem_used / 1024);
+  s.Printf("  Average memory usage per instruction: %zu bytes\n",
+   mem_used / insn_len);
   return;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -78,7 +78,7 @@
 }
 
 Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -104,9 +105,11 @@
 ///
 /// \return
 ///   The decoded instructions.
-static std::vector
-DecodeInstructions(pt_insn_decoder &decoder) {
-  std::vector instructions;
+static DecodedThreadSP DecodeInstructions(pt_insn_decoder &decoder,
+  const ThreadSP &thread_sp,
+  const size_t raw_trace_size) {
+  DecodedThreadSP decoded_thread = std::make_shared(
+  thread_sp, std::vector(), raw_trace_size);
 
   while (true) {
 int errcode = FindNextSynchronizationPoint(decoder);
@@ -114,7 +117,7 @@
   break;
 
 if (errcode < 0) {
-  instructions.emplace_back(make_error(errcode));
+  decoded_thread->AppendError(errcode);
   break;
 }
 
@@ -123,17 +126,17 @@
 while (true) {
   errcode = ProcessPTEvents(decoder, errcode);
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode));
+decoded_thread->AppendError(errcode);
 break;
   }
-  pt_insn insn;
 
+  pt_insn insn;
   errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
   if (errcode == -pte_eos)
 break;
 
   if (errcode < 0) {
-instructions.emplace_back(make_error(errcode, insn.ip));
+decoded_thread->AppendError(errcode, insn.ip);
 break;
   }
 
@@ -142,22 +145,22 @@
   if (time_error == -pte_invalid) {
 // This happens if we invoke the pt_ins

[Lldb-commits] [PATCH] D110172: [lldb/win] Default to native PDB reader when building with LLVM_ENABLE_DIA_SDK=NO

[Mehdi Chinoune]

MehdiChinoune added a comment.
Herald added a project: All.

This commit make building standalone LLDB impossible on Windows. 
`llvm/Config/config.h` file is only generated at build-time and not get 
installed with llvm.
Please Fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110172

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

[Lldb-commits] [PATCH] D122523: [lldb] Fix building standalone LLDB on Windows.

[Mehdi Chinoune]

MehdiChinoune created this revision.
MehdiChinoune added reviewers: LLDB, thakis.
MehdiChinoune added projects: LLDB, LLVM.
Herald added subscribers: JDevlieghere, mgorny.
Herald added a project: All.
MehdiChinoune requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits.

It was broken since https://reviews.llvm.org/D110172


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122523

Files:
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Config/llvm-config.h.cmake


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -116,4 +116,7 @@
  * in non assert builds */
 #cmakedefine01 LLVM_UNREACHABLE_OPTIMIZE
 
+/* Define to 1 if you have the DIA SDK installed, and to 0 if you don't. */
+#cmakedefine01 LLVM_ENABLE_DIA_SDK
+
 #endif
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -50,9 +50,6 @@
don't. */
 #cmakedefine01 HAVE_DECL_STRERROR_S
 
-/* Define to 1 if you have the DIA SDK installed, and to 0 if you don't. */
-#cmakedefine01 LLVM_ENABLE_DIA_SDK
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_DLFCN_H ${HAVE_DLFCN_H}
 
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -57,7 +57,7 @@
 #include "Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h"
 
 #if defined(_WIN32)
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #endif
 
 using namespace lldb;


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -116,4 +116,7 @@
  * in non assert builds */
 #cmakedefine01 LLVM_UNREACHABLE_OPTIMIZE
 
+/* Define to 1 if you have the DIA SDK installed, and to 0 if you don't. */
+#cmakedefine01 LLVM_ENABLE_DIA_SDK
+
 #endif
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -50,9 +50,6 @@
don't. */
 #cmakedefine01 HAVE_DECL_STRERROR_S
 
-/* Define to 1 if you have the DIA SDK installed, and to 0 if you don't. */
-#cmakedefine01 LLVM_ENABLE_DIA_SDK
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_DLFCN_H ${HAVE_DLFCN_H}
 
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -57,7 +57,7 @@
 #include "Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h"
 
 #if defined(_WIN32)
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #endif
 
 using namespace lldb;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits