date:20210812

Re: [Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread Jim Ingham via lldb-commits

Sure, thanks for the context!  

Actually, the posix_spawn usage is only in the darwin Host code, probably 
because for posix_spawn to work for debugging
you need a couple of non-standard flags that were added on Darwin for us (the 
CoreOS folks really wanted us to use posix_spawn).
So you probably can't use it on Linux w/o some kernel support.

Jim


> On Aug 12, 2021, at 3:56 PM, David Blaikie via Phabricator 
>  wrote:
> 
> dblaikie added a comment.
> 
> In D105732#2942716 , @jingham wrote:
> 
>> Do modern Linux's not have posix_spawn?  If it exists that's a better 
>> interface, and lets the system handle a lot of the complicated machinations 
>> you have to do by hand if you roll it yourself out of fork and exec.
> 
> https://man7.org/linux/man-pages/man3/posix_spawn.3.html - looks like it's 
> there. I was reporting on the current implementation/adding some detail on 
> the current state of things.
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D105732/new/
> 
> https://reviews.llvm.org/D105732
> 

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

[Lldb-commits] [PATCH] D107669: [trace] [intel pt] Create a "process trace save" command

2021-08-12 Thread hanbing wang via Phabricator via lldb-commits

hanbingwang updated this revision to Diff 366151.
hanbingwang edited the summary of this revision.
hanbingwang added a comment.
Herald added a subscriber: pengfei.

*TraceSessionSaver.h, TraceSessionSaver.cpp:
-move BuildModulesSection(), BuildThreadsSection(), BuildProcessesSection(), 
WriteSessionToFile() from TraceIntelPTSessionSaver.cpp to this file. These 
functions are generic for all trace plug-ins.

*TestTraceSave.py:
-new test case for "process trace save" command

*TraceIntelPT.h, Trace.h

- change IsTraced(const Thread ) to IsTraced(lldb::tid_t tid)

*TraceIntelPTJSONStructs.h
-rename JSONTraceIntelPTSchema to JSONTraceIntelPTSession


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

https://reviews.llvm.org/D107669

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/common/CMakeLists.txt
  lldb/source/Plugins/Trace/common/TraceJSONStructs.cpp
  lldb/source/Plugins/Trace/common/TraceJSONStructs.h
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
  lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp
  lldb/source/Plugins/Trace/common/TraceSessionSaver.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h
  lldb/test/API/commands/trace/TestTraceSave.py

Index: lldb/test/API/commands/trace/TestTraceSave.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceSave.py
@@ -0,0 +1,75 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceSave(TraceIntelPTTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("process trace save",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("process trace save",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("process trace save",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testSaveTrace(self):
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+self.expect("b main")
+self.expect("r")
+self.expect("thread trace start")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+self.expect("n")
+
+ci = self.dbg.GetCommandInterpreter()
+res = lldb.SBCommandReturnObject()
+
+ci.HandleCommand("thread trace dump instructions -c 10 --forwards", res)
+self.assertEqual(res.Succeeded(), True)
+first_ten_instructions = res.GetOutput()
+
+ci.HandleCommand("thread trace dump instructions -c 10", res)
+self.assertEqual(res.Succeeded(), True)
+last_ten_instructions = res.GetOutput()
+
+# Save the trace to 
+self.expect("process trace save -d " +
+os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir"))
+
+# Load the trace just saved
+self.expect("trace load -v " +
+os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir", "trace.json"),
+substrs=["intel-pt"])
+
+# Compare with instructions saved at the first time
+ci.HandleCommand("thread trace dump instructions -c 10 --forwards", res)
+self.assertEqual(res.Succeeded(), True)
+self.assertEqual(res.GetOutput(), first_ten_instructions)
+
+ci.HandleCommand("thread trace dump instructions -c 10", res)
+self.assertEqual(res.Succeeded(), True)
+self.assertEqual(res.GetOutput(), last_ten_instructions)
Index:

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread David Blaikie via Phabricator via lldb-commits

dblaikie added a comment.

In D105732#2942716 , @jingham wrote:

> Do modern Linux's not have posix_spawn?  If it exists that's a better 
> interface, and lets the system handle a lot of the complicated machinations 
> you have to do by hand if you roll it yourself out of fork and exec.

https://man7.org/linux/man-pages/man3/posix_spawn.3.html - looks like it's 
there. I was reporting on the current implementation/adding some detail on the 
current state of things.


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

Do modern Linux's not have posix_spawn?  If it exists that's a better 
interface, and lets the system handle a lot of the complicated machinations you 
have to do by hand if you roll it yourself out of fork and exec.


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread David Blaikie via Phabricator via lldb-commits

dblaikie added a comment.

In D105732#2942355 , @clayborg wrote:

> Sorry for the delay. This seems like it should work just fine. Where is this 
> actually used? I thought most clients should be using posix_spawn() as this 
> is the preferred launching mechanism over using fork/exec

Looks like posix_spawn is used in the macosx host, but not in the posix host 
(where, as @rdhindsa mentioned, fork is used), for whatever that's worth.


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread Rumeet Dhindsa via Phabricator via lldb-commits

rdhindsa added a comment.

fork is being called in LaunchProcess function in this 
file(ProcessLauncherPosixFork.cpp) itself.

Thank you for the review!


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

2021-08-12 Thread Jason Molenda via Phabricator via lldb-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7150b562081f: Symbolicate aarch64 adrp+add pc-relative addr 
in disass (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
  
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64.yaml
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c

Index: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c
@@ -0,0 +1,110 @@
+#include 
+
+// For the test case, we really want the the layout of this binary
+// to be:
+//
+//   foo()
+//   bar() - 4096 bytes of nop's
+//   main()
+//   "HI" string
+//
+// in reality getting this layout from the compiler and linker
+// is a crapshoot, so I have yaml's checked in of the correct
+// layout.  Recompiling from source may not get the needed
+// binary layout.
+
+static int bar();
+static int foo() { return 5 + bar(); }
+// A function of 4096 bytes, so when main() loads the
+// address of foo() before this one, it has to subtract
+// a 4096 page.
+#define SIXTY_FOUR_BYTES_NOP   \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");
+
+static int bar() {
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  return 5;
+}
+int main() {
+  int (*f)(void) = foo;
+  puts("HI");
+  return f();
+}
Index: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
===
--- /dev/null
+++ lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
@@ -0,0 +1,379 @@
+--- !mach-o
+FileHeader:
+  magic:

[Lldb-commits] [lldb] 7150b56 - Symbolicate aarch64 adrp+add pc-relative addr in disass

2021-08-12 Thread Jason Molenda via lldb-commits


Author: Jason Molenda
Date: 2021-08-12T14:44:17-07:00
New Revision: 7150b562081ffb2ec5406edfa579b16d3ec20d90

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

LOG: Symbolicate aarch64 adrp+add pc-relative addr in disass

On aarch64 a two instruction sequence is used to calculate a
pc-relative address, add some state to the DisassemblerLLVMC
symbolicator so it can track the necessary data across the
two instructions and compute the address being calculated.

Differential Revision: https://reviews.llvm.org/D107213
rdar://49119253

Added: 

lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64.yaml

lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c

Modified: 
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h

Removed: 




diff  --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp 
b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index 7cd505d0ed292..24998e96af6fd 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1030,7 +1030,8 @@ bool 
DisassemblerLLVMC::MCDisasmInstance::IsCall(llvm::MCInst _inst) const {
 DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec ,
  const char *flavor_string)
 : Disassembler(arch, flavor_string), m_exe_ctx(nullptr), m_inst(nullptr),
-  m_data_from_file(false) {
+  m_data_from_file(false), m_adrp_address(LLDB_INVALID_ADDRESS),
+  m_adrp_insn() {
   if (!FlavorValidForArchSpec(arch, m_flavor.c_str())) {
 m_flavor.assign("default");
   }
@@ -1310,6 +1311,46 @@ const char *DisassemblerLLVMC::SymbolLookup(uint64_t 
value, uint64_t *type_ptr,
   Target *target = m_exe_ctx ? m_exe_ctx->GetTargetPtr() : nullptr;
   Address value_so_addr;
   Address pc_so_addr;
+  if (target->GetArchitecture().GetMachine() == llvm::Triple::aarch64 ||
+  target->GetArchitecture().GetMachine() == llvm::Triple::aarch64_be ||
+  target->GetArchitecture().GetMachine() == llvm::Triple::aarch64_32) {
+if (*type_ptr == LLVMDisassembler_ReferenceType_In_ARM64_ADRP) {
+  m_adrp_address = pc;
+  m_adrp_insn = value;
+  *name = nullptr;
+  *type_ptr = LLVMDisassembler_ReferenceType_InOut_None;
+  return nullptr;
+}
+// If this instruction is an ADD and
+// the previous instruction was an ADRP and
+// the ADRP's register and this ADD's register are the same,
+// then this is a pc-relative address calculation.
+if (*type_ptr == LLVMDisassembler_ReferenceType_In_ARM64_ADDXri &&
+m_adrp_insn.hasValue() && m_adrp_address == pc - 4 &&
+(m_adrp_insn.getValue() & 0x1f) == ((value >> 5) & 0x1f)) {
+  uint32_t addxri_inst;
+  uint64_t adrp_imm, addxri_imm;
+  // Get immlo and immhi bits, OR them together to get the ADRP imm
+  // value.
+  adrp_imm = ((m_adrp_insn.getValue() & 0x00e0) >> 3) |
+ ((m_adrp_insn.getValue() >> 29) & 0x3);
+  // if high bit of immhi after right-shifting set, sign extend
+  if (adrp_imm & (1ULL << 20))
+adrp_imm |= ~((1ULL << 21) - 1);
+
+  addxri_inst = value;
+  addxri_imm = (addxri_inst >> 10) & 0xfff;
+  // check if 'sh' bit is set, shift imm value up if so
+  // (this would make no sense, ADRP already gave us this part)
+  if ((addxri_inst >> (12 + 5 + 5)) & 1)
+addxri_imm <<= 12;
+  value = (m_adrp_address & 0xf000LL) + (adrp_imm << 12) +
+  addxri_imm;
+}
+m_adrp_address = LLDB_INVALID_ADDRESS;
+m_adrp_insn.reset();
+  }
+
   if (m_inst->UsingFileAddress()) {
 ModuleSP module_sp(m_inst->GetAddress().GetModule());
 if (module_sp) {
@@ -1371,6 +1412,12 @@ const char *DisassemblerLLVMC::SymbolLookup(uint64_t 
value, uint64_t *type_ptr,
 }
   }
 
+  // TODO: llvm-objdump sets the type_ptr to the
+  // LLVMDisassembler_ReferenceType_Out_* values
+  // based on where value_so_addr is pointing, with
+  // Mach-O specific augmentations in MachODump.cpp. e.g.
+  // see what AArch64ExternalSymbolizer::tryAddingSymbolicOperand
+  // handles.
   *type_ptr = LLVMDisassembler_ReferenceType_InOut_None;
   *name = nullptr;
   return nullptr;

diff  --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet

2021-08-12 Thread Michał Górny via Phabricator via lldb-commits

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

Handle unsupported `qLaunchSuccess` gracefully on the client. This effectively 
makes it possible to launch programs via gdbserver.

Enhance the client tests to verify that the process has actually 'started'.


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

https://reviews.llvm.org/D107931

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -10,6 +10,9 @@
 the initial set of tests implemented.
 """
 
+import binascii
+import itertools
+
 import unittest2
 import gdbremote_testcase
 import lldbgdbserverutils
@@ -1246,3 +1249,61 @@
 # Make sure we read back what we wrote.
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
+
+def test_launch_via_A(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+# NB: strictly speaking we should use %x here but this packet
+# is deprecated, so no point in changing lldb-server's expectations
+self.test_sequence.add_log_lines(
+["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
+ tuple(itertools.chain.from_iterable(
+ [(len(x), x) for x in hex_args])),
+ "send packet: $OK#00",
+ "read packet: $c#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+self.assertEqual(context["O_content"],
+ b'arg1\r\narg2\r\narg3\r\n')
+
+def test_launch_via_vRun(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
+ {"direction": "send",
+  "regex": r"^\$T([0-9a-fA-F]+)"},
+ "read packet: $c#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+
+def test_launch_via_vRun_no_args(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+hex_path = binascii.b2a_hex(exe_path.encode()).decode()
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s#00" % (hex_path,),
+ {"direction": "send",
+  "regex": r"^\$T([0-9a-fA-F]+)"},
+ "read packet: $c#00",
+ "send packet: $W00#00"],
+True)
+self.expect_gdbremote_sequence()
Index: lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
===
--- lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
+++ lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
@@ -183,6 +183,10 @@
 return self.qPathComplete()
 if packet.startswith("vFile:"):
 return self.vFile(packet)
+if packet.startswith("vRun;"):
+return self.vRun(packet)
+if packet.startswith("qLaunchSuccess"):
+return self.qLaunchSuccess()
 
 return self.other(packet)
 
@@ -293,6 +297,12 @@
 def vFile(self, packet):
 return ""
 
+def vRun(self, packet):
+return ""
+
+def qLaunchSuccess(self):
+return ""
+
 """
 Raised when we receive a packet for which there is no default action.
 Override the responder class to implement behavior suitable for the test at
Index:

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

2021-08-12 Thread Greg Clayton via Phabricator via lldb-commits

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

Sorry for the delay. This seems like it should work just fine. Where is this 
actually used? I thought most clients should be using posix_spawn() as this is 
the preferred launching mechanism over using fork/exec


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

2021-08-12 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:305
+  "ScriptedProcess::%s ERROR = %s", __FUNCTION__,
+  message.str().c_str());
+return false;

Same comment as in the other review: no need to allocate a std::string here. 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:309-320
+  Status error;
+  lldb::ThreadSP thread_sp;
+  ScriptLanguage language = m_interpreter->GetLanguage();
+
+  if (language != eScriptLanguagePython)
+return error_with_message(
+(llvm::Twine("ScriptInterpreter language (") +





Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:109
+const char *ScriptedThread::GetQueueName() {
+  if (m_name.empty()) {
+CheckInterpreterAndScriptObject();




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107585

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

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet

2021-08-12 Thread Michał Górny via Phabricator via lldb-commits

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

Correct the vRun packet to return a stop reason rather than 'OK' to match GDB 
behavior. Add initial client tests.


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

https://reviews.llvm.org/D107931

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -10,6 +10,9 @@
 the initial set of tests implemented.
 """
 
+import binascii
+import itertools
+
 import unittest2
 import gdbremote_testcase
 import lldbgdbserverutils
@@ -1246,3 +1249,59 @@
 # Make sure we read back what we wrote.
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
+
+def test_launch_via_A(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+# NB: strictly speaking we should use %x here but this packet
+# is deprecated, so no point in changing lldb-server's expectations
+self.test_sequence.add_log_lines(
+["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
+ tuple(itertools.chain.from_iterable(
+ [(len(x), x) for x in hex_args])),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+self.assertEqual(context["O_content"],
+ b'arg1\r\narg2\r\narg3\r\n')
+
+def test_launch_via_vRun(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+
+def test_launch_via_vRun_no_args(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+hex_path = binascii.b2a_hex(exe_path.encode()).decode()
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s#00" % (hex_path,),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+self.expect_gdbremote_sequence()
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -363,6 +363,8 @@
 return eServerPacketType_vCont;
   if (PACKET_MATCHES("vCont?"))
 return eServerPacketType_vCont_actions;
+  if (PACKET_STARTS_WITH("vRun;"))
+return eServerPacketType_vRun;
 }
 break;
   case '_':
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
@@ -182,6 +182,9 @@
   RegisterMemberFunctionHandler(
   StringExtractorGDBRemote::eServerPacketType_vCont_actions,
   ::Handle_vCont_actions);
+  RegisterMemberFunctionHandler(
+  StringExtractorGDBRemote::eServerPacketType_vRun,
+  ::Handle_vRun);
   RegisterMemberFunctionHandler(
   StringExtractorGDBRemote::eServerPacketType_x,
   ::Handle_memory_read);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h

[Lldb-commits] [PATCH] D107470: 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor

2021-08-12 Thread Jan Kratochvil via Phabricator via lldb-commits

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

Comment at: llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp:128
+  EXPECT_EQ(Table.getAddrSize(), 8U);
+  Extractor.setAddressSize(Table.getAddrSize());
+  Expected List = Table.findList(Extractor, Offset);

ikudrin wrote:
> This looks odd. `DWARFListTableBase::findList()` should not require the 
> setting to be done in the calling code if it can apply it itself. 
I agree but as I have found it is not so simple. `findList` is being used also 
with uninitialized headers: [[ 
https://github.com/llvm/llvm-project/blob/e4977f9cb58ff7820d0287ba309490af57787749/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp#L619
 | llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp DWARFUnit::findRnglistFromOffset ]]
So I had to implement `DWARFListTableBase::setAddrSize` which is a bit ugly. 
One cannot find appropriate `.debug_loclists` header just for the requested 
`offset` without `DW_AT_rnglists_base` which may be missing, that is what 
D106466 is about. Or one could use `DWARFListTableHeader::create` from that 
patch but that looks to me as too much complicated.

In fact the codebase is already designed so that caller has to prepare proper 
`DWARFDataExtractor` - `llvm::DWARFDebugLoclists` and `llvm::DWARFDebugLoc` 
also rely on it, I tried to implement this callee approach for them as: 
https://www.jankratochvil.net/t/gccoffset1-2-2b.patch
But I am not going to submit it as one either has to provide address size to 
both `DWARFDataExtractor` and `llvm::DWARFDebugLoc*` constructors or otherwise 
`DWARFDataExtractor` would have unset address size (implemented in the patch) 
which is also a bit ugly.

So personally I would choose the previous patch but I am fine also with this 
patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107470

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

[Lldb-commits] [PATCH] D107470: 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor

2021-08-12 Thread Jan Kratochvil via Phabricator via lldb-commits

jankratochvil updated this revision to Diff 366017.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107470

Files:
  lldb/unittests/SymbolFile/DWARF/DWARFUnitTest.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
  llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
  llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
  llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
  llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/DebugInfo/DWARF/DWARFListTable.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugRnglists.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
@@ -99,4 +100,74 @@
   EXPECT_EQ(Header.length(), sizeof(SecData) - 1);
 }
 
+TEST(DWARFListTableHeader, AddressSize64) {
+  static const char SecData[] = "\x1a\x00\x00\x00" // Length
+"\x05\x00" // Version
+"\x08" // Address size
+"\x00" // Segment selector size
+"\x00\x00\x00\x00" // Offset entry count
+"\x06" // DW_RLE_start_end
+"\x07\x06\x05\x04" // 64-bit starting address
+"\x03\x02\x01\x00" // 64-bit starting address
+"\x87\x86\x85\x84" // 64-bit ending address
+"\x83\x82\x81\x80" // 64-bit ending address
+"\x00";// DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+   /*isLittleEndian=*/true,
+   /*AddrSize=*/4);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+  /*ListTypeString=*/"range");
+  uint64_t Offset = 0;
+  llvm::DWARFDebugRnglistTable Table;
+  llvm::Error E = Table.extractHeaderAndOffsets(Extractor, );
+  EXPECT_FALSE(!!E);
+  EXPECT_EQ(Offset, 12U);
+  EXPECT_EQ(Table.length(), sizeof(SecData) - 1);
+  EXPECT_EQ(Extractor.getAddressSize(), 4U);
+  EXPECT_EQ(Table.getAddrSize(), 8U);
+  Expected List = Table.findList(Extractor, Offset);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x0001020304050607U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x8081828384858687U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 17U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
+TEST(DWARFListTableHeader, AddressSize32) {
+  static const char SecData[] = "\x12\x00\x00\x00" // Length
+"\x05\x00" // Version
+"\x04" // Address size
+"\x00" // Segment selector size
+"\x00\x00\x00\x00" // Offset entry count
+"\x06" // DW_RLE_start_end
+"\x03\x02\x01\x00" // 32-bit starting address
+"\x83\x82\x81\x80" // 32-bit ending address
+"\x00";// DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+   /*isLittleEndian=*/true,
+   /*AddrSize=*/8);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+  /*ListTypeString=*/"range");
+  uint64_t Offset = 0;
+  llvm::DWARFDebugRnglistTable Table;
+  llvm::Error E = Table.extractHeaderAndOffsets(Extractor, );
+  EXPECT_FALSE(!!E);
+  EXPECT_EQ(Offset, 12U);
+  EXPECT_EQ(Table.length(), sizeof(SecData) - 1);
+  EXPECT_EQ(Extractor.getAddressSize(), 8U);
+  EXPECT_EQ(Table.getAddrSize(), 4U);
+  Expected List = Table.findList(Extractor, Offset);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x00010203U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x80818283U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 9U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
 } // end anonymous namespace
Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

[Lldb-commits] [PATCH] D107704: [LLDB][NFC] Simplify IOHandler's getLine to avoid strange casts and redundant checks

2021-08-12 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

> Only clang-tidy is upset and it is falsely flagging "missing" files

This is because llvm precommit doesn't actually build lldb (due to some test 
failures they were/are having) so those headers are unknown to clang-tidy when 
it runs. You can ignore it.


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

https://reviews.llvm.org/D107704

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

[Lldb-commits] [PATCH] D107704: [LLDB][NFC] Simplify IOHandler's getLine to avoid strange casts and redundant checks

2021-08-12 Thread Alf via Phabricator via lldb-commits

gAlfonso-bit added a comment.

Only clang-tidy is upset and it is falsely flagging "missing" files


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

https://reviews.llvm.org/D107704

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

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

2021-08-12 Thread Omar Emara via Phabricator via lldb-commits

OmarEmaraDev updated this revision to Diff 365958.
OmarEmaraDev added a comment.

- Separate environment field into two fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3985,6 +3985,45 @@
   return ComputeEnvironment();
 }
 
+Environment TargetProperties::GetInheritedEnvironment() const {
+  Environment environment;
+
+  if (m_target == nullptr)
+return environment;
+
+  if (!m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, ePropertyInheritEnv,
+  g_target_properties[ePropertyInheritEnv].default_uint_value != 0))
+return environment;
+
+  PlatformSP platform_sp = m_target->GetPlatform();
+  if (platform_sp == nullptr)
+return environment;
+
+  Environment platform_environment = platform_sp->GetEnvironment();
+  for (const auto  : platform_environment)
+environment[KV.first()] = KV.second;
+
+  Args property_unset_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyUnsetEnvVars,
+property_unset_environment);
+  for (const auto  : property_unset_environment)
+environment.erase(var.ref());
+
+  return environment;
+}
+
+Environment TargetProperties::GetTargetEnvironment() const {
+  Args property_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyEnvVars,
+property_environment);
+  Environment environment;
+  for (const auto  : Environment(property_environment))
+environment[KV.first()] = KV.second;
+
+  return environment;
+}
+
 void TargetProperties::SetEnvironment(Environment env) {
   // TODO: Get rid of the Args intermediate step
   const uint32_t idx = ePropertyEnvVars;
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObjectUpdater.h"
 #include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -1260,6 +1261,14 @@
 
   const std::string () { return m_content; }
 
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_content.clear();
+  return;
+}
+m_content = text;
+  }
+
 protected:
   std::string m_label;
   bool m_required;
@@ -1619,6 +1628,33 @@
   }
 };
 
+class LazyBooleanFieldDelegate : public ChoicesFieldDelegate {
+public:
+  LazyBooleanFieldDelegate(const char *label, const char *calculate_label)
+  : ChoicesFieldDelegate(label, 3, GetPossibleOptions(calculate_label)) {}
+
+  static constexpr const char *kNo = "No";
+  static constexpr const char *kYes = "Yes";
+
+  std::vector GetPossibleOptions(const char *calculate_label) {
+std::vector options;
+options.push_back(calculate_label);
+options.push_back(kYes);
+options.push_back(kNo);
+return options;
+  }
+
+  LazyBool GetLazyBoolean() {
+std::string choice = GetChoiceContent();
+if (choice == kNo)
+  return eLazyBoolNo;
+else if (choice == kYes)
+  return eLazyBoolYes;
+else
+  return eLazyBoolCalculate;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1909,6 +1945,29 @@
   SelectionType m_selection_type;
 };
 
+class ArgumentsFieldDelegate : public ListFieldDelegate {
+public:
+  ArgumentsFieldDelegate()
+  : ListFieldDelegate("Arguments",
+  TextFieldDelegate("Argument", "", false)) {}
+
+  Args GetArguments() {
+Args arguments;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  arguments.AppendArgument(GetField(i).GetText());
+}
+return arguments;
+  }
+
+  void AddArguments(const Args ) {
+for (size_t i = 0; i < arguments.GetArgumentCount(); i++) {
+  AddNewField();
+  TextFieldDelegate  = GetField(GetNumberOfFields() - 1);
+  field.SetText(arguments.GetArgumentAtIndex(i));
+}
+  }
+};
+
 template 
 class MappingFieldDelegate : public FieldDelegate {
 public:
@@ -2069,14 +2128,36 @@
   const std::string () { return GetKeyField().GetName(); }
 
   const std::string () { return GetValueField().GetText(); }
+
+  void SetName(const char *name) { return GetKeyField().SetText(name); }
+
+  void SetValue(const char *value) { return GetValueField().SetText(value); }
 };
 
 class EnvironmentVariableListFieldDelegate
 : public ListFieldDelegate {
 public:
-

[Lldb-commits] [PATCH] D107079: [lldb] Add an option for emitting LLDB logs as JSON

2021-08-12 Thread Raphael Isemann via Phabricator via lldb-commits

teemperor updated this revision to Diff 365944.
teemperor added a comment.

- Fix double printing and formatting of the old plain text format.


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

https://reviews.llvm.org/D107079

Files:
  lldb/docs/design/logging.rst
  lldb/docs/index.rst
  lldb/include/lldb/Interpreter/CommandCompletions.h
  lldb/include/lldb/Utility/Log.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Commands/CommandObjectLog.cpp
  lldb/source/Commands/Options.td
  lldb/source/Interpreter/CommandObject.cpp
  lldb/source/Utility/Log.cpp
  lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
  lldb/test/API/functionalities/completion/TestCompletion.py
  lldb/test/API/functionalities/logging/Makefile
  lldb/test/API/functionalities/logging/TestLogging.py
  lldb/test/API/functionalities/logging/main.cpp
  lldb/unittests/Utility/LogTest.cpp

Index: lldb/unittests/Utility/LogTest.cpp
===
--- lldb/unittests/Utility/LogTest.cpp
+++ lldb/unittests/Utility/LogTest.cpp
@@ -11,6 +11,7 @@
 
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Threading.h"
 #include 
@@ -82,6 +83,16 @@
   llvm::StringRef takeOutput();
   llvm::StringRef logAndTakeOutput(llvm::StringRef Message);
 
+  llvm::json::Object logAndTakeLogObject(llvm::StringRef Message) {
+llvm::StringRef Out = logAndTakeOutput(Message).trim();
+EXPECT_TRUE(Out.consume_back(","));
+llvm::Expected parsed = llvm::json::parse(Out);
+EXPECT_TRUE(static_cast(parsed));
+llvm::json::Object *parsed_obj = parsed->getAsObject();
+EXPECT_NE(parsed_obj, nullptr);
+return *parsed_obj;
+  }
+
 public:
   void SetUp() override;
 };
@@ -92,6 +103,11 @@
 
   std::string error;
   ASSERT_TRUE(EnableChannel(m_stream_sp, 0, "chan", {}, error));
+  llvm::raw_string_ostream error_stream(error);
+  // Use JSON logging by default so the log messages can be parsed.
+  ASSERT_TRUE(
+  Log::SetLogChannelFormat("chan", Log::OutputFormat::JSON, error_stream));
+  EXPECT_EQ(error, "");
 
   m_log = test_channel.GetLogIfAll(FOO);
   ASSERT_NE(nullptr, m_log);
@@ -211,37 +227,71 @@
 
 TEST_F(LogChannelEnabledTest, log_options) {
   std::string Err;
-  EXPECT_EQ("Hello World\n", logAndTakeOutput("Hello World"));
+  EXPECT_EQ("{\"message\":\"Hello World\"},\n",
+logAndTakeOutput("Hello World"));
   EXPECT_TRUE(EnableChannel(getStream(), LLDB_LOG_OPTION_THREADSAFE, "chan", {},
 Err));
-  EXPECT_EQ("Hello World\n", logAndTakeOutput("Hello World"));
+  EXPECT_EQ("{\"message\":\"Hello World\"},\n",
+logAndTakeOutput("Hello World"));
 
   {
 EXPECT_TRUE(EnableChannel(getStream(), LLDB_LOG_OPTION_PREPEND_SEQUENCE,
   "chan", {}, Err));
-llvm::StringRef Msg = logAndTakeOutput("Hello World");
-int seq_no;
-EXPECT_EQ(1, sscanf(Msg.str().c_str(), "%d Hello World", _no));
+llvm::json::Object parsed_obj = logAndTakeLogObject("Hello World");
+// Check that any sequence-id was added.
+EXPECT_TRUE(parsed_obj.getInteger("sequence-id"));
   }
 
   {
 EXPECT_TRUE(EnableChannel(getStream(), LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION,
   "chan", {}, Err));
-llvm::StringRef Msg = logAndTakeOutput("Hello World");
-char File[12];
-char Function[17];
-  
-sscanf(Msg.str().c_str(), "%[^:]:%s Hello World", File, Function);
-EXPECT_STRCASEEQ("LogTest.cpp", File);
-EXPECT_STREQ("logAndTakeOutput", Function);
+llvm::json::Object parsed_obj = logAndTakeLogObject("Hello World");
+// Check that function/file parameters were added.
+llvm::Optional function = parsed_obj.getString("function");
+llvm::Optional file = parsed_obj.getString("file");
+EXPECT_TRUE(function.hasValue());
+EXPECT_TRUE(file.hasValue());
+
+EXPECT_EQ(*function, "logAndTakeOutput");
+EXPECT_EQ(*file, "LogTest.cpp");
   }
 
-  EXPECT_TRUE(EnableChannel(
-  getStream(), LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD, "chan", {}, Err));
-  EXPECT_EQ(llvm::formatv("[{0,0+4}/{1,0+4}] Hello World\n", ::getpid(),
-  llvm::get_threadid())
-.str(),
-logAndTakeOutput("Hello World"));
+  {
+EXPECT_TRUE(EnableChannel(
+getStream(), LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD, "chan", {}, Err));
+llvm::json::Object parsed_obj = logAndTakeLogObject("Hello World");
+
+// Check that function/file parameters were added.
+llvm::Optional pid = parsed_obj.getInteger("pid");
+llvm::Optional tid = parsed_obj.getInteger("tid");
+EXPECT_TRUE(pid.hasValue());
+EXPECT_TRUE(tid.hasValue());
+
+EXPECT_EQ(*pid, static_cast(::getpid()));
+EXPECT_EQ(*tid,

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

2021-08-12 Thread Omar Emara via Phabricator via lldb-commits

OmarEmaraDev added a comment.

Adding a boolean for inheriting the environment may not be necessary, we can 
just add two environment fields, one for inherited and one for user specified. 
The inherited one will be put in advanced settings with another boolean that 
show or hide the field. Both will be filled with the appropriate default 
values. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

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

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

2021-08-12 Thread Omar Emara via Phabricator via lldb-commits

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3223
+
+const Environment _environment = target->GetEnvironment();
+m_enviroment_field->AddEnvironmentVariables(target_environment);

clayborg wrote:
> Does this currently get all target env vars including the inherited ones?
Yes. It seems the logic in `TargetProperties::ComputeEnvironment` adds the 
inherited environment, erase the unset environment, and finally add the target 
environment. If we want the target environment only, we can add another method 
to `TargetProperties` to get only those, or we could  erase the platform 
environment elements from the computed environment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

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

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

2021-08-12 Thread David Spickett via Phabricator via lldb-commits

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213

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

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet [WIP]

2021-08-12 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 365927.
mgorny edited the summary of this revision.
mgorny added a comment.

Include server tests.


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

https://reviews.llvm.org/D107931

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -10,6 +10,9 @@
 the initial set of tests implemented.
 """
 
+import binascii
+import itertools
+
 import unittest2
 import gdbremote_testcase
 import lldbgdbserverutils
@@ -1246,3 +1249,59 @@
 # Make sure we read back what we wrote.
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
+
+def test_launch_via_A(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+# NB: strictly speaking we should use %x here but this packet
+# is deprecated, so no point in changing lldb-server's expectations
+self.test_sequence.add_log_lines(
+["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
+ tuple(itertools.chain.from_iterable(
+ [(len(x), x) for x in hex_args])),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+self.assertEqual(context["O_content"],
+ b'arg1\r\narg2\r\narg3\r\n')
+
+def test_launch_via_vRun(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+context = self.expect_gdbremote_sequence()
+
+def test_launch_via_vRun_no_args(self):
+self.build()
+exe_path = self.getBuildArtifact("a.out")
+hex_path = binascii.b2a_hex(exe_path.encode()).decode()
+
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vRun;%s#00" % (hex_path,),
+ "send packet: $OK#00",
+ "read packet: $C00#00",
+ "send packet: $W00#00"],
+True)
+self.expect_gdbremote_sequence()
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -363,6 +363,8 @@
 return eServerPacketType_vCont;
   if (PACKET_MATCHES("vCont?"))
 return eServerPacketType_vCont_actions;
+  if (PACKET_STARTS_WITH("vRun;"))
+return eServerPacketType_vRun;
 }
 break;
   case '_':
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
@@ -182,6 +182,9 @@
   RegisterMemberFunctionHandler(
   StringExtractorGDBRemote::eServerPacketType_vCont_actions,
   ::Handle_vCont_actions);
+  RegisterMemberFunctionHandler(
+  StringExtractorGDBRemote::eServerPacketType_vRun,
+  ::Handle_vRun);
   RegisterMemberFunctionHandler(
   StringExtractorGDBRemote::eServerPacketType_x,
   ::Handle_memory_read);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

2021-08-12 Thread Jason Molenda via Phabricator via lldb-commits

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

Address David's second review comments -- explained the unusual formatting of 
the main.c in the test case, change the m_adrp_insn to be an Optional instead 
of using 0 as a "not-set" value.  I'll prob land this tomorrow unless someone 
has additional feedback, I think it's in reasonable shape.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
  
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64.yaml
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
  lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c

Index: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c
@@ -0,0 +1,110 @@
+#include 
+
+// For the test case, we really want the the layout of this binary
+// to be:
+//
+//   foo()
+//   bar() - 4096 bytes of nop's
+//   main()
+//   "HI" string
+//
+// in reality getting this layout from the compiler and linker
+// is a crapshoot, so I have yaml's checked in of the correct
+// layout.  Recompiling from source may not get the needed
+// binary layout.
+
+static int bar();
+static int foo() { return 5 + bar(); }
+// A function of 4096 bytes, so when main() loads the
+// address of foo() before this one, it has to subtract
+// a 4096 page.
+#define SIXTY_FOUR_BYTES_NOP   \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");  \
+  asm("nop");
+
+static int bar() {
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  SIXTY_FOUR_BYTES_NOP;
+  return 5;
+}
+int main() {
+  int (*f)(void) = foo;
+  puts("HI");
+  return f();
+}
Index: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/a.out-arm64_32.yaml
===
--- /dev/null
+++

Re: [Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D107669: [trace] [intel pt] Create a "process trace save" command

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

[Lldb-commits] [lldb] 7150b56 - Symbolicate aarch64 adrp+add pc-relative addr in disass

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet

[Lldb-commits] [PATCH] D107470: 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor

[Lldb-commits] [PATCH] D107470: 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor

[Lldb-commits] [PATCH] D107704: [LLDB][NFC] Simplify IOHandler's getLine to avoid strange casts and redundant checks

[Lldb-commits] [PATCH] D107704: [LLDB][NFC] Simplify IOHandler's getLine to avoid strange casts and redundant checks

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Lldb-commits] [PATCH] D107079: [lldb] Add an option for emitting LLDB logs as JSON

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

[Lldb-commits] [PATCH] D107931: [lldb] [gdb-remote] Implement vRun packet [WIP]

[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

23 matches

Site Navigation

Mail list logo

Footer information