[Lldb-commits] [PATCH] D128956: make debugserver able to inspect mach-o binaries present in memory, but not yet registered with dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for the review David.  I am still fiddling with handling the case where 
debugserver is handed the address of something that isn't actually a Mach-O 
binary in memory, but I'll update the patch soon with these.




Comment at: lldb/test/API/macosx/unregistered-macho/TestUnregisteredMacho.py:21
+self.build()
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.c"))

DavidSpickett wrote:
> You can drop the `()` around the variable names.
Ah, yes thanks.



Comment at: lldb/test/API/macosx/unregistered-macho/main.c:21
+  mh.ncmds = 2;
+  mh.sizeofcmds = size_of_mh_and_cmds;
+  mh.flags = MH_NOUNDEFS | MH_DYLDLINK | MH_TWOLEVEL | MH_PIE;

DavidSpickett wrote:
> It seems like this should be the size of the commands only, not including the 
> header. Though I can't find documentation for it so maybe this is just an 
> unfortunate name.
No, you're exactly correct.  When I was writing the test case originally I had 
a little bug and switched sizeofcmds to be size of the mach header and the 
commands, and it's not correct.  Most parsers limit their parsing to the number 
of load commands and the total size of load commands, so they wouldn't read 
past the actual load commands.  Thanks for noticing, I fixed it after 
confirming I was wrong, hah.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128956

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

[Lldb-commits] [PATCH] D128932: [lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have a feeling the semaphores will not work (compile) on darwin. I didn't 
find any sem_init call there -- just sem_open. Maybe instead of semaphores we 
could use files for the synchronization? Something similar to the 
`wait_for_file_on_target` function, just in the other direction. Then the test 
could create a file to move the processes forward? (This might be easier to 
achieve with a dedicated inferior, instead of trying to fit it into the 
universal LLGS inferior.)




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1110
 // does not interfere with our protocol.
-StopSTDIOForwarding();
+if (!m_non_stop)
+  StopSTDIOForwarding();

In the multiprocess all-stop mode, if one process stops, we are supposed to 
stop all of them, right? I take it that's not something we do right now, is it?



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py:116
+parent_pid, parent_tid, child_pid, child_tid = (
+self.start_fork_test(["fork", "sleep:2", "print-pid", "sleep:2",
+  "stop"],

mgorny wrote:
> mgorny wrote:
> > I really dislike these sleeps but I can't think of a better way of doing it 
> > (short of using IPC for synchronization). The goal is to 1) ensure that 
> > both processes start before they start outputting, and 2) ensure that both 
> > output before the first stop reason comes.
> Ok, semaphores are not scary after all, and I suppose we can expect them to 
> work if we expect `fork()` to work.
> ensure that both processes start before they start outputting
But we should be able to see the output from the first process (if it had any) 
even it is the only process running. Do we have a test for that? Could you add 
a step where the first process outputs something before it waits to synchronize 
with the second process?


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

https://reviews.llvm.org/D128932

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

[Lldb-commits] [PATCH] D129177: [lldb][AArch64] Use "+all" feature for the disassembler

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The "+all" feature name was added in https://reviews.llvm.org/D128029.

This feature means we don't have to generate a list of features
or use a base architecture feature.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129177

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s


Index: lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
===
--- lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
+++ lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
@@ -3,11 +3,7 @@
 # This checks that lldb's disassembler enables every extension that an AArch64
 # target could have.
 
-# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t \
-# RUN: --mattr=+tme,+mte,+crc,+lse,+rdm,+sm4,+sha3,+aes,+dotprod,+fullfp16 \
-# RUN: 
--mattr=+fp16fml,+sve,+sve2,+sve2-aes,+sve2-sm4,+sve2-sha3,+sve2-bitperm \
-# RUN: --mattr=+spe,+rcpc,+ssbs,+sb,+predres,+bf16,+mops,+hbc,+sme,+sme-i64 \
-# RUN: --mattr=+sme-f64,+flagm,+pauth,+brbe,+ls64,+f64mm,+f32mm,+i8mm,+rand
+# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t 
--mattr=+all
 # RUN: %lldb %t -o "disassemble -n fn" -o exit 2>&1 | FileCheck %s
 
 .globl  fn
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1181,11 +1181,7 @@
 
   // If any AArch64 variant, enable latest ISA with all extensions.
   if (triple.isAArch64()) {
-features_str += "+v9.3a,";
-std::vector features;
-// Get all possible features
-llvm::AArch64::getExtensionFeatures(-1, features);
-features_str += llvm::join(features, ",");
+features_str += "+all,";
 
 if (triple.getVendor() == llvm::Triple::Apple)
   cpu = "apple-latest";


Index: lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
===
--- lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
+++ lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
@@ -3,11 +3,7 @@
 # This checks that lldb's disassembler enables every extension that an AArch64
 # target could have.
 
-# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t \
-# RUN: --mattr=+tme,+mte,+crc,+lse,+rdm,+sm4,+sha3,+aes,+dotprod,+fullfp16 \
-# RUN: --mattr=+fp16fml,+sve,+sve2,+sve2-aes,+sve2-sm4,+sve2-sha3,+sve2-bitperm \
-# RUN: --mattr=+spe,+rcpc,+ssbs,+sb,+predres,+bf16,+mops,+hbc,+sme,+sme-i64 \
-# RUN: --mattr=+sme-f64,+flagm,+pauth,+brbe,+ls64,+f64mm,+f32mm,+i8mm,+rand
+# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t --mattr=+all
 # RUN: %lldb %t -o "disassemble -n fn" -o exit 2>&1 | FileCheck %s
 
 .globl  fn
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1181,11 +1181,7 @@
 
   // If any AArch64 variant, enable latest ISA with all extensions.
   if (triple.isAArch64()) {
-features_str += "+v9.3a,";
-std::vector features;
-// Get all possible features
-llvm::AArch64::getExtensionFeatures(-1, features);
-features_str += llvm::join(features, ",");
+features_str += "+all,";
 
 if (triple.getVendor() == llvm::Triple::Apple)
   cpu = "apple-latest";
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 4270c9c - [lldb] Stop passing both i386 and i686 in parallel as architectures on Windows

[Martin Storsjö via lldb-commits]

Author: Martin Storsjö
Date: 2022-07-06T12:13:36+03:00
New Revision: 4270c9cd44f2703bc5376ff085d0add156af9080

URL: 
https://github.com/llvm/llvm-project/commit/4270c9cd44f2703bc5376ff085d0add156af9080
DIFF: 
https://github.com/llvm/llvm-project/commit/4270c9cd44f2703bc5376ff085d0add156af9080.diff

LOG: [lldb] Stop passing both i386 and i686 in parallel as architectures on 
Windows

When an object file returns multiple architectures, it is treated
as a fat binary - which really isn't the case of i386 vs i686 where
the object file actually has one architecture.

This allows getting rid of hardcoded architecture triples in
PlatformWindows.

The parallel i386 and i686 architecture strings stem from
5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 and
ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658.

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

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp 
b/lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
index 1c10efed95640..44c708676e529 100644
--- a/lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ b/lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@ size_t ObjectFilePDB::GetModuleSpecifications(
   case PDB_Machine::x86:
 module_arch.SetTriple("i386-pc-windows");
 specs.Append(module_spec);
-module_arch.SetTriple("i686-pc-windows");
-specs.Append(module_spec);
 break;
   case PDB_Machine::ArmNT:
 module_arch.SetTriple("armv7-pc-windows");

diff  --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp 
b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
index fd34ac65970ba..45593e95863a2 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@ size_t ObjectFilePECOFF::GetModuleSpecifications(
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");

diff  --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp 
b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index 38f387dfdb29d..e5e235881e58a 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@ PlatformWindows::PlatformWindows(bool is_host) : 
RemoteAwarePlatform(is_host) {
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args &args) {

diff  --git a/lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml 
b/lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
index ca2bac38027fa..561210455010b 100644
--- a/lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ b/lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:



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

[Lldb-commits] [PATCH] D128617: [lldb] Stop passing both i386 and i686 in parallel as architectures on Windows

[Martin Storsjö via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4270c9cd44f2: [lldb] Stop passing both i386 and i686 in 
parallel as architectures on Windows (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128617

Files:
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args &args) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@
   case PDB_Machine::x86:
 module_arch.SetTriple("i386-pc-windows");
 specs.Append(module_spec);
-module_arch.SetTriple("i686-pc-windows");
-specs.Append(module_spec);
 break;
   case PDB_Machine::ArmNT:
 module_arch.SetTriple("armv7-pc-windows");


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args &args) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@
   case PDB_Machine::x86:
 m

[Lldb-commits] [PATCH] D129012: [lldb] [test] Improve stability of llgs vCont-threads tests

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Seems like an improvement. I'd like to hear what you make of the inline comment 
though.

I can also imagine a setup where most of the verification happens inside the 
inferior. Like, each time a thread gets to run it increments a variable 
specific to that thread. Once that number crosses some threshold, it check the 
variables of the other threads, and verifies that one of them has the number 
zero, and the number of the other thread is reasonably high (> 10% of the 
threshold or something).




Comment at: lldb/test/API/tools/lldb-server/vCont-threads/main.cpp:32
+  get_thread_id());
+write(STDOUT_FILENO, buf, strlen(buf));
+

I am not sure if `write` is an (atomic) system call on windows. Maybe just put 
a mutex around the printf call? In this setup, I think it would be sufficient 
to write this once, and then spend the rest of the time making sure the other 
suspended thread gets a chance to run (if it is not suspended for whatever 
reason).


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

https://reviews.llvm.org/D129012

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

[Lldb-commits] [PATCH] D129177: [lldb][AArch64] Use "+all" feature for the disassembler

[Pavel Labath via Phabricator via lldb-commits]

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

Awesome. thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129177

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

[Lldb-commits] [PATCH] D128932: [lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D128932#3631903 , @labath wrote:

> I have a feeling the semaphores will not work (compile) on darwin. I didn't 
> find any sem_init call there -- just sem_open. Maybe instead of semaphores we 
> could use files for the synchronization? Something similar to the 
> `wait_for_file_on_target` function, just in the other direction. Then the 
> test could create a file to move the processes forward? (This might be easier 
> to achieve with a dedicated inferior, instead of trying to fit it into the 
> universal LLGS inferior.)

Damn, and I thought POSIX actually means something to Darwin. Yeah, I'll look 
into using some other synchronization mechanism.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1110
 // does not interfere with our protocol.
-StopSTDIOForwarding();
+if (!m_non_stop)
+  StopSTDIOForwarding();

labath wrote:
> In the multiprocess all-stop mode, if one process stops, we are supposed to 
> stop all of them, right? I take it that's not something we do right now, is 
> it?
We don't support resuming multiple processes in all-stop mode. `vCont` returns 
an error if you try to do that, and other continue packets are blocking, so it 
can't happen ;-).



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py:116
+parent_pid, parent_tid, child_pid, child_tid = (
+self.start_fork_test(["fork", "sleep:2", "print-pid", "sleep:2",
+  "stop"],

labath wrote:
> mgorny wrote:
> > mgorny wrote:
> > > I really dislike these sleeps but I can't think of a better way of doing 
> > > it (short of using IPC for synchronization). The goal is to 1) ensure 
> > > that both processes start before they start outputting, and 2) ensure 
> > > that both output before the first stop reason comes.
> > Ok, semaphores are not scary after all, and I suppose we can expect them to 
> > work if we expect `fork()` to work.
> > ensure that both processes start before they start outputting
> But we should be able to see the output from the first process (if it had 
> any) even it is the only process running. Do we have a test for that? Could 
> you add a step where the first process outputs something before it waits to 
> synchronize with the second process?
Yes, I suppose covering that with a test also makes sense.


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

https://reviews.llvm.org/D128932

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

[Lldb-commits] [PATCH] D81471: [lldb] Add support for using integral const static data members in the expression evaluator

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

My only complaint is about the restriction in CanTakeAddressOfLValue. It seems 
arbitrary. There's anything in C++ or DWARF that would prevent us from having 
constant-valued variables of class types. It's just that clang/llvm does not 
know how to emit the DW_AT_const_value for such variables (yet). Gcc does not 
seem to have that problem (it happily emits a block form for those). So, it 
feels to me this is just kicking the can down the road, and we will sooner or 
later need to come up with a better story for this.

I guess the condition we really want to express here is "does this expression 
refer to a constexpr variable (ideally one without a location)"? And the 
problem is that clang does not give us the means to detect that?

Is that really the case? Would it maybe be possible to use some c++ magic to 
get clang to do that for us. Write something like `if constexpr 
(__builtin_constant_p(user_expression)) do_something_rvalue_like(); else 
assume_regular_lvalue();` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81471

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

[Lldb-commits] [PATCH] D81471: [lldb] Add support for using integral const static data members in the expression evaluator

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:249
   //
   // For Lvalues
   //

Minor: Should we update this documentation?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2689
+  // TODO: Support float/double static members as well.
+  if (!attrs.const_value_form && !ct.IsIntegerOrEnumerationType(unused))
+return;

Should this be:

```
if (!attrs.const_value_form || !ct.IsIntegerOrEnumerationType(unused))
```
?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2696
+   "Failed to add const value to variable {1}: {0}",
+   v->getQualifiedNameAsString());
+return;

Can `v` be `nullptr` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81471

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

[Lldb-commits] [PATCH] D126614: [lldb] [gdb-remote] Client support for using the non-stop protocol

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 442519.
mgorny marked 2 inline comments as done.
mgorny added a comment.

Partial update. Implemented most of the requests, except what noted below. Also 
implemented support for `%Stdio:` stdout forwarding style as was eventually 
implemented in LLGS.

I haven't been able to figure out how to make the test suite work without the 
public-private state change.

As for the `OK` response to `vCtrlC`… it's hard because the incoming packet 
causes the pending read in 
`GDBRemoteClientBase::SendContinuePacketAndWaitForResponse()` to succeed, so 
the `OK` is swallowed there. I can't think of any way to do this that wouldn't 
be super ugly.


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

https://reviews.llvm.org/D126614

Files:
  lldb/packages/Python/lldbsuite/test/gdbclientutils.py
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
  lldb/source/Target/Process.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestNonStop.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestNonStop.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestNonStop.py
@@ -0,0 +1,157 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+
+class TestNonStop(GDBRemoteTestBase):
+def test_run(self):
+class MyResponder(MockGDBServerResponder):
+vStopped_counter = 0
+
+def qSupported(self, client_supported):
+return "QNonStop+;" + super().qSupported(client_supported)
+
+def QNonStop(self, val):
+return "OK"
+
+def qfThreadInfo(self):
+return "m10,12"
+
+def qsThreadInfo(self):
+return "l"
+
+def vStopped(self):
+self.vStopped_counter += 1
+return ("OK" if self.vStopped_counter > 1
+else "T00;thread:10")
+
+def cont(self):
+self.vStopped_counter = 0
+return ["OK", "%Stop:T02;thread:12"]
+
+def vCtrlC(self):
+return "OK"
+
+self.dbg.HandleCommand(
+"settings set plugin.process.gdb-remote.use-non-stop-protocol true")
+self.addTearDownHook(lambda:
+self.runCmd(
+"settings set plugin.process.gdb-remote.use-non-stop-protocol "
+"false"))
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget("")
+process = self.connect(target)
+self.assertPacketLogContains(["QNonStop:1"])
+
+process.Continue()
+self.assertPacketLogContains(["vStopped"])
+self.assertEqual(process.GetSelectedThread().GetStopReason(),
+ lldb.eStopReasonSignal)
+self.assertEqual(process.GetSelectedThread().GetStopDescription(100),
+ "signal SIGINT")
+
+def test_stdio(self):
+class MyResponder(MockGDBServerResponder):
+vStdio_counter = 0
+vStopped_counter = 0
+
+def qSupported(self, client_supported):
+return "QNonStop+;" + super().qSupported(client_supported)
+
+def QNonStop(self, val):
+return "OK"
+
+def qfThreadInfo(self):
+return "m10,12"
+
+def qsThreadInfo(self):
+return "l"
+
+def vStdio(self):
+self.vStdio_counter += 1
+# intersperse notifications with replies for better testing
+return ("OK" if self.vStdio_counter > 1
+else ["%Stop:T02;thread:12",
+  "O7365636f6e64206c696e650d0a"])
+
+def vStopped(self):
+self.vStopped_counter += 1
+return ("OK" if self.vStopped_counter > 1
+else "T00;thread:10")
+
+def cont(self):
+self.vStopped_counter = 0
+self.vStdio_counter = 0
+return ["OK",
+"%Stdio:O6669727374206c696e650d0a",]
+
+def vCtrlC(self):
+return "OK"
+
+self.dbg.HandleCommand(
+"settings set plugin.process.gdb-remot

[Lldb-commits] [PATCH] D129078: WIP: [LLDB][ClangExpression] Allow expression evaluation from within C++ Lambdas

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 442525.
Michael137 added a comment.

- [LLDB][NFC] Create variable for hardcoded alignment/size constants in 
materializer
- [LLDB][Expression] Allow instantiation of IR Entity from ValueObject
- Removed redundant m_object_pointer_type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129078

Files:
  lldb/include/lldb/Expression/Materializer.h
  lldb/include/lldb/Expression/UserExpression.h
  lldb/source/Expression/Materializer.cpp
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionVariable.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
  lldb/test/API/commands/expression/expr_inside_lambda/Makefile
  lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
  lldb/test/API/commands/expression/expr_inside_lambda/main.cpp

Index: lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
@@ -0,0 +1,99 @@
+#include 
+#include 
+
+namespace {
+int global_var = -5;
+} // namespace
+
+struct Baz {
+  virtual ~Baz() = default;
+
+  virtual int baz_virt() = 0;
+
+  int base_base_var = 12;
+};
+
+struct Bar : public Baz {
+  virtual ~Bar() = default;
+
+  virtual int baz_virt() override {
+base_var = 10;
+return 1;
+  }
+
+  int base_var = 15;
+};
+
+struct Foo : public Bar {
+  int class_var = 9;
+  int shadowed = -137;
+  int *class_ptr;
+
+  virtual ~Foo() = default;
+
+  virtual int baz_virt() override {
+shadowed = -1;
+return 2;
+  }
+
+  void method() {
+int local_var = 137;
+int shadowed;
+class_ptr = &local_var;
+auto lambda = [&shadowed, this, &local_var,
+   local_var_copy = local_var]() mutable {
+  int lambda_local_var = 5;
+  shadowed = 5;
+  class_var = 109;
+  --base_var;
+  --base_base_var;
+  std::puts("break here");
+
+  auto nested_lambda = [this, &lambda_local_var] {
+std::puts("break here");
+lambda_local_var = 0;
+  };
+
+  nested_lambda();
+  --local_var_copy;
+  std::puts("break here");
+
+  struct LocalLambdaClass {
+int lambda_class_local = -12345;
+Foo *outer_ptr;
+
+void inner_method() {
+  auto lambda = [this] {
+std::puts("break here");
+lambda_class_local = -2;
+outer_ptr->class_var *= 2;
+  };
+
+  lambda();
+}
+  };
+
+  LocalLambdaClass l;
+  l.outer_ptr = this;
+  l.inner_method();
+};
+lambda();
+  }
+
+  void non_capturing_method() {
+int local = 5;
+int local2 = 10;
+
+class_var += [=] {
+  std::puts("break here");
+  return local + local2;
+}();
+  }
+};
+
+int main() {
+  Foo f;
+  f.method();
+  f.non_capturing_method();
+  return global_var;
+}
Index: lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
@@ -0,0 +1,123 @@
+""" Test that evaluating expressions from within C++ lambdas works
+Particularly, we test the case of capturing "this" and
+using members of the captured object in expression evaluation
+while we're on a breakpoint inside a lambda.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+
+
+class ExprInsideLambdaTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def expectExprError(self, expr : str, expected : str):
+frame = self.thread.GetFrameAtIndex(0)
+value = frame.EvaluateExpression(expr)
+errmsg = value.GetError().GetCString()
+self.assertIn(expected, errmsg)
+
+def test_expr_inside_lambda(self):
+"""Test that lldb evaluating expressions inside lambda expressions works correctly."""
+self.build()
+(target, process, self.thread, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))
+
+# Inside 'Foo::method'
+
+# Check access to captured 'this'
+self.expect_expr("class_var", result_type="int", result_value="109")
+self.expect_expr("this->class_var", result_type="int", result_value="109")
+
+# Check that captured shadowed variables take preference over the
+# corresponding member variable
+s

[Lldb-commits] [PATCH] D129177: [lldb][AArch64] Use "+all" feature for the disassembler

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe5fdcfac1bbe: [lldb][AArch64] Use "+all" feature 
for the disassembler (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129177

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s


Index: lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
===
--- lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
+++ lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
@@ -3,11 +3,7 @@
 # This checks that lldb's disassembler enables every extension that an AArch64
 # target could have.
 
-# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t \
-# RUN: --mattr=+tme,+mte,+crc,+lse,+rdm,+sm4,+sha3,+aes,+dotprod,+fullfp16 \
-# RUN: 
--mattr=+fp16fml,+sve,+sve2,+sve2-aes,+sve2-sm4,+sve2-sha3,+sve2-bitperm \
-# RUN: --mattr=+spe,+rcpc,+ssbs,+sb,+predres,+bf16,+mops,+hbc,+sme,+sme-i64 \
-# RUN: --mattr=+sme-f64,+flagm,+pauth,+brbe,+ls64,+f64mm,+f32mm,+i8mm,+rand
+# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t 
--mattr=+all
 # RUN: %lldb %t -o "disassemble -n fn" -o exit 2>&1 | FileCheck %s
 
 .globl  fn
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1181,11 +1181,7 @@
 
   // If any AArch64 variant, enable latest ISA with all extensions.
   if (triple.isAArch64()) {
-features_str += "+v9.3a,";
-std::vector features;
-// Get all possible features
-llvm::AArch64::getExtensionFeatures(-1, features);
-features_str += llvm::join(features, ",");
+features_str += "+all,";
 
 if (triple.getVendor() == llvm::Triple::Apple)
   cpu = "apple-latest";


Index: lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
===
--- lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
+++ lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
@@ -3,11 +3,7 @@
 # This checks that lldb's disassembler enables every extension that an AArch64
 # target could have.
 
-# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t \
-# RUN: --mattr=+tme,+mte,+crc,+lse,+rdm,+sm4,+sha3,+aes,+dotprod,+fullfp16 \
-# RUN: --mattr=+fp16fml,+sve,+sve2,+sve2-aes,+sve2-sm4,+sve2-sha3,+sve2-bitperm \
-# RUN: --mattr=+spe,+rcpc,+ssbs,+sb,+predres,+bf16,+mops,+hbc,+sme,+sme-i64 \
-# RUN: --mattr=+sme-f64,+flagm,+pauth,+brbe,+ls64,+f64mm,+f32mm,+i8mm,+rand
+# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t --mattr=+all
 # RUN: %lldb %t -o "disassemble -n fn" -o exit 2>&1 | FileCheck %s
 
 .globl  fn
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1181,11 +1181,7 @@
 
   // If any AArch64 variant, enable latest ISA with all extensions.
   if (triple.isAArch64()) {
-features_str += "+v9.3a,";
-std::vector features;
-// Get all possible features
-llvm::AArch64::getExtensionFeatures(-1, features);
-features_str += llvm::join(features, ",");
+features_str += "+all,";
 
 if (triple.getVendor() == llvm::Triple::Apple)
   cpu = "apple-latest";
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] e5fdcfa - [lldb][AArch64] Use "+all" feature for the disassembler

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-07-06T12:15:01Z
New Revision: e5fdcfac1bbea28c4f26721e8ff7ebddde0f9f2d

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

LOG: [lldb][AArch64] Use "+all" feature for the disassembler

The "+all" feature name was added in https://reviews.llvm.org/D128029.

This feature means we don't have to generate a list of features
or use a base architecture feature.

Reviewed By: labath

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

Added: 


Modified: 
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s

Removed: 




diff  --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp 
b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index c85c66442510c..a774d5b61cfe7 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1181,11 +1181,7 @@ DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec 
&arch,
 
   // If any AArch64 variant, enable latest ISA with all extensions.
   if (triple.isAArch64()) {
-features_str += "+v9.3a,";
-std::vector features;
-// Get all possible features
-llvm::AArch64::getExtensionFeatures(-1, features);
-features_str += llvm::join(features, ",");
+features_str += "+all,";
 
 if (triple.getVendor() == llvm::Triple::Apple)
   cpu = "apple-latest";

diff  --git a/lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s 
b/lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
index ede6fdb3471ba..07fad4d487e19 100644
--- a/lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
+++ b/lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
@@ -3,11 +3,7 @@
 # This checks that lldb's disassembler enables every extension that an AArch64
 # target could have.
 
-# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t \
-# RUN: --mattr=+tme,+mte,+crc,+lse,+rdm,+sm4,+sha3,+aes,+dotprod,+fullfp16 \
-# RUN: 
--mattr=+fp16fml,+sve,+sve2,+sve2-aes,+sve2-sm4,+sve2-sha3,+sve2-bitperm \
-# RUN: --mattr=+spe,+rcpc,+ssbs,+sb,+predres,+bf16,+mops,+hbc,+sme,+sme-i64 \
-# RUN: --mattr=+sme-f64,+flagm,+pauth,+brbe,+ls64,+f64mm,+f32mm,+i8mm,+rand
+# RUN: llvm-mc -filetype=obj -triple aarch64-linux-gnueabihf %s -o %t 
--mattr=+all
 # RUN: %lldb %t -o "disassemble -n fn" -o exit 2>&1 | FileCheck %s
 
 .globl  fn



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

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm afraid this will not work on systems which do not default to libc++ (which 
includes at least linux and windows), because `-stdlib=libc++` is not 
"equivalent" to `-nostdlib++ -lc++` -- the former changes the include paths to 
use libc++, while the latter doesn't. And, for most of the things that we do 
here, headers are more important than the library itself.

If the goal here is to preclude any possibility of picking up a different 
library (it's not clear to me what kind of situation you're referring to in the 
description), then the most principled approach would be to also use 
`-nostdinc++` and manually add the appropriate include paths. One nice property 
of that is that it would work with compilers (gcc) which do not understand the 
`-stdlib` flag.

The simple solution would be to use the `-stdlib=libc++ -nostdlib++` as a 
combo, but I am not sure if that would solve what it is supposed to solve (as 
it still relies on clang to autodetect the header path).

Another thing to consider is that historically (back when we were running the 
tests against the system compiler by default), the `USE_LIBCPP` flag meant "use 
the system libc++" (or rather, the default library of the compiler in use). Now 
we use the in-tree compiler (by default), but our libc++ story is not so clear, 
as it mostly relies on clang. If the build contains libc++ then we will use 
that (and sometimes fail due to incorrect usage). If it doesn't, but there's a 
libc++ on the system somewhere, then we will use that one (and be 
non-hermetic). Otherwise, we will skip tests which depend on this (and the 
skipping code uses its own logic, which is not touched here).

Overall, I think it makes sense to use libc++ when we have it available. I am 
not so sure about the other modes. While I think it would be nice to support 
running the tests against a random c++ library (similar to how we do it for 
compilers), I don't think we need to support it unless there someone actively 
interested in running it. I do think we should support a mode where we don't 
have a libc++ in the build tree, but I'd be fine if that just causes all of the 
relevant tests to be skipped.

The thing where that gets interesting is with remote tests. Unlike system 
libc++, I think we have people (including myself, to some extent) whose job is 
to make sure those work. Right now, we don't make any special effort to copy 
the c++ library to the remote system. If it works, it works because the system 
already has a compatible c++ library lying around. The reason these two are 
related is that the system libc++ is more likely to be present on a remote 
system than the one we just built. Messing with the way use libc++ may break 
some of the remote use cases, so it might be good to take some position on 
that. Do we not support that? Do we expect the user to make sure the libc++ is 
on the system already? Do we expect the user to link libc++ statically (I think 
that's what happens in all of our use cases)?

I realize this isn't very helpful, but I'm trying to explain why I fear this 
may be more complicated than it may seem at first. Maybe it is not necessary to 
answer all of these questions for the problem at hand, but these are the 
questions that have stopped me from doing anything in this area in the past.


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

https://reviews.llvm.org/D129166

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

[Lldb-commits] [PATCH] D129012: [lldb] [test] Improve stability of llgs vCont-threads tests

[Michał Górny via Phabricator via lldb-commits]

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

In D129012#3631985 , @labath wrote:

> I can also imagine a setup where most of the verification happens inside the 
> inferior. Like, each time a thread gets to run it increments a variable 
> specific to that thread. Once that number crosses some threshold, it check 
> the variables of the other threads, and verifies that one of them has the 
> number zero, and the number of the other thread is reasonably high (> 10% of 
> the threshold or something).

I suppose this would be possible but tbh I think it'd be more complex than the 
current solution.




Comment at: lldb/test/API/tools/lldb-server/vCont-threads/main.cpp:32
+  get_thread_id());
+write(STDOUT_FILENO, buf, strlen(buf));
+

labath wrote:
> I am not sure if `write` is an (atomic) system call on windows. Maybe just 
> put a mutex around the printf call? In this setup, I think it would be 
> sufficient to write this once, and then spend the rest of the time making 
> sure the other suspended thread gets a chance to run (if it is not suspended 
> for whatever reason).
For some reason, I've assumed we're avoiding mutexes. However, this certainly 
makes sense, as well as removing duplicate writes. I've also noticed we had a 
race condition between starting prints and SIGSTOP, so I've fixed that too. Now 
the main thread is always resumed.


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

https://reviews.llvm.org/D129012

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

[Lldb-commits] [PATCH] D129012: [lldb] [test] Improve stability of llgs vCont-threads tests

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 442550.
mgorny marked an inline comment as done.
mgorny edited the summary of this revision.
mgorny added a comment.

Implement the discussed changes.


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

https://reviews.llvm.org/D129012

Files:
  lldb/test/API/tools/lldb-server/vCont-threads/TestGdbRemote_vContThreads.py
  lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py
  lldb/test/API/tools/lldb-server/vCont-threads/TestSignal.py
  lldb/test/API/tools/lldb-server/vCont-threads/main.cpp

Index: lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
===
--- lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
+++ lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
@@ -6,26 +6,39 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 
 pseudo_barrier_t barrier;
+std::mutex print_mutex;
+bool can_exit_now = false;
 
 static void sigusr1_handler(int signo) {
-  char buf[100];
-  std::snprintf(buf, sizeof(buf),
-"received SIGUSR1 on thread id: %" PRIx64 "\n",
-get_thread_id());
-  write(STDOUT_FILENO, buf, strlen(buf));
+  std::lock_guard lock{print_mutex};
+  std::printf("received SIGUSR1 on thread id: %" PRIx64 "\n", get_thread_id());
+  can_exit_now = true;
 }
 
 static void thread_func() {
   pseudo_barrier_wait(barrier);
-  for (int i = 0; i < 300; ++i) {
+
+  {
+std::lock_guard lock{print_mutex};
 std::printf("thread %" PRIx64 " running\n", get_thread_id());
+  }
+
+  // give other threads a fair chance to run
+  for (int i = 0; i < 5; ++i) {
+std::this_thread::yield();
 std::this_thread::sleep_for(std::chrono::milliseconds(200));
+if (can_exit_now)
+  return;
   }
+
+  // if we didn't get signaled, terminate the program explicitly.
+  _exit(0);
 }
 
 int main(int argc, char **argv) {
@@ -36,12 +49,12 @@
   signal(SIGUSR1, sigusr1_handler);
 
   std::vector threads;
-  for(int i = 0; i < num; ++i)
+  for (int i = 0; i < num; ++i)
 threads.emplace_back(thread_func);
 
-  pseudo_barrier_wait(barrier);
+  std::raise(SIGSTOP);
 
-  std::puts("@started");
+  pseudo_barrier_wait(barrier);
 
   for (std::thread &thread : threads)
 thread.join();
Index: lldb/test/API/tools/lldb-server/vCont-threads/TestSignal.py
===
--- lldb/test/API/tools/lldb-server/vCont-threads/TestSignal.py
+++ lldb/test/API/tools/lldb-server/vCont-threads/TestSignal.py
@@ -1,23 +1,18 @@
-import json
 import re
-import time
 
 import gdbremote_testcase
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
-class TestGdbRemote_vContThreads(gdbremote_testcase.GdbRemoteTestCaseBase):
 
+class TestSignal(gdbremote_testcase.GdbRemoteTestCaseBase):
 def start_threads(self, num):
 procs = self.prep_debug_monitor_and_inferior(inferior_args=[str(num)])
-# start the process and wait for output
 self.test_sequence.add_log_lines([
 "read packet: $c#63",
-{"type": "output_match", "regex": r".*@started\r\n.*"},
+{"direction": "send", "regex": "[$]T.*;reason:signal.*"},
 ], True)
-# then interrupt it
-self.add_interrupt_packets()
 self.add_threadinfo_collection_packets()
 
 context = self.expect_gdbremote_sequence()
@@ -29,22 +24,21 @@
 self.reset_test_sequence()
 return threads
 
+SIGNAL_MATCH_RE = re.compile(r"received SIGUSR1 on thread id: ([0-9a-f]+)")
+
 def send_and_check_signal(self, vCont_data, threads):
 self.test_sequence.add_log_lines([
 "read packet: $vCont;{0}#00".format(vCont_data),
-{"type": "output_match",
- "regex": len(threads) *
-  r".*received SIGUSR1 on thread id: ([0-9a-f]+)\r\n.*",
- "capture": dict((i, "tid{0}".format(i)) for i
- in range(1, len(threads)+1)),
- },
+"send packet: $W00#00",
 ], True)
-
-context = self.expect_gdbremote_sequence()
-self.assertIsNotNone(context)
-tids = sorted(int(context["tid{0}".format(x)], 16)
-  for x in range(1, len(threads)+1))
-self.assertEqual(tids, sorted(threads))
+exp = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+tids = []
+for line in exp["O_content"].decode().splitlines():
+m = self.SIGNAL_MATCH_RE.match(line)
+if m is not None:
+tids.append(int(m.group(1), 16))
+self.assertEqual(sorted(tids), sorted(threads))
 
 def get_pid(self):
 self.add_process_info_collection_packets()
@@ -242,72 +236,3 @@
 
 context = self.expect_gdbremote_sequence()
 self.assertIsNotNone(context)
-
-THREAD_MATCH_RE = re.compil

[Lldb-commits] [PATCH] D129012: [lldb] [test] Improve stability of llgs vCont-threads tests

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/test/API/tools/lldb-server/vCont-threads/main.cpp:32
+  get_thread_id());
+write(STDOUT_FILENO, buf, strlen(buf));
+

mgorny wrote:
> labath wrote:
> > I am not sure if `write` is an (atomic) system call on windows. Maybe just 
> > put a mutex around the printf call? In this setup, I think it would be 
> > sufficient to write this once, and then spend the rest of the time making 
> > sure the other suspended thread gets a chance to run (if it is not 
> > suspended for whatever reason).
> For some reason, I've assumed we're avoiding mutexes. However, this certainly 
> makes sense, as well as removing duplicate writes. I've also noticed we had a 
> race condition between starting prints and SIGSTOP, so I've fixed that too. 
> Now the main thread is always resumed.
We have tests like that, but I think they're all related to the situations 
where we want to ensure some things happen as concurrently as possible. This is 
not one of those cases, and I don't see any problem with using mutexes here 
(and it definitely won't be the first such test).


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

https://reviews.llvm.org/D129012

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

[Lldb-commits] [PATCH] D128932: [lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py:116
+parent_pid, parent_tid, child_pid, child_tid = (
+self.start_fork_test(["fork", "sleep:2", "print-pid", "sleep:2",
+  "stop"],

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > mgorny wrote:
> > > > I really dislike these sleeps but I can't think of a better way of 
> > > > doing it (short of using IPC for synchronization). The goal is to 1) 
> > > > ensure that both processes start before they start outputting, and 2) 
> > > > ensure that both output before the first stop reason comes.
> > > Ok, semaphores are not scary after all, and I suppose we can expect them 
> > > to work if we expect `fork()` to work.
> > > ensure that both processes start before they start outputting
> > But we should be able to see the output from the first process (if it had 
> > any) even it is the only process running. Do we have a test for that? Could 
> > you add a step where the first process outputs something before it waits to 
> > synchronize with the second process?
> Yes, I suppose covering that with a test also makes sense.
Actually, this is already covered by `TestNonStop.py::test_stdio`.


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

https://reviews.llvm.org/D128932

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

[Lldb-commits] [PATCH] D128932: [lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1110
 // does not interfere with our protocol.
-StopSTDIOForwarding();
+if (!m_non_stop)
+  StopSTDIOForwarding();

mgorny wrote:
> labath wrote:
> > In the multiprocess all-stop mode, if one process stops, we are supposed to 
> > stop all of them, right? I take it that's not something we do right now, is 
> > it?
> We don't support resuming multiple processes in all-stop mode. `vCont` 
> returns an error if you try to do that, and other continue packets are 
> blocking, so it can't happen ;-).
ok, got it.


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

https://reviews.llvm.org/D128932

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

[Lldb-commits] [PATCH] D129078: WIP: [LLDB][ClangExpression] Allow expression evaluation from within C++ Lambdas

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 442561.
Michael137 added a comment.

- Add `AddOneVariable` overload that takes `ValueObjectSP`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129078

Files:
  lldb/include/lldb/Expression/Materializer.h
  lldb/include/lldb/Expression/UserExpression.h
  lldb/source/Expression/Materializer.cpp
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionVariable.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
  lldb/test/API/commands/expression/expr_inside_lambda/Makefile
  lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
  lldb/test/API/commands/expression/expr_inside_lambda/main.cpp

Index: lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
@@ -0,0 +1,99 @@
+#include 
+#include 
+
+namespace {
+int global_var = -5;
+} // namespace
+
+struct Baz {
+  virtual ~Baz() = default;
+
+  virtual int baz_virt() = 0;
+
+  int base_base_var = 12;
+};
+
+struct Bar : public Baz {
+  virtual ~Bar() = default;
+
+  virtual int baz_virt() override {
+base_var = 10;
+return 1;
+  }
+
+  int base_var = 15;
+};
+
+struct Foo : public Bar {
+  int class_var = 9;
+  int shadowed = -137;
+  int *class_ptr;
+
+  virtual ~Foo() = default;
+
+  virtual int baz_virt() override {
+shadowed = -1;
+return 2;
+  }
+
+  void method() {
+int local_var = 137;
+int shadowed;
+class_ptr = &local_var;
+auto lambda = [&shadowed, this, &local_var,
+   local_var_copy = local_var]() mutable {
+  int lambda_local_var = 5;
+  shadowed = 5;
+  class_var = 109;
+  --base_var;
+  --base_base_var;
+  std::puts("break here");
+
+  auto nested_lambda = [this, &lambda_local_var] {
+std::puts("break here");
+lambda_local_var = 0;
+  };
+
+  nested_lambda();
+  --local_var_copy;
+  std::puts("break here");
+
+  struct LocalLambdaClass {
+int lambda_class_local = -12345;
+Foo *outer_ptr;
+
+void inner_method() {
+  auto lambda = [this] {
+std::puts("break here");
+lambda_class_local = -2;
+outer_ptr->class_var *= 2;
+  };
+
+  lambda();
+}
+  };
+
+  LocalLambdaClass l;
+  l.outer_ptr = this;
+  l.inner_method();
+};
+lambda();
+  }
+
+  void non_capturing_method() {
+int local = 5;
+int local2 = 10;
+
+class_var += [=] {
+  std::puts("break here");
+  return local + local2;
+}();
+  }
+};
+
+int main() {
+  Foo f;
+  f.method();
+  f.non_capturing_method();
+  return global_var;
+}
Index: lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
@@ -0,0 +1,123 @@
+""" Test that evaluating expressions from within C++ lambdas works
+Particularly, we test the case of capturing "this" and
+using members of the captured object in expression evaluation
+while we're on a breakpoint inside a lambda.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+
+
+class ExprInsideLambdaTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def expectExprError(self, expr : str, expected : str):
+frame = self.thread.GetFrameAtIndex(0)
+value = frame.EvaluateExpression(expr)
+errmsg = value.GetError().GetCString()
+self.assertIn(expected, errmsg)
+
+def test_expr_inside_lambda(self):
+"""Test that lldb evaluating expressions inside lambda expressions works correctly."""
+self.build()
+(target, process, self.thread, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))
+
+# Inside 'Foo::method'
+
+# Check access to captured 'this'
+self.expect_expr("class_var", result_type="int", result_value="109")
+self.expect_expr("this->class_var", result_type="int", result_value="109")
+
+# Check that captured shadowed variables take preference over the
+# corresponding member variable
+self.expect_expr("shadowed", result_type="int", result_value="5")
+self.expect_expr("this->shadowed", result_type="int", result_value

[Lldb-commits] [PATCH] D129078: WIP: [LLDB][ClangExpression] Allow expression evaluation from within C++ Lambdas

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 442563.
Michael137 added a comment.

- Fixed doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129078

Files:
  lldb/include/lldb/Expression/Materializer.h
  lldb/include/lldb/Expression/UserExpression.h
  lldb/source/Expression/Materializer.cpp
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionVariable.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
  lldb/test/API/commands/expression/expr_inside_lambda/Makefile
  lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
  lldb/test/API/commands/expression/expr_inside_lambda/main.cpp

Index: lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/main.cpp
@@ -0,0 +1,99 @@
+#include 
+#include 
+
+namespace {
+int global_var = -5;
+} // namespace
+
+struct Baz {
+  virtual ~Baz() = default;
+
+  virtual int baz_virt() = 0;
+
+  int base_base_var = 12;
+};
+
+struct Bar : public Baz {
+  virtual ~Bar() = default;
+
+  virtual int baz_virt() override {
+base_var = 10;
+return 1;
+  }
+
+  int base_var = 15;
+};
+
+struct Foo : public Bar {
+  int class_var = 9;
+  int shadowed = -137;
+  int *class_ptr;
+
+  virtual ~Foo() = default;
+
+  virtual int baz_virt() override {
+shadowed = -1;
+return 2;
+  }
+
+  void method() {
+int local_var = 137;
+int shadowed;
+class_ptr = &local_var;
+auto lambda = [&shadowed, this, &local_var,
+   local_var_copy = local_var]() mutable {
+  int lambda_local_var = 5;
+  shadowed = 5;
+  class_var = 109;
+  --base_var;
+  --base_base_var;
+  std::puts("break here");
+
+  auto nested_lambda = [this, &lambda_local_var] {
+std::puts("break here");
+lambda_local_var = 0;
+  };
+
+  nested_lambda();
+  --local_var_copy;
+  std::puts("break here");
+
+  struct LocalLambdaClass {
+int lambda_class_local = -12345;
+Foo *outer_ptr;
+
+void inner_method() {
+  auto lambda = [this] {
+std::puts("break here");
+lambda_class_local = -2;
+outer_ptr->class_var *= 2;
+  };
+
+  lambda();
+}
+  };
+
+  LocalLambdaClass l;
+  l.outer_ptr = this;
+  l.inner_method();
+};
+lambda();
+  }
+
+  void non_capturing_method() {
+int local = 5;
+int local2 = 10;
+
+class_var += [=] {
+  std::puts("break here");
+  return local + local2;
+}();
+  }
+};
+
+int main() {
+  Foo f;
+  f.method();
+  f.non_capturing_method();
+  return global_var;
+}
Index: lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/expr_inside_lambda/TestExprInsideLambdas.py
@@ -0,0 +1,123 @@
+""" Test that evaluating expressions from within C++ lambdas works
+Particularly, we test the case of capturing "this" and
+using members of the captured object in expression evaluation
+while we're on a breakpoint inside a lambda.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+
+
+class ExprInsideLambdaTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def expectExprError(self, expr : str, expected : str):
+frame = self.thread.GetFrameAtIndex(0)
+value = frame.EvaluateExpression(expr)
+errmsg = value.GetError().GetCString()
+self.assertIn(expected, errmsg)
+
+def test_expr_inside_lambda(self):
+"""Test that lldb evaluating expressions inside lambda expressions works correctly."""
+self.build()
+(target, process, self.thread, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))
+
+# Inside 'Foo::method'
+
+# Check access to captured 'this'
+self.expect_expr("class_var", result_type="int", result_value="109")
+self.expect_expr("this->class_var", result_type="int", result_value="109")
+
+# Check that captured shadowed variables take preference over the
+# corresponding member variable
+self.expect_expr("shadowed", result_type="int", result_value="5")
+self.expect_expr("this->shadowed", result_type="int", result_value="-137")
+
+# Check access to local cap

Re: [Lldb-commits] [lldb] f51c47d - Revert "[lldb/test] Don't use preexec_fn for launching inferiors"

[Pavel Labath via lldb-commits]

On 05/07/2022 19:14, Jonas Devlieghere via lldb-commits wrote:


Author: Jonas Devlieghere
Date: 2022-07-05T10:12:57-07:00
New Revision: f51c47d987917d18108f0415334f47c75db9e908

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

LOG: Revert "[lldb/test] Don't use preexec_fn for launching inferiors"

This reverts commit b15b1421bc9a11b318b65b489e5fd58dd917db1f because it
breaks GreenDragon [1]. The bot has been red for several days, so
reverting to green while I take a look.
In case you are looking at this -- you don't have to. I already know 
what the problem was, and I'll submit a fixed version soon.


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

[Lldb-commits] [PATCH] D128932: [lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 442602.
mgorny added a comment.

Replace semaphores with completely non-fancy file-based locking.


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

https://reviews.llvm.org/D128932

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
  lldb/test/API/tools/lldb-server/main.cpp

Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -224,6 +224,8 @@
   int return_value = 0;
 
 #if !defined(_WIN32)
+  bool is_child = false;
+
   // Set the signal handler.
   sig_t sig_result = signal(SIGALRM, signal_handler);
   if (sig_result == SIG_ERR) {
@@ -324,10 +326,32 @@
   func_p();
 #if !defined(_WIN32) && !defined(TARGET_OS_WATCH) && !defined(TARGET_OS_TV)
 } else if (arg == "fork") {
-  assert (fork() != -1);
+  pid_t fork_pid = fork();
+  assert(fork_pid != -1);
+  is_child = fork_pid == 0;
 } else if (arg == "vfork") {
   if (vfork() == 0)
 _exit(0);
+} else if (consume_front(arg, "process:sync:")) {
+  // this is only valid after fork
+  const char *filenames[] = {"parent", "child"};
+  std::string my_file = arg + "." + filenames[is_child];
+  std::string other_file = arg + "." + filenames[!is_child];
+
+  // indicate that we're ready
+  FILE *f = fopen(my_file.c_str(), "w");
+  assert(f);
+  fclose(f);
+
+  // wait for the other process to be ready
+  for (int i = 0; i < 5; ++i) {
+f = fopen(other_file.c_str(), "r");
+if (f)
+  break;
+std::this_thread::sleep_for(std::chrono::milliseconds(125 * i));
+  }
+  assert(f);
+  fclose(f);
 #endif
 } else if (consume_front(arg, "thread:new")) {
   std::promise promise;
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
@@ -1,3 +1,5 @@
+import binascii
+
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
@@ -112,3 +114,33 @@
 def test_vCont_interspersed_nonstop(self):
 self.resume_one_test(run_order=["parent", "child", "parent", "child"],
  use_vCont=True, nonstop=True)
+
+@add_test_categories(["fork"])
+def test_c_both_nonstop(self):
+lock1 = self.getBuildArtifact("lock1")
+lock2 = self.getBuildArtifact("lock2")
+parent_pid, parent_tid, child_pid, child_tid = (
+self.start_fork_test(["fork", "process:sync:" + lock1, "print-pid",
+  "process:sync:" + lock2, "stop"],
+ nonstop=True))
+
+self.test_sequence.add_log_lines([
+"read packet: $Hcp{}.{}#00".format(parent_pid, parent_tid),
+"send packet: $OK#00",
+"read packet: $c#00",
+"send packet: $OK#00",
+"read packet: $Hcp{}.{}#00".format(child_pid, child_tid),
+"send packet: $OK#00",
+"read packet: $c#00",
+"send packet: $OK#00",
+{"direction": "send", "regex": "%Stop:T.*"},
+# see the comment in TestNonStop.py, test_stdio
+"read packet: $vStdio#00",
+"read packet: $vStdio#00",
+"send packet: $OK#00",
+], True)
+ret = self.expect_gdbremote_sequence()
+self.assertIn("PID: {}".format(int(parent_pid, 16)).encode(),
+  ret["O_content"])
+self.assertIn("PID: {}".format(int(child_pid, 16)).encode(),
+  ret["O_content"])
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1107,14 +1107,16 @@
 SendProcessOutput();
 // Then stop the forwarding, so that any late output (see llvm.org/pr25652)
 // does not interfere with our protocol.
-StopSTDIOForwarding();
+if (!m_non_stop)
+  StopSTDIOForwarding();
 HandleInferiorState_Stopped(process);
 break;
 
   case StateType::eStateExited:
 // Same as above
 SendProcessOutput();
-StopSTDIOForwarding();
+if (!m_non_stop)
+  StopSTDIOForwarding();
 HandleInferiorState_Exited(process);
 break;
 
@@ -1417,7 +1419,8 @@
 GDBRemoteCommunicationServerLLGS::Handle_k(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
 
-  StopSTDIOForwarding();
+  if (!m_non_stop)
+StopSTDIOForwarding();
 
   if (m_debugg

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D129166#3632597 , @labath wrote:

> I'm afraid this will not work on systems which do not default to libc++ 
> (which includes at least linux and windows), because `-stdlib=libc++` is not 
> "equivalent" to `-nostdlib++ -lc++` -- the former changes the include paths 
> to use libc++, while the latter doesn't. And, for most of the things that we 
> do here, headers are more important than the library itself.
>
> If the goal here is to preclude any possibility of picking up a different 
> library (it's not clear to me what kind of situation you're referring to in 
> the description), then the most principled approach would be to also use 
> `-nostdinc++` and manually add the appropriate include paths. One nice 
> property of that is that it would work with compilers (gcc) which do not 
> understand the `-stdlib` flag.
>
> The simple solution would be to use the `-stdlib=libc++ -nostdlib++` as a 
> combo, but I am not sure if that would solve what it is supposed to solve (as 
> it still relies on clang to autodetect the header path).
>
> Another thing to consider is that historically (back when we were running the 
> tests against the system compiler by default), the `USE_LIBCPP` flag meant 
> "use the system libc++" (or rather, the default library of the compiler in 
> use). Now we use the in-tree compiler (by default), but our libc++ story is 
> not so clear, as it mostly relies on clang. If the build contains libc++ then 
> we will use that (and sometimes fail due to incorrect usage). If it doesn't, 
> but there's a libc++ on the system somewhere, then we will use that one (and 
> be non-hermetic). Otherwise, we will skip tests which depend on this (and the 
> skipping code uses its own logic, which is not touched here).
>
> Overall, I think it makes sense to use libc++ when we have it available. I am 
> not so sure about the other modes. While I think it would be nice to support 
> running the tests against a random c++ library (similar to how we do it for 
> compilers), I don't think we need to support it unless there someone actively 
> interested in running it. I do think we should support a mode where we don't 
> have a libc++ in the build tree, but I'd be fine if that just causes all of 
> the relevant tests to be skipped.
>
> The thing where that gets interesting is with remote tests. Unlike system 
> libc++, I think we have people (including myself, to some extent) whose job 
> is to make sure those work. Right now, we don't make any special effort to 
> copy the c++ library to the remote system. If it works, it works because the 
> system already has a compatible c++ library lying around. The reason these 
> two are related is that the system libc++ is more likely to be present on a 
> remote system than the one we just built. Messing with the way use libc++ may 
> break some of the remote use cases, so it might be good to take some position 
> on that. Do we not support that? Do we expect the user to make sure the 
> libc++ is on the system already? Do we expect the user to link libc++ 
> statically (I think that's what happens in all of our use cases)?
>
> I realize this isn't very helpful, but I'm trying to explain why I fear this 
> may be more complicated than it may seem at first. Maybe it is not necessary 
> to answer all of these questions for the problem at hand, but these are the 
> questions that have stopped me from doing anything in this area in the past.

Thanks for the thoughtful reply Pavel. The remote tests are something we care 
about as well, so I'd like to have a solution for that. What do you think about 
adding a "stdlib" mode to dotest.py which allows you to pick between "system 
libc++", "system libstdc++" and "just built libc++". The latter would be 
hermetic, and the former would match what we do today.


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

https://reviews.llvm.org/D129166

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

Re: [Lldb-commits] [lldb] f51c47d - Revert "[lldb/test] Don't use preexec_fn for launching inferiors"

[Jonas Devlieghere via lldb-commits]

I didn't get around to it today but I was planning to at least take a look
today. Thanks for following up.

On Wed, Jul 6, 2022 at 8:14 AM Pavel Labath  wrote:

> On 05/07/2022 19:14, Jonas Devlieghere via lldb-commits wrote:
> >
> > Author: Jonas Devlieghere
> > Date: 2022-07-05T10:12:57-07:00
> > New Revision: f51c47d987917d18108f0415334f47c75db9e908
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/f51c47d987917d18108f0415334f47c75db9e908
> > DIFF:
> https://github.com/llvm/llvm-project/commit/f51c47d987917d18108f0415334f47c75db9e908.diff
> >
> > LOG: Revert "[lldb/test] Don't use preexec_fn for launching inferiors"
> >
> > This reverts commit b15b1421bc9a11b318b65b489e5fd58dd917db1f because it
> > breaks GreenDragon [1]. The bot has been red for several days, so
> > reverting to green while I take a look.
> In case you are looking at this -- you don't have to. I already know
> what the problem was, and I'll submit a fixed version soon.
>
> pl
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D129166#3633116 , @JDevlieghere 
wrote:

> Thanks for the thoughtful reply Pavel. The remote tests are something we care 
> about as well, so I'd like to have a solution for that. What do you think 
> about adding a "stdlib" mode to dotest.py which allows you to pick between 
> "system libc++", "system libstdc++" and "just built libc++". The latter would 
> be hermetic, and the former would match what we do today.

Well.. I think that specifying some of this explicitly would be great, but I 
don't think a choice between a libc++ and libstdc++ makes sense.

The way I see it, we have three kinds of tests:

1. Tests which don't care which library we use. This is the vast majority of 
them. The only need it to be there, but the actual test result should not 
depend on the library used in any way. For these tests, we can use any library 
we like. (except maybe for the gmodules test variant, but I don't actually know 
how that one works). I don't think we need to offer a choice here. Ideally we 
would be able to just pick the option that works (it may not be the same option 
for each config).
2. Tests which explicitly require libc++. There shouldn't be too many of these, 
and ideally these would be limited to tests for the libc++ pretty printers and 
such. It doesn't make sense to run these against libstdc++. In fact, that would 
be harmful, because it might actually work, but test the wrong thing. Ideally, 
we'd give the user the option to choose between the system libc++, just-built 
libc++ or a way to specify the arguments needed to build&run these kinds of 
executables.
3. Tests which explicitly require libstdc++. These is the same thing, except 
for libstdc++ pretty printers. And that we obviously don't have an in-tree 
version of libstdc++. And I don't think we have many people interested in 
running tests against libstdc++, so we probably don't have to go overboard on 
this one, but it would be nice to be able to keep running the existing tests 
against system libstdc++ on systems which have one.


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

https://reviews.llvm.org/D129166

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

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D129166#3633243 , @labath wrote:

> In D129166#3633116 , @JDevlieghere 
> wrote:
>
>> Thanks for the thoughtful reply Pavel. The remote tests are something we 
>> care about as well, so I'd like to have a solution for that. What do you 
>> think about adding a "stdlib" mode to dotest.py which allows you to pick 
>> between "system libc++", "system libstdc++" and "just built libc++". The 
>> latter would be hermetic, and the former would match what we do today.
>
> Well.. I think that specifying some of this explicitly would be great, but I 
> don't think a choice between a libc++ and libstdc++ makes sense.
>
> The way I see it, we have three kinds of tests:
>
> 1. Tests which don't care which library we use. This is the vast majority of 
> them. The only need it to be there, but the actual test result should not 
> depend on the library used in any way. For these tests, we can use any 
> library we like. (except maybe for the gmodules test variant, but I don't 
> actually know how that one works). I don't think we need to offer a choice 
> here. Ideally we would be able to just pick the option that works (it may not 
> be the same option for each config).
> 2. Tests which explicitly require libc++. There shouldn't be too many of 
> these, and ideally these would be limited to tests for the libc++ pretty 
> printers and such. It doesn't make sense to run these against libstdc++. In 
> fact, that would be harmful, because it might actually work, but test the 
> wrong thing. Ideally, we'd give the user the option to choose between the 
> system libc++, just-built libc++ or a way to specify the arguments needed to 
> build&run these kinds of executables.
> 3. Tests which explicitly require libstdc++. These is the same thing, except 
> for libstdc++ pretty printers. And that we obviously don't have an in-tree 
> version of libstdc++. And I don't think we have many people interested in 
> running tests against libstdc++, so we probably don't have to go overboard on 
> this one, but it would be nice to be able to keep running the existing tests 
> against system libstdc++ on systems which have one.

Okay, so a binary option to specify the system libc++ or the just-built libc++. 
The libstd+++ stuff remains unchanged.


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

https://reviews.llvm.org/D129166

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

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 442635.
JDevlieghere added a comment.
Herald added a subscriber: mgorny.

I still have to test different configurations, but putting it up here for 
feedback about the general direction. This doesn't account yet for remote runs. 
Should be easy to detect based on whether `--platform-name` is set in dotest.py.


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

https://reviews.llvm.org/D129166

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/configuration.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/dotest_args.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/lit.cfg.py
  lldb/test/API/lit.site.cfg.py.in
  lldb/test/CMakeLists.txt

Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -127,6 +127,7 @@
   # dependency as it's also possible to run the libc++ tests against the libc++
   # installed on the system.
   if (TARGET cxx)
+set(LLDB_HAS_LIBCXX ON)
 add_lldb_test_dependency(cxx)
   endif()
 
@@ -172,6 +173,7 @@
   LLDB_ENABLE_LZMA
   LLVM_ENABLE_ZLIB
   LLVM_ENABLE_SHARED_LIBS
+  LLDB_HAS_LIBCXX
   LLDB_USE_SYSTEM_DEBUGSERVER
   LLDB_IS_64_BITS)
 
Index: lldb/test/API/lit.site.cfg.py.in
===
--- lldb/test/API/lit.site.cfg.py.in
+++ lldb/test/API/lit.site.cfg.py.in
@@ -4,6 +4,7 @@
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@")
 config.llvm_libs_dir = lit_config.substitute("@LLVM_LIBS_DIR@")
+config.llvm_include_dir = lit_config.substitute("@LLVM_INCLUDE_DIR@")
 config.llvm_shlib_dir = lit_config.substitute("@SHLIBDIR@")
 config.llvm_build_mode = lit_config.substitute("@LLVM_BUILD_MODE@")
 config.lit_tools_dir = "@LLVM_LIT_TOOLS_DIR@"
@@ -29,6 +30,7 @@
 config.test_arch = '@LLDB_TEST_ARCH@'
 config.test_compiler = lit_config.substitute('@LLDB_TEST_COMPILER@')
 config.dsymutil = lit_config.substitute('@LLDB_TEST_DSYMUTIL@')
+config.has_libcxx = '@LLDB_HAS_LIBCXX@'
 # The API tests use their own module caches.
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-api")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-api")
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -158,14 +158,22 @@
 if is_configured('dotest_args_str'):
   dotest_cmd.extend(config.dotest_args_str.split(';'))
 
-# Library path may be needed to locate just-built clang.
+# Library path may be needed to locate just-built clang and libcxx.
 if is_configured('llvm_libs_dir'):
   dotest_cmd += ['--env', 'LLVM_LIBS_DIR=' + config.llvm_libs_dir]
 
+# Include path may be needed to locate just-built libcxx.
+if is_configured('llvm_include_dir'):
+  dotest_cmd += ['--env', 'LLVM_INCLUDE_DIR=' + config.llvm_include_dir]
+
 # This path may be needed to locate required llvm tools
 if is_configured('llvm_tools_dir'):
   dotest_cmd += ['--env', 'LLVM_TOOLS_DIR=' + config.llvm_tools_dir]
 
+# If we have a just-built libcxx, prefer it over the system one.
+if is_configured('has_libcxx'):
+  dotest_cmd += ['--hermetic-libcxx']
+
 # Forward ASan-specific environment variables to tests, as a test may load an
 # ASan-ified dylib.
 for env_var in ('ASAN_OPTIONS', 'DYLD_INSERT_LIBRARIES'):
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -387,17 +387,22 @@
 endif
 
 ifeq (1,$(USE_LIBCPP))
-	CXXFLAGS += -DLLDB_USING_LIBCPP
-	ifeq "$(OS)" "Android"
-		# Nothing to do, this is already handled in
-		# Android.rules.
+	ifeq (1,$(USE_HERMETIC_LIBCPP))
+		CXXFLAGS += -nostdlib++ -nostdinc++ -I$(LLVM_INCLUDE_DIR)/c++/v1
+		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR) -lc++
 	else
-		CXXFLAGS += -stdlib=libc++
-		LDFLAGS += -stdlib=libc++
-	endif
-	ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
-		ifneq (,$(LLVM_LIBS_DIR))
-			LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+		CXXFLAGS += -DLLDB_USING_LIBCPP
+		ifeq "$(OS)" "Android"
+			# Nothing to do, this is already handled in
+			# Android.rules.
+		else
+			CXXFLAGS += -stdlib=libc++
+			LDFLAGS += -stdlib=libc++
+		endif
+		ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+			ifneq (,$(LLVM_LIBS_DIR))
+LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+			endif
 		endif
 	endif
 endif
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -43,6

[Lldb-commits] [PATCH] D125509: [LLDB][NFC] Decouple dwarf location table from DWARFExpression.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509

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

[Lldb-commits] [PATCH] D129078: [LLDB][ClangExpression] Allow expression evaluation from within C++ Lambdas

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I'm not sure whether I'm bothered that this patch handles the other captures 
for lambda's with captured "this" pretty differently from ones that don't 
capture "this".  But the method for the ones that don't capture "this" is more 
straightforward, so maybe that's desirable.

You introduced a handful of API's that take StackFrame *'s but then their 
callers end up having StackFrameSP's that they have to call "get" on to pass to 
your new API's.  That doesn't seem desirable.

Other inline comments...




Comment at: lldb/source/Expression/Materializer.cpp:31
 
+static constexpr uint32_t g_default_var_alignment = 8;
+static constexpr uint32_t g_default_var_byte_size = 8;

This seems weird to me.  You didn't do it, you're just replacing hard-coded 8's 
later in the code.  But this seems like something we should be getting from the 
target?  You don't need to solve this for this patch since it isn't intrinsic 
to it, but at least leave a FIXME...



Comment at: lldb/source/Expression/Materializer.cpp:777
+  lldb::ValueObjectSP
+  GetValueObject(ExecutionContextScope *scope) const override {
+return ValueObjectVariable::Create(scope, m_variable_sp);

Naively it seems like it would be a bad idea to call GetValueObject twice.  We 
don't want independent ValueObjectVariable shared pointers floating around for 
the same Entity.  Should this maybe do an `if (!m_variable_sp)` first?



Comment at: lldb/source/Expression/Materializer.cpp:819
+
+  bool LocationExpressionIsValid() const override { return true; }
+

Is this right?  For instance, in the case where the fake "this" Variable in the 
lambda expression has an invalid expression, all the ValueObjects you try to 
make (the "real captured this" as well as any other captures that were hanging 
off the fake "this" would have invalid location expressions.



Comment at: lldb/source/Expression/UserExpression.cpp:101
 
-lldb::addr_t UserExpression::GetObjectPointer(lldb::StackFrameSP frame_sp,
-  ConstString &object_name,
-  Status &err) {
+lldb::ValueObjectSP UserExpression::GetObjectValueObject(
+StackFrame *frame, ConstString const &object_name, Status &err) {

Can we think of a better name for this?  Having two instances of Object with 
different meanings in the same name is confusing?  Even 
`GetObjectPointerValueObject` would be clearer.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:68
+namespace {
+// Return ValueObject of 'this' if we are currently
+// inside the unnamed structure of a C++ lambda.

This comment is confusing.  Partly because you refer to an "unnamed structure" 
which you then look up by name "this".  I think it's clearer to say that clang 
makes an artificial class whose members are the captures, and makes the lambda 
look like a method of that class.  Then the captured "this" is a special case 
of all the captures.

This method also ONLY returns the fake this if the real "this" was captured, 
which you should say.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1725
+
+  if (ClangExpressionVariable::ParserVars *parser_vars =
+  AddExpressionVariable(context, pt, ut, std::move(valobj))) {

Is there enough advantage to move-ing the incoming valobj as opposed to just 
copying the shared pointer?  It's a little weird to have API's that consume one 
of their incoming arguments.  If that really is worth doing, you should note 
that you've done that.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:227
+
+  if (auto thisValSP = GetLambdaValueObject(frame)) {
+uint32_t numChildren = thisValSP->GetNumChildren();

It's worth noting that you handle lambda's that capture "this" and lambda's 
that don't capture "this" differently.  In the former case, we promote all the 
captures to local variables and ignore the fake this.  In the latter case 
(because GetLambdaValueObject only returns a valid ValueObjectSP if it has a 
child called "this"), we keep finding the values by implicit lookup in the fake 
this instead.

I don't think that a problem, no need to use the more complex method just for 
consistency, but you should note that somewhere.



Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h:202
+
+  lldb::addr_t GetCppObjectPointer(StackFrame *frame, ConstString &object_name,
+   Status &err);

Why did you make this take a StackFrame *?  It seems to force some other 
functions to change from StackFrameSP to StackFrame * but the only place it 
gets called it has a StackFrameSP, and explicitly calls

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: jj10306, persona0220.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

A trace bundle contains many trace files, and, in the case of intel pt, the
largest files are often the context switch traces because they are not
compressed by default. As a way to improve this, I'm adding a --compact option
to the `trace save` command that filters out unwanted processes from the
context switch traces. Eventually we can do the same for intel pt traces as
well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
   /// \a llvm::success if the operation was successful, or an \a llvm::Error
   /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory,
+ bool compact);
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cp

[Lldb-commits] [PATCH] D128956: make debugserver able to inspect mach-o binaries present in memory, but not yet registered with dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 442720.
jasonmolenda added a comment.

Updated patch to address Jonas and David's feedback.

Updated the test C file so the mach header sizeofcmds is correct.  Update test 
to test querying an invalid mach-o binary, and the combination of a valid & 
invalid mach-o binary addresses, and check that we get the correct replies.

Add simple checks to MachProcess::GetMachOInformationFromMemory() which parses 
inferior memory as a Mach-O binary, to detect things that are not mach-o 
binaries and return false in those case.

Update callers to GetMachOInformationFromMemory() to mark any entry that had a 
problem parsing it to mark that entry is is_valid=false.

Update MachProcess::FormatDynamicLibrariesIntoJSON() to check the is_valid flag 
for each entry, and don't add those entries to the StructuredData return array. 
 Do return validly formatted mach-o's that were found during the scan, though.

MachProcess::GetLoadedDynamicLibrariesInfos() (not used on modern Darwin 
systems any more) - in the process of updating its handling of macho-parsing 
call, noticed that the indentation was all incorrect for this method.  Fixed 
it, removed an extra return at the end.  This looks like it's undergone some 
incorrect revision over the years -- probably by me.  It's near time this could 
be removed altogether most likely, but I don't want to do that just yet.

I'll add an NDEBUG assert to DynamicLoaderMacOS later which checks that lldb 
gets back the same number of binary image entries that it asked for -- so we 
can flag any case where debugserver omits a binary in our testing.  I can't 
imagine this would happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128956

Files:
  lldb/test/API/macosx/unregistered-macho/Makefile
  lldb/test/API/macosx/unregistered-macho/TestUnregisteredMacho.py
  lldb/test/API/macosx/unregistered-macho/main.c
  lldb/tools/debugserver/source/MacOSX/MachProcess.h
  lldb/tools/debugserver/source/MacOSX/MachProcess.mm

Index: lldb/tools/debugserver/source/MacOSX/MachProcess.mm
===
--- lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -757,12 +757,25 @@
 uint32_t dyld_platform, nub_addr_t mach_o_header_addr, int wordsize,
 struct mach_o_information &inf) {
   uint64_t load_cmds_p;
+  auto is_valid_macho_header = [](uint32_t magic, uint32_t cputype) -> bool {
+if (magic != MH_MAGIC && magic != MH_CIGAM && magic != MH_MAGIC_64 &&
+magic != MH_CIGAM_64)
+  return false;
+if (cputype != CPU_TYPE_X86_64 && cputype != CPU_TYPE_ARM &&
+cputype != CPU_TYPE_ARM64 && cputype != CPU_TYPE_ARM64_32)
+  return false;
+return true;
+  };
+
   if (wordsize == 4) {
 struct mach_header header;
 if (ReadMemory(mach_o_header_addr, sizeof(struct mach_header), &header) !=
 sizeof(struct mach_header)) {
   return false;
 }
+if (!is_valid_macho_header(header.magic, header.cputype))
+  return false;
+
 load_cmds_p = mach_o_header_addr + sizeof(struct mach_header);
 inf.mach_header.magic = header.magic;
 inf.mach_header.cputype = header.cputype;
@@ -779,6 +792,8 @@
&header) != sizeof(struct mach_header_64)) {
   return false;
 }
+if (!is_valid_macho_header(header.magic, header.cputype))
+  return false;
 load_cmds_p = mach_o_header_addr + sizeof(struct mach_header_64);
 inf.mach_header.magic = header.magic;
 inf.mach_header.cputype = header.cputype;
@@ -896,6 +911,8 @@
   const size_t image_count = image_infos.size();
 
   for (size_t i = 0; i < image_count; i++) {
+if (!image_infos[i].is_valid)
+  continue;
 JSONGenerator::DictionarySP image_info_dict_sp(
 new JSONGenerator::Dictionary());
 image_info_dict_sp->AddIntegerItem("load_address",
@@ -970,7 +987,6 @@
   }
 
   JSONGenerator::DictionarySP reply_sp(new JSONGenerator::Dictionary());
-  ;
   reply_sp->AddItem("images", image_infos_array_sp);
 
   return reply_sp;
@@ -985,8 +1001,8 @@
 // information.
 JSONGenerator::ObjectSP MachProcess::GetLoadedDynamicLibrariesInfos(
 nub_process_t pid, nub_addr_t image_list_address, nub_addr_t image_count) {
-  JSONGenerator::DictionarySP reply_sp;
 
+  JSONGenerator::ObjectSP empty_reply_sp(new JSONGenerator::Dictionary());
   int pointer_size = GetInferiorAddrSize(pid);
 
   std::vector image_infos;
@@ -994,91 +1010,89 @@
 
   uint8_t *image_info_buf = (uint8_t *)malloc(image_infos_size);
   if (image_info_buf == NULL) {
-return reply_sp;
+return empty_reply_sp;
+  }
+  if (ReadMemory(image_list_address, image_infos_size, image_info_buf) !=
+  image_infos_size) {
+return empty_reply_sp;
   }
-if (ReadMemory(image_list_address, image_infos_size, image_info_buf) !=
-image_infos_size) {
-

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Commands/CommandObjectProcess.cpp:582
   }
-  
+
   Target *target = m_exe_ctx.GetTargetPtr();

many formatting changes sneaked in. The actual changes are in the end of this 
file



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:241
+if (std::error_code ec =
+llvm::sys::fs::copy_file(file, path_to_copy_module.GetPath()))
   return createStringError(

this was a bug in the existing code that prevented copying a module that was 
loaded from a bundle


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

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

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 442759.
wallace added a comment.

minor improvements for autocompletion in the CLI


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSwitchDecoder.h"

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 442765.
wallace added a comment.

make relative all paths being returned in the description file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSw

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 442769.
wallace added a comment.

improve command handling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSwitchDecoder.h"
 #include "TraceIntelPT

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 442770.
wallace added a comment.

fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSave.py

Index: lldb/test/API/commands/trace/TestTraceSave.py
===
--- lldb/test/API/commands/trace/TestTraceSave.py
+++ lldb/test/API/commands/trace/TestTraceSave.py
@@ -43,12 +43,12 @@
 self.expect("n")
 
 # Check the output when saving without providing the directory argument
-self.expect("process trace save -d",
-substrs=["error: last option requires an argument"],
+self.expect("process trace save ",
+substrs=["error: a single path to a directory where the trace bundle will be created is required"],
 error=True)
 
 # Check the output when saving to an invalid directory
-self.expect("process trace save -d /",
+self.expect("process trace save /",
 substrs=["error: couldn't write to the file"],
 error=True)
 
@@ -58,7 +58,7 @@
 substrs=["intel-pt"])
 
 # Check the output when not doing live tracing
-self.expect("process trace save -d " +
+self.expect("process trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_not_live_dir"))
 
 def testSaveMultiCpuTrace(self):
@@ -76,7 +76,7 @@
 self.expect("b 7")
 
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
-self.expect("process trace save -d " + output_dir)
+self.expect("process trace save " + output_dir)
 
 def checkSessionBundle(session_file_path):
 with open(session_file_path) as session_file:
@@ -107,7 +107,7 @@
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
 self.expect("trace load " + os.path.join(output_dir, "trace.json"))
 output_copy_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "copy_trace_save")
-self.expect("process trace save -d " + output_copy_dir)
+self.expect("process trace save " + output_copy_dir)
 
 # We now check that the new bundle is correct on its own
 copied_trace_session_file = os.path.join(output_copy_dir, "trace.json")
@@ -153,7 +153,7 @@
 last_ten_instructions = res.GetOutput()
 
 # Now, save the trace to 
-self.expect("process trace save -d " +
+self.expect("process trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir"))
 
 # Load the trace just saved
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 442779.
wallace added a comment.

rename 'process trace save' to 'trace save' because it can actually dump the 
information of multiple processes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSave.py

Index: lldb/test/API/commands/trace/TestTraceSave.py
===
--- lldb/test/API/commands/trace/TestTraceSave.py
+++ lldb/test/API/commands/trace/TestTraceSave.py
@@ -14,7 +14,7 @@
 
 def testErrorMessages(self):
 # We first check the output when there are no targets
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: invalid target, create a target using the 'target create' command"],
 error=True)
 
@@ -22,7 +22,7 @@
 self.expect("target create " +
 os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
 
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: Command requires a current process."],
 error=True)
 
@@ -30,7 +30,7 @@
 self.expect("b main")
 self.expect("run")
 
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: Process is not being traced"],
 error=True)
 
@@ -43,12 +43,12 @@
 self.expect("n")
 
 # Check the output when saving without providing the directory argument
-self.expect("process trace save -d",
-substrs=["error: last option requires an argument"],
+self.expect("trace save ",
+substrs=["error: a single path to a directory where the trace bundle will be created is required"],
 error=True)
 
 # Check the output when saving to an invalid directory
-self.expect("process trace save -d /",
+self.expect("trace save /",
 substrs=["error: couldn't write to the file"],
 error=True)
 
@@ -58,7 +58,7 @@
 substrs=["intel-pt"])
 
 # Check the output when not doing live tracing
-self.expect("process trace save -d " +
+self.expect("trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_not_live_dir"))
 
 def testSaveMultiCpuTrace(self):
@@ -76,7 +76,7 @@
 self.expect("b 7")
 
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
-self.expect("process trace save -d " + output_dir)
+self.expect("trace save " + output_dir)
 
 def checkSessionBundle(session_file_path):
 with open(session_file_path) as session_file:
@@ -107,7 +107,7 @@
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
 self.expect("trace load " + os.path.join(output_dir, "trace.json"))
 output_copy_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "copy_trace_save")
-self.expect("process trace save -d " + output_copy_dir)
+self.expect("trace save " + output_copy_dir)
 
 # We now check that the new bundle is correct on its own
 copied_trace_session_file = os.path.join(output_copy_dir, "trace.json")
@@ -153,7 +153,7 @@
 last_ten_instructions = res.GetOutput()
 
 # Now, save the trace to 
-self.expect("process trace save -d " +
+self.expect("trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir"))
 
 # Load the trace just saved
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+   

[Lldb-commits] [PATCH] D129249: [trace][intel pt] Measure the time it takes to decode a thread in per-cpu mode

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: jj10306, persona0220.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This metric was missing. We were only measuring in per-thread mode, and
this completes the work.

For a sample trace I have, the `dump info` command shows

  Timing for this thread:
  Decoding instructions: 0.12s

I also improved a bit the TaskTime function so that callers don't need to
specify the template argument


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129249

Files:
  lldb/source/Plugins/Trace/intel-pt/TaskTimer.h
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
@@ -39,30 +39,35 @@
   if (Error err = CorrelateContextSwitchesAndIntelPtTraces())
 return std::move(err);
 
-  auto it = m_decoded_threads.find(thread.GetID());
-  if (it != m_decoded_threads.end())
-return it->second;
-
-  DecodedThreadSP decoded_thread_sp =
-  std::make_shared(thread.shared_from_this());
-
   TraceIntelPTSP trace_sp = GetTrace();
 
-  Error err = trace_sp->OnAllCpusBinaryDataRead(
-  IntelPTDataKinds::kIptTrace,
-  [&](const DenseMap> &buffers) -> Error {
-auto it = m_continuous_executions_per_thread->find(thread.GetID());
-if (it != m_continuous_executions_per_thread->end())
-  return DecodeSystemWideTraceForThread(*decoded_thread_sp, *trace_sp,
-buffers, it->second);
-
-return Error::success();
+  return trace_sp
+  ->GetThreadTimer(thread.GetID())
+  .TimeTask("Decoding instructions", [&]() -> Expected {
+auto it = m_decoded_threads.find(thread.GetID());
+if (it != m_decoded_threads.end())
+  return it->second;
+
+DecodedThreadSP decoded_thread_sp =
+std::make_shared(thread.shared_from_this());
+
+Error err = trace_sp->OnAllCpusBinaryDataRead(
+IntelPTDataKinds::kIptTrace,
+[&](const DenseMap> &buffers) -> Error {
+  auto it =
+  m_continuous_executions_per_thread->find(thread.GetID());
+  if (it != m_continuous_executions_per_thread->end())
+return DecodeSystemWideTraceForThread(
+*decoded_thread_sp, *trace_sp, buffers, it->second);
+
+  return Error::success();
+});
+if (err)
+  return std::move(err);
+
+m_decoded_threads.try_emplace(thread.GetID(), decoded_thread_sp);
+return decoded_thread_sp;
   });
-  if (err)
-return std::move(err);
-
-  m_decoded_threads.try_emplace(thread.GetID(), decoded_thread_sp);
-  return decoded_thread_sp;
 }
 
 static Expected>
@@ -153,7 +158,7 @@
   if (m_continuous_executions_per_thread)
 return Error::success();
 
-  Error err = GetTrace()->GetTimer().ForGlobal().TimeTask(
+  Error err = GetTrace()->GetGlobalTimer().TimeTask(
   "Context switch and Intel PT traces correlation", [&]() -> Error {
 if (auto correlation = DoCorrelateContextSwitchesAndIntelPtTraces()) {
   m_continuous_executions_per_thread.emplace(std::move(*correlation));
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -157,6 +157,14 @@
   /// The timer object for this trace.
   TaskTimer &GetTimer();
 
+  /// \return
+  /// The ScopedTaskTimer object for the given thread in this trace.
+  ScopedTaskTimer &GetThreadTimer(lldb::tid_t tid);
+
+  /// \return
+  /// The global copedTaskTimer object for this trace.
+  ScopedTaskTimer &GetGlobalTimer();
+
   TraceIntelPTSP GetSharedPtr();
 
 private:
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
@@ -199,10 +199,10 @@
   std::chrono::milliseconds duration) {
   s.Format("{0}: {1:2}s\n", name, duration.count() / 1000.0);
 };
-GetTimer().ForThread(tid).ForEachTimedTask(print_duration);
+GetThreadTimer(tid).ForEachTimedTask(print_duration);
 
 s << "\n  Timing for global tasks:\n";
-GetTimer().ForGlobal().ForEachTimedTask(print_duration);
+GetGlobalTimer().ForEachTimedTask(print_duration);