[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
anton.kolesov added a project: LLDB.
Herald added subscribers: lldb-commits, abidh, ki.stfu.

Simple "file" might not be enough for Eclipse CDT to find the file,
therefore it is preferable for -data-disassemble response to include full
absolute file path in the "fullname" field, similarly to how it is
implemented in the CMICmnLLDBDebugSessionInfo::GetFrameInfo() function.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59015

Files:
  lldb/tools/lldb-mi/MICmdCmdData.cpp


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  static char pPathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pPathBuffer, sizeof(pPathBuffer));
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  static char pPathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pPathBuffer, sizeof(pPathBuffer));
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 193863.
anton.kolesov added a comment.

Added a simple test case.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
  lldb/tools/lldb-mi/MICmdCmdData.cpp


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  static char pPathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pPathBuffer, sizeof(pPathBuffer));
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 
"\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"},"
 %
 addr)
 
+# Test -data-disassemble with source line information
+self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 
0x10))
+self.expect(
+'\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+
'line_asm_insn=\[{address="0x0*%x",func-name="main",offset="0",size="[1-9]+",inst=".+?"}\],'
+'fullname="%s"}' %
+(addr, os.path.abspath("main.cpp")) )
+
 # Run to hello_world
 self.runCmd("-break-insert -f hello_world")
 self.expect("\^done,bkpt={number=\"2\"")


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  static char pPathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pPathBuffer, sizeof(pPathBuffer));
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 "\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," %
 addr)
 
+# Test -data-disassemble with source line information
+self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 0x10))
+self.expect(
+'\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+'line_asm_i

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 194376.
anton.kolesov added a comment.

Replaced static local variable (as can be found in 
CMICmnLLDBDebugSessionInfo::GetFrameInfo) with a unique_ptr to the char array 
(as done in CMICmnLLDBDebuggerHandleEvents::MiHelpGetModuleInfo).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
  lldb/tools/lldb-mi/MICmdCmdData.cpp


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  std::unique_ptr pPathBuffer(new char[PATH_MAX]);
+  lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer.get());
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 
"\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"},"
 %
 addr)
 
+# Test -data-disassemble with source line information
+self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 
0x10))
+self.expect(
+'\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+
'line_asm_insn=\[{address="0x0*%x",func-name="main",offset="0",size="[1-9]+",inst=".+?"}\],'
+'fullname="%s"}' %
+(addr, os.path.abspath("main.cpp")) )
+
 # Run to hello_world
 self.runCmd("-break-insert -f hello_world")
 self.expect("\^done,bkpt={number=\"2\"")


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -415,8 +415,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  std::unique_ptr pPathBuffer(new char[PATH_MAX]);
+  lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -427,6 +431,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pPathBuffer.get());
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 "\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," %
 addr)
 
+# Test -data-disassemble with source line information
+   

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.



Comment at: lldb/tools/lldb-mi/MICmdCmdData.cpp:420
+  std::unique_ptr pPathBuffer(new char[PATH_MAX]);
+  lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);
 

clayborg wrote:
> There is a variant of FileSpec::GetPath() that returns a std::string:
> ```
> std::string path = lineEntry.GetFileSpec().GetPath();
> ```
> Then you can get rid of the code above:
> ```
> std::unique_ptr pPathBuffer(new char[PATH_MAX]);
> ```
LLDB MI uses SBFileSpec rather then FileSpec, and SBFileSpec doesn't have such 
a function.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015



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

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.



Comment at: lldb/tools/lldb-mi/MICmdCmdData.cpp:419
+  // Get a full path to the file.
+  std::unique_ptr pPathBuffer(new char[PATH_MAX]);
+  lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);

clayborg wrote:
> Confused as to why we are calling malloc and free here for pPathBuffer? Why 
> not just:
> ```
> char pPathBuffer[PATH_MAX];
> ```
I don't have a strong opinion on this, so to maintain consistency in the code 
I'm trying to use what other code in lldb-mi uses in similar situations - which 
is either unique_ptr or static local variable, but I presume it was decided 
that second approach is not good. FWIW, If I were to write code without regard 
of what is being done in the same project, then I would never ever had a 
variable named "miValueConst5".


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015



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

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 243828.
anton.kolesov retitled this revision from "[lldb] Set executable module when 
adding modules to the Target" to "[lldb-vscode] Ensure that target matches the 
executable file".
anton.kolesov edited the summary of this revision.
anton.kolesov added a comment.

Reverted to the original idea of modifying lldb-vscode. Unlike first version, 
this commit also modifies request_attach to have the same behaviour. Two new 
properties are added to request arguments: "targetTriple" and "platformName" to 
specify values to respective arguments of SBDebugger::CreateTarget().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847

Files:
  lldb/tools/lldb-vscode/lldb-vscode.cpp
  lldb/tools/lldb-vscode/package.json

Index: lldb/tools/lldb-vscode/package.json
===
--- lldb/tools/lldb-vscode/package.json
+++ lldb/tools/lldb-vscode/package.json
@@ -122,6 +122,14 @@
 "type": "string",
 "description": "Specify a working directory to set the debug adaptor to so relative object files can be located."
 			},
+			"targetTriple": {
+"type": "string",
+"description": "Triplet of the target architecture to override value derived from the program file."
+			},
+			"platformName": {
+"type": "string",
+"description": "Name of the execution platform to override value derived from the program file."
+			},
 			"initCommands": {
 	"type": "array",
 	"description": "Initialization commands executed upon debugger startup.",
@@ -175,6 +183,14 @@
 "type": "string",
 "description": "Specify a working directory to set the debug adaptor to so relative object files can be located."
 			},
+			"targetTriple": {
+"type": "string",
+"description": "Triplet of the target architecture to override value derived from the program file."
+			},
+			"platformName": {
+"type": "string",
+"description": "Name of the execution platform to override value derived from the program file."
+			},
 			"attachCommands": {
 "type": "array",
 "description": "Custom commands that are executed instead of attaching to a process ID or to a process by name. These commands may optionally create a new target and must perform an attach. A valid process must exist after these commands complete or the \"attach\" will fail.",
Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -110,6 +110,16 @@
   return newsockfd;
 }
 
+void AddBreakpointListener(VSCode &vscode) {
+  // Configure breakpoint event listeners for the target.
+  lldb::SBListener listener = vscode.debugger.GetListener();
+  listener.StartListeningForEvents(
+  vscode.target.GetBroadcaster(),
+  lldb::SBTarget::eBroadcastBitBreakpointChanged);
+  listener.StartListeningForEvents(vscode.broadcaster,
+   eBroadcastBitStopEventThread);
+}
+
 std::vector MakeArgv(const llvm::ArrayRef &strs) {
   // Create and return an array of "const char *", one for each C string in
   // "strs" and terminate the list with a NULL. This can be used for argument
@@ -512,27 +522,45 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
-  // Grab the name of the program we need to debug and set it as the first
-  // argument that will be passed to the program we will debug.
-  const auto program = GetString(arguments, "program");
-  if (!program.empty()) {
-lldb::SBFileSpec program_fspec(program.data(), true /*resolve_path*/);
-
-g_vsc.launch_info.SetExecutableFile(program_fspec,
-false /*add_as_first_arg*/);
-const char *target_triple = nullptr;
-const char *uuid_cstr = nullptr;
-// Stand alone debug info file if different from executable
-const char *symfile = nullptr;
-g_vsc.target.AddModule(program.data(), target_triple, uuid_cstr, symfile);
-if (error.Fail()) {
-  response["success"] = llvm::json::Value(false);
-  EmplaceSafeString(response, "message", std::string(error.GetCString()));
-  g_vsc.SendJSON(llvm::json::Value(std::move(response)));
-  return;
-}
+  // Grab the name of the program we need to debug and create a target using
+  // the given program as an argument. Executable file can be a source of target
+  // architecture and platform, if they differ from the host. Setting exe path
+  // in launch info is useless because Target.Launch() will not change
+  // architecture and platform, therefore they should be known at the target
+  // creation. We also use target triple and platform from the launch
+  // configuration, if g

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 244160.
anton.kolesov added a comment.

Updated in attempt to reduce amount of code duplication between request_attach 
and request_launch.
Built and tested with testsuite on Linux/x64, also built and manually tested on 
Windows/x64 host with a baremetal ARC cpu target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847

Files:
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp
  lldb/tools/lldb-vscode/package.json

Index: lldb/tools/lldb-vscode/package.json
===
--- lldb/tools/lldb-vscode/package.json
+++ lldb/tools/lldb-vscode/package.json
@@ -122,6 +122,14 @@
 "type": "string",
 "description": "Specify a working directory to set the debug adaptor to so relative object files can be located."
 			},
+			"targetTriple": {
+"type": "string",
+"description": "Triplet of the target architecture to override value derived from the program file."
+			},
+			"platformName": {
+"type": "string",
+"description": "Name of the execution platform to override value derived from the program file."
+			},
 			"initCommands": {
 	"type": "array",
 	"description": "Initialization commands executed upon debugger startup.",
@@ -175,6 +183,14 @@
 "type": "string",
 "description": "Specify a working directory to set the debug adaptor to so relative object files can be located."
 			},
+			"targetTriple": {
+"type": "string",
+"description": "Triplet of the target architecture to override value derived from the program file."
+			},
+			"platformName": {
+"type": "string",
+"description": "Name of the execution platform to override value derived from the program file."
+			},
 			"attachCommands": {
 "type": "array",
 "description": "Custom commands that are executed instead of attaching to a process ID or to a process by name. These commands may optionally create a new target and must perform an attach. A valid process must exist after these commands complete or the \"attach\" will fail.",
Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -69,8 +69,6 @@
 
 enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
 
-enum VSCodeBroadcasterBits { eBroadcastBitStopEventThread = 1u << 0 };
-
 SOCKET AcceptConnection(int portno) {
   // Accept a socket connection from any host on "portno".
   SOCKET newsockfd = -1;
@@ -512,25 +510,13 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
-  // Grab the name of the program we need to debug and set it as the first
-  // argument that will be passed to the program we will debug.
-  const auto program = GetString(arguments, "program");
-  if (!program.empty()) {
-lldb::SBFileSpec program_fspec(program.data(), true /*resolve_path*/);
-
-g_vsc.launch_info.SetExecutableFile(program_fspec,
-false /*add_as_first_arg*/);
-const char *target_triple = nullptr;
-const char *uuid_cstr = nullptr;
-// Stand alone debug info file if different from executable
-const char *symfile = nullptr;
-g_vsc.target.AddModule(program.data(), target_triple, uuid_cstr, symfile);
-if (error.Fail()) {
-  response["success"] = llvm::json::Value(false);
-  EmplaceSafeString(response, "message", std::string(error.GetCString()));
-  g_vsc.SendJSON(llvm::json::Value(std::move(response)));
-  return;
-}
+  lldb::SBError status;
+  g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
+  if (status.Fail()) {
+response["success"] = llvm::json::Value(false);
+EmplaceSafeString(response, "message", status.GetCString());
+g_vsc.SendJSON(llvm::json::Value(std::move(response)));
+return;
   }
 
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
@@ -543,7 +529,8 @@
 char attach_info[256];
 auto attach_info_len =
 snprintf(attach_info, sizeof(attach_info),
- "Waiting to attach to \"%s\"...", program.data());
+ "Waiting to attach to \"%s\"...",
+ g_vsc.target.GetExecutable().GetFilename());
 g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_info,
   attach_info_len));
   }
@@ -1217,13 +1204,6 @@
 g_vsc.debugger.SetErrorFileHandle(out, false);
   }
 
-  g_vsc.target = g_vsc.debugger.CreateTarget(nullptr);
-  lldb::SBListener listener = g_vsc.debugger.GetListener();
-  listener.StartListeningForEvents(
-  g_vsc.

[Lldb-commits] [PATCH] D75975: [lldb] Copy m_behaves_like_zeroth_frame on stack frame update

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Field StackFrame::m_behaves_like_zeroth_frame was introduced in commit
[1], however that commit hasn't added a copying of the field to
UpdatePreviousFrameFromCurrentFrame, therefore the value wouldn't change
when updating frames to reflect the current situation.

The particular scenario, where this matters is following. Assume we have
function `main` that invokes function `func1`. We set breakpoint at
`func1` entry and in `main` after the `func1` call, and do not stop at
the `main` entry. Therefore, when debugger stops for the first time,
`func1` is frame#0, while `main` is frame#1, thus
m_behaves_like_zeroth_frame is set to 0 for `main` frame. Execution is
resumed, and stops now in `main`, where it is now frame#0. However while
updating the frame object, m_behaves_like_zeroth_frame remains false.
This field plays an important role when calculating line information for
backtrace: for frame#0, PC is the current line, therefore line
information is retrieved for PC, however for all other frames this is
not the case - calculated PC is a return-PC, i.e. instruction after the
function call line, therefore for those frames LLDB needs to step back
by one instruction. Initial implementation did this strictly for frames
that have index != 0 (and index is updated properly in
`UpdatePreviousFrameFromCurrentFrame`), but m_behaves_like_zeroth_frame
added a capability for middle-of-stack frames to behave in a similar
manner. But because current code now doesn't check frame idx,
m_behaves_like_zeroth_frame must be set to true for frames with 0 index,
not only for frame that behave like one. In the described test case,
after stopping in `main`, LLDB would still consider frame#0 as
non-zeroth, and would subtract instruction from the PC, and would report
previous like as current line.

The error doesn't manifest itself in LLDB interpreter though - it can be
reproduced through LLDB-MI and when using SB API, but not when we
interpreter command "continue" is executed.  Honestly, I didn't fully
understand why it works in interpreter, I did found that bug "fixes"
itself if I enable `DEBUG_STACK_FRAMES` in StackFrameList.cpp, because
that calls `StackFrame::Dump` and that calls
`GetSymbolContext(eSymbolContextEverything)`, which fills the context of
frame on the first breakpoint, therefore it doesn't have to be
recalculated (improperly) on a second frame. However, on first
breakpoint symbol context is calculated for the "call" line, not the
next one, therefore it should be recalculated anyway on a second
breakpoint, and it is done correctly, even though
m_behaves_like_zeroth_frame is still incorrect, as long as
`GetSymbolContext(eSymbolContextEverything)` has been called.

[1] 31e6dbe1c6a6 Fix PC adjustment in StackFrame::GetSymbolContext


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75975

Files:
  lldb/source/Target/StackFrame.cpp
  lldb/test/API/functionalities/unwind/zeroth_frame/Makefile
  lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
  lldb/test/API/functionalities/unwind/zeroth_frame/main.c

Index: lldb/test/API/functionalities/unwind/zeroth_frame/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/zeroth_frame/main.c
@@ -0,0 +1,8 @@
+void func_inner() {
+int a = 1;  // Set breakpoint 1 here
+}
+
+int main() {
+func_inner();
+return 0; // Set breakpoint 2 here
+}
Index: lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
@@ -0,0 +1,96 @@
+"""
+Test that line information is recalculated properly for a frame when it moves
+from the middle of the backtrace to a zero index.
+
+This is a regression test for a StackFrame bug, where whether frame is zero or
+not depends on an internal field. When LLDB was updating its frame list value
+of the field wasn't copied into existing StackFrame instances, so those
+StackFrame instances, would use an incorrect line entry evaluation logic in
+situations if it was in the middle of the stack frame list (not zeroth), and
+then moved to the top position. The difference in logic is that for zeroth
+frames line entry is returned for program counter, while for other frame
+(except for those that "behave like zeroth") it is for the instruction
+preceding PC, as PC points to the next instruction after function call. When
+the bug is present, when execution stops at the second breakpoint
+SBFrame.GetLineEntry() returns line entry for the previous line, rather than
+the one with a breakpoint. Note that this is specific to
+SBFrame.GetLineEntry(), SBFrame.GetPCAddress().GetLineEntry() would return
+correct entry.
+
+This bug doesn't reproduce through an LLDB interpretator, however it happens
+wh

[Lldb-commits] [PATCH] D75979: [lldb] Implement StackFrame::BehavesLikeZerothFrame

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Commit [1] added a declaration of function-member
StackFrame::BehavesLikeZerothFrame but hasn't added an implementation
for the function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75979

Files:
  lldb/source/Target/StackFrame.cpp


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -1211,6 +1211,10 @@
   return m_stack_frame_kind == StackFrame::Kind::Artificial;
 }
 
+bool StackFrame::BehavesLikeZerothFrame() const {
+  return m_behaves_like_zeroth_frame;
+}
+
 lldb::LanguageType StackFrame::GetLanguage() {
   CompileUnit *cu = GetSymbolContext(eSymbolContextCompUnit).comp_unit;
   if (cu)


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -1211,6 +1211,10 @@
   return m_stack_frame_kind == StackFrame::Kind::Artificial;
 }
 
+bool StackFrame::BehavesLikeZerothFrame() const {
+  return m_behaves_like_zeroth_frame;
+}
+
 lldb::LanguageType StackFrame::GetLanguage() {
   CompileUnit *cu = GetSymbolContext(eSymbolContextCompUnit).comp_unit;
   if (cu)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D75979: [lldb] Remove unimplemented StackFrame::BehavesLikeZerothFrame

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 249844.
anton.kolesov retitled this revision from "[lldb] Implement 
StackFrame::BehavesLikeZerothFrame" to "[lldb] Remove unimplemented 
StackFrame::BehavesLikeZerothFrame".
anton.kolesov edited the summary of this revision.
anton.kolesov added a comment.

Removed function declaration as requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75979

Files:
  lldb/include/lldb/Target/StackFrame.h


Index: lldb/include/lldb/Target/StackFrame.h
===
--- lldb/include/lldb/Target/StackFrame.h
+++ lldb/include/lldb/Target/StackFrame.h
@@ -367,12 +367,6 @@
   /// may have limited support for inspecting variables.
   bool IsArtificial() const;
 
-  /// Query whether this frame behaves like the zeroth frame, in the sense
-  /// that its pc value might not immediately follow a call (and thus might
-  /// be the first address of its function).  True for actual frame zero as
-  /// well as any other frame with the same trait.
-  bool BehavesLikeZerothFrame() const;
-
   /// Query this frame to find what frame it is in this Thread's
   /// StackFrameList.
   ///
@@ -517,6 +511,11 @@
   bool m_cfa_is_valid; // Does this frame have a CFA?  Different from CFA ==
// LLDB_INVALID_ADDRESS
   Kind m_stack_frame_kind;
+
+  // Whether this frame behaves like the zeroth frame, in the sense
+  // that its pc value might not immediately follow a call (and thus might
+  // be the first address of its function). True for actual frame zero as
+  // well as any other frame with the same trait.
   bool m_behaves_like_zeroth_frame;
   lldb::VariableListSP m_variable_list_sp;
   ValueObjectList m_variable_list_value_objects; // Value objects for each


Index: lldb/include/lldb/Target/StackFrame.h
===
--- lldb/include/lldb/Target/StackFrame.h
+++ lldb/include/lldb/Target/StackFrame.h
@@ -367,12 +367,6 @@
   /// may have limited support for inspecting variables.
   bool IsArtificial() const;
 
-  /// Query whether this frame behaves like the zeroth frame, in the sense
-  /// that its pc value might not immediately follow a call (and thus might
-  /// be the first address of its function).  True for actual frame zero as
-  /// well as any other frame with the same trait.
-  bool BehavesLikeZerothFrame() const;
-
   /// Query this frame to find what frame it is in this Thread's
   /// StackFrameList.
   ///
@@ -517,6 +511,11 @@
   bool m_cfa_is_valid; // Does this frame have a CFA?  Different from CFA ==
// LLDB_INVALID_ADDRESS
   Kind m_stack_frame_kind;
+
+  // Whether this frame behaves like the zeroth frame, in the sense
+  // that its pc value might not immediately follow a call (and thus might
+  // be the first address of its function). True for actual frame zero as
+  // well as any other frame with the same trait.
   bool m_behaves_like_zeroth_frame;
   lldb::VariableListSP m_variable_list_sp;
   ValueObjectList m_variable_list_value_objects; // Value objects for each
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76351: [lldb-vscode] Don't use SBLaunchInfo in request_attach

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
anton.kolesov added reviewers: labath, clayborg.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

If LLDB attaches to an already running target, then structure SBAttachInfo is
used instead of SBLaunchInfo. lldb-vscode function request_attach sets some
values to g_vsc.launch_info, however this field is then not passed anywhere, so
this action has no effect. This commit removes invocation of
SBLaunchInfo::SetDetachOnError, which has no equivalent in SBAttachInfo.

File package.json doesn't describe detachOnError property for "attach" request
type, therefore it is not needed to update it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76351

Files:
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -543,9 +543,6 @@
 return;
   }
 
-  const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
 


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -543,9 +543,6 @@
 return;
   }
 
-  const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert g_vsc.launch_info to a local variable

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
anton.kolesov added reviewers: labath, clayborg.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This field inside of the global variable can be a simple local variable because
it is used in only one function: request_launch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76593

Files:
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1381,26 +1381,26 @@
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
-g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
+launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto envs = GetStrings(arguments, "env");
   if (!envs.empty())
-g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
 
-  auto flags = g_vsc.launch_info.GetLaunchFlags();
+  auto flags = launch_info.GetLaunchFlags();
 
   if (GetBoolean(arguments, "disableASLR", true))
 flags |= lldb::eLaunchFlagDisableASLR;
@@ -1409,9 +1409,9 @@
   if (GetBoolean(arguments, "shellExpandArguments", false))
 flags |= lldb::eLaunchFlagShellExpandArguments;
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-  g_vsc.launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-   lldb::eLaunchFlagStopAtEntry);
+  launch_info.SetDetachOnError(detatchOnError);
+  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
+ lldb::eLaunchFlagStopAtEntry);
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
@@ -1419,7 +1419,7 @@
 // Disable async events so the launch will be successful when we return 
from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g_vsc.target.Launch(g_vsc.launch_info, error);
+g_vsc.target.Launch(launch_info, error);
 g_vsc.debugger.SetAsync(true);
   } else {
 g_vsc.RunLLDBCommands("Running launchCommands:", launchCommands);
Index: lldb/tools/lldb-vscode/VSCode.h
===
--- lldb/tools/lldb-vscode/VSCode.h
+++ lldb/tools/lldb-vscode/VSCode.h
@@ -71,7 +71,6 @@
   lldb::SBDebugger debugger;
   lldb::SBTarget target;
   lldb::SBAttachInfo attach_info;
-  lldb::SBLaunchInfo launch_info;
   lldb::SBValueList variables;
   lldb::SBBroadcaster broadcaster;
   int64_t num_regs;
Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -28,7 +28,7 @@
 VSCode g_vsc;
 
 VSCode::VSCode()
-: launch_info(nullptr), variables(), broadcaster("lldb-vscode"),
+: variables(), broadcaster("lldb-vscode"),
   num_regs(0), num_locals(0), num_globals(0), log(),
   exception_breakpoints(
   {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1381,26 +1381,26 @@
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
-g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
+launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto en

[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert g_vsc.launch_info to a local variable

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added a comment.

In D76593#1936337 , @labath wrote:

> Sounds like a good idea. Could you do the same for `attach_info` (it looks 
> like it should be possible)? Otherwise, one is left wondering about what's 
> the difference...


I have that as my next patch - it seems like something that should be in a 
separate commit, but in the same pull request, but I don't know how to properly 
submit patch series for review at Phabricator - only through "related 
revisions", I presume?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76593



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

[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert launch_info and attach_info to local variables

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 252503.
anton.kolesov retitled this revision from "[lldb-vscode] Convert 
g_vsc.launch_info to a local variable" to "[lldb-vscode] Convert launch_info 
and attach_info to local variables".
anton.kolesov edited the summary of this revision.
anton.kolesov added a comment.

Added attach_info conversion into the same revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76593

Files:
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -508,13 +508,14 @@
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
+  lldb::SBAttachInfo attach_info;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
   GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
   if (pid != LLDB_INVALID_PROCESS_ID)
-g_vsc.attach_info.SetProcessID(pid);
+attach_info.SetProcessID(pid);
   const auto wait_for = GetBoolean(arguments, "waitFor", false);
-  g_vsc.attach_info.SetWaitForLaunch(wait_for, false /*async*/);
+  attach_info.SetWaitForLaunch(wait_for, false /*async*/);
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
@@ -547,20 +548,20 @@
   g_vsc.RunPreRunCommands();
 
   if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
-char attach_info[256];
-auto attach_info_len =
-snprintf(attach_info, sizeof(attach_info),
+char attach_msg[256];
+auto attach_msg_len =
+snprintf(attach_msg, sizeof(attach_msg),
  "Waiting to attach to \"%s\"...",
  g_vsc.target.GetExecutable().GetFilename());
-g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_info,
-  attach_info_len));
+g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_msg,
+  attach_msg_len));
   }
   if (attachCommands.empty()) {
 // No "attachCommands", just attach normally.
 // Disable async events so the attach will be successful when we return from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g_vsc.target.Attach(g_vsc.attach_info, error);
+g_vsc.target.Attach(attach_info, error);
 // Reenable async events
 g_vsc.debugger.SetAsync(true);
   } else {
@@ -1381,26 +1382,26 @@
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
-g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
+launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto envs = GetStrings(arguments, "env");
   if (!envs.empty())
-g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
 
-  auto flags = g_vsc.launch_info.GetLaunchFlags();
+  auto flags = launch_info.GetLaunchFlags();
 
   if (GetBoolean(arguments, "disableASLR", true))
 flags |= lldb::eLaunchFlagDisableASLR;
@@ -1409,9 +1410,9 @@
   if (GetBoolean(arguments, "shellExpandArguments", false))
 flags |= lldb::eLaunchFlagShellExpandArguments;
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-  g_vsc.launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-   lldb::eLaunchFlagStopAtEntry);
+  launch_info.SetDetachOnError(detatchOnError);
+  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
+ lldb::eLaunchFlagStopAtEntry);
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
@@ -1419,7 +1420,7 @@
 // Disable async events so the launch will be successful when we return from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g_vsc.target.Launch(g_vsc.launch_info, error);
+g_vsc.t

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.



Comment at: lldb/tools/lldb-mi/MICmdCmdData.cpp:419
+  // Get a full path to the file.
+  std::unique_ptr pPathBuffer(new char[PATH_MAX]);
+  lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);

clayborg wrote:
> anton.kolesov wrote:
> > clayborg wrote:
> > > Confused as to why we are calling malloc and free here for pPathBuffer? 
> > > Why not just:
> > > ```
> > > char pPathBuffer[PATH_MAX];
> > > ```
> > I don't have a strong opinion on this, so to maintain consistency in the 
> > code I'm trying to use what other code in lldb-mi uses in similar 
> > situations - which is either unique_ptr or static local variable, but I 
> > presume it was decided that second approach is not good. FWIW, If I were to 
> > write code without regard of what is being done in the same project, then I 
> > would never ever had a variable named "miValueConst5".
> I would just use a local variable on the stack as I suggested. static 
> variables are not correct and should never be used in cases like this, so if 
> you see other errors, might be a good idea to submit a patch and improve 
> lldb-mi. I am all for consistency where it makes sense, but I am also for 
> fixing issues when we see them. 
But what is wrong with the approach of using std::unique_ptr local 
variable?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015



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

[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 197117.
anton.kolesov added a comment.

Allocate path buffer on stack instead of heap.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
  lldb/tools/lldb-mi/MICmdCmdData.cpp


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -401,8 +401,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  char pathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pathBuffer, PATH_MAX);
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -413,6 +417,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 
"\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"},"
 %
 addr)
 
+# Test -data-disassemble with source line information
+self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 
0x10))
+self.expect(
+'\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+
'line_asm_insn=\[{address="0x0*%x",func-name="main",offset="0",size="[1-9]+",inst=".+?"}\],'
+'fullname="%s"}' %
+(addr, os.path.abspath("main.cpp")) )
+
 # Run to hello_world
 self.runCmd("-break-insert -f hello_world")
 self.expect("\^done,bkpt={number=\"2\"")


Index: lldb/tools/lldb-mi/MICmdCmdData.cpp
===
--- lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -401,8 +401,12 @@
   const MIuint nLine = lineEntry.GetLine();
   const char *pFileName = lineEntry.GetFileSpec().GetFilename();
   pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+  // Get a full path to the file.
+  char pathBuffer[PATH_MAX];
+  lineEntry.GetFileSpec().GetPath(pathBuffer, PATH_MAX);
 
-  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+  // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+  // fullname=\"%s\"}"
   const CMICmnMIValueConst miValueConst(
   CMIUtilString::Format("%u", nLine));
   const CMICmnMIValueResult miValueResult("line", miValueConst);
@@ -413,6 +417,9 @@
   const CMICmnMIValueList miValueList(miValueTuple);
   const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
   miValueTuple2.Add(miValueResult3);
+  const CMICmnMIValueConst miValueConst5(pathBuffer);
+  const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+  miValueTuple2.Add(miValueResult5);
   const CMICmnMIValueResult miValueResult4("src_and_asm_line",
miValueTuple2);
   m_miValueList.Add(miValueResult4);
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -56,6 +56,14 @@
 "\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," %
 addr)
 
+# Test -data-disassemble with source line information
+self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 0x10))
+self.expect(
+'\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+'line_asm_insn=\[{address="0x0*%

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov created this revision.
anton.kolesov added a project: LLDB.
Herald added a subscriber: lldb-commits.

Different architectures may use different target processes, so default
"gdb-remote" target process created by lldb-vscode may not work for all
targets. This commit changes behaviour of the request_launch function -
first it assigns an executable file to the debugger, which may create the
new SBTarget instance with a different target process implementation. In
contrast, if executable file is specified inside of the SBLaunchInfo
structure, then this structure is passed to an already existing target and
thus can't force recreation of the target to match a desired process
implementation.

New code has a behavior similar to LLDB MI [1], where typically IDE would
specify target file with -file-exec-and-symbols, and then only do -exec-run
command that would launch the process. In lldb-vscode those two actions are
merged into one request_launch function. Similarly in the interpreter
session, used would first do "file" command, then "process launch"


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70847

Files:
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1346,28 +1346,19 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
-  // Grab the current working directory if there is one and set it in the
-  // launch info.
-  const auto cwd = GetString(arguments, "cwd");
-  if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
-
-  // Grab the name of the program we need to debug and set it as the first
-  // argument that will be passed to the program we will debug.
+  // Grab the name of the program we need to debug and create a new target 
using
+  // the given program as an argument and replace previous dummy target with a
+  // new one. New target might have a different type then the previous one,
+  // based on the ABI or architecture of the file. Settings program path in
+  // LaunchInfo is useless, because launch info will recycle existing target,
+  // which might be incorrect for the selected file.
   llvm::StringRef program = GetString(arguments, "program");
   if (!program.empty()) {
-lldb::SBFileSpec program_fspec(program.data(), true /*resolve_path*/);
-g_vsc.launch_info.SetExecutableFile(program_fspec,
-true /*add_as_first_arg*/);
-const char *target_triple = nullptr;
-const char *uuid_cstr = nullptr;
-// Stand alone debug info file if different from executable
-const char *symfile = nullptr;
-lldb::SBModule module = g_vsc.target.AddModule(
-program.data(), target_triple, uuid_cstr, symfile);
-if (!module.IsValid()) {
-  response["success"] = llvm::json::Value(false);
+// Create a target that matches the file.
+g_vsc.target = g_vsc.debugger.CreateTarget(program.data());
 
+if (!g_vsc.target.IsValid()) {
+  response["success"] = llvm::json::Value(false);
   EmplaceSafeString(
   response, "message",
   llvm::formatv("Could not load program '{0}'.", program).str());
@@ -1376,6 +1367,15 @@
 }
   }
 
+  // Instantiate a launch info instance for the target.
+  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+
+  // Grab the current working directory if there is one and set it in the
+  // launch info.
+  const auto cwd = GetString(arguments, "cwd");
+  if (!cwd.empty())
+g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1346,28 +1346,19 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
-  // Grab the current working directory if there is one and set it in the
-  // launch info.
-  const auto cwd = GetString(arguments, "cwd");
-  if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
-
-  // Grab the name of the program we need to debug and set it as the first
-  // argument that will be passed to the program we will debug.
+  // Grab the name of the program we need to debug and create a new target using
+  // the given program as an argument and replace previous dummy target with a
+  // new one. New target might have a different type then the previous one,
+  // based on the ABI or architecture of the file. Settings program path in
+  // LaunchInfo is useless, because launch info will recycle existing target,
+  // which might be incorrect for the selec

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added a comment.

In D70847#1763797 , @labath wrote:

> This needs a test case, though judging by your description it looks like 
> pretty much every test case on affected platforms (I guess, windows) should 
> hit this. Can you check if any of the existing vscode tests which are marked 
> @skipIfWindows start passing now?


Yes, that should be tested by existing launch tests already. My case is a 
proprietary Process plugin, not a Windows target and because my target is an 
embedded system with no ability to pass environemnt variables, command line 
arguments or an implementation of process exit I can't really run the existing 
tests for my target. I will try to build lldb for Windows and see if 
lldb-vscode tests will start working on it - as far as I can see all 
lldb-vscode tests are currently skipped for Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 232512.
anton.kolesov retitled this revision from "[lldb-vscode] Ensure that target 
matches the executable file" to "[lldb] Set executable module when adding 
modules to the Target".
anton.kolesov edited the summary of this revision.
anton.kolesov added a comment.

Reimplement the solution based on a comment from Greg.

Tested with testsuite on Linux for x64 - there are no changes in test results. 
Also manually tested with a proprietary process plugin - currently it is not 
possible to run existing testsuite with that plugin because it targets embedded 
systems which has no `cwd`, `exit()` or environment.

I also attemted to test this against Windows hosts and targets. The change, 
apparently, resolves that launch capability on Windows, however setting of 
breakpoints doesn't work - it is only possible to stop application on entry, 
and on resume it will run till the end. Among the existing lldb-vscode tests 
some of them depend on  unavailable on windows, others fail because 
of the breakpoints that don't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847

Files:
  lldb/source/Target/Target.cpp


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2016,12 +2016,15 @@
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.
 if (module_sp) {
+  bool isExecutable = false;
   ObjectFile *objfile = module_sp->GetObjectFile();
   if (objfile) {
 switch (objfile->GetType()) {
+case ObjectFile::eTypeExecutable: /// A normal executable
+  isExecutable = true;
+  LLVM_FALLTHROUGH;
 case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint 
of
 /// a program's execution state
-case ObjectFile::eTypeExecutable:/// A normal executable
 case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
  /// executable
 case ObjectFile::eTypeObjectFile:/// An intermediate object file
@@ -2084,6 +2087,16 @@
 } else {
   m_images.Append(module_sp, notify);
 }
+
+// Ensure that architecture of the Target matches that of the
+// executable file. Otherwise Target might use a "default" platform
+// that can't actually debug the executable. For example, if the Target
+// is created and by default assumes that it should use "gdb-remote"
+// process, however executable has an architecture that requires a
+// different Process class - without explicitly set executable module
+// Target would attempt to use "gdb-remote" created initially.
+if (isExecutable)
+  SetExecutableModule(module_sp, eLoadDependentsNo);
   } else
 module_sp.reset();
 }


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2016,12 +2016,15 @@
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.
 if (module_sp) {
+  bool isExecutable = false;
   ObjectFile *objfile = module_sp->GetObjectFile();
   if (objfile) {
 switch (objfile->GetType()) {
+case ObjectFile::eTypeExecutable: /// A normal executable
+  isExecutable = true;
+  LLVM_FALLTHROUGH;
 case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint of
 /// a program's execution state
-case ObjectFile::eTypeExecutable:/// A normal executable
 case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
  /// executable
 case ObjectFile::eTypeObjectFile:/// An intermediate object file
@@ -2084,6 +2087,16 @@
 } else {
   m_images.Append(module_sp, notify);
 }
+
+// Ensure that architecture of the Target matches that of the
+// executable file. Otherwise Target might use a "default" platform
+// that can't actually debug the executable. For example, if the Target
+// is created and by default assumes that it should use "gdb-remote"
+// process, however executable has an architecture that requires a
+// different Process class - without explicitly set executable module
+// Target would attempt to use "gdb-remote" created initially.
+if (isExecutable)
+  SetExecutableModule(module_sp, eLoadDependentsNo);
   } else
 module_sp.reset();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 240533.
anton.kolesov added a comment.

Added a testcase. Because target's architecture is not directly exposed through 
an API, test looks at the target triplet - it is empty for targets created 
without an exe file. Without the patch, triplet remains unchanged after adding 
an executable, but with the patch, it changes to the architecture of the 
executable file, whichever it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847

Files:
  lldb/packages/Python/lldbsuite/test/commands/target/set-exec/Makefile
  
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py
  lldb/packages/Python/lldbsuite/test/commands/target/set-exec/main.c
  lldb/source/Target/Target.cpp


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2016,12 +2016,15 @@
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.
 if (module_sp) {
+  bool isExecutable = false;
   ObjectFile *objfile = module_sp->GetObjectFile();
   if (objfile) {
 switch (objfile->GetType()) {
+case ObjectFile::eTypeExecutable: /// A normal executable
+  isExecutable = true;
+  LLVM_FALLTHROUGH;
 case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint 
of
 /// a program's execution state
-case ObjectFile::eTypeExecutable:/// A normal executable
 case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
  /// executable
 case ObjectFile::eTypeObjectFile:/// An intermediate object file
@@ -2084,6 +2087,16 @@
 } else {
   m_images.Append(module_sp, notify);
 }
+
+// Ensure that architecture of the Target matches that of the
+// executable file. Otherwise Target might use a "default" platform
+// that can't actually debug the executable. For example, if the Target
+// is created and by default assumes that it should use "gdb-remote"
+// process, however executable has an architecture that requires a
+// different Process class - without explicitly set executable module
+// Target would attempt to use "gdb-remote" created initially.
+if (isExecutable)
+  SetExecutableModule(module_sp, eLoadDependentsNo);
   } else
 module_sp.reset();
 }
Index: lldb/packages/Python/lldbsuite/test/commands/target/set-exec/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/target/set-exec/main.c
@@ -0,0 +1,3 @@
+int main() {
+  return 0;
+}
Index: 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py
===
--- /dev/null
+++ 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py
@@ -0,0 +1,26 @@
+"""
+Test that the first module of the target is set as an executable.
+"""
+
+from __future__ import print_function
+
+from lldbsuite.test.lldbtest import *
+
+
+class SetExecutableTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+"""Test adding images to the target."""
+self.build()
+
+# Create an empty target - it will have an empty triplet.
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+self.assertEqual(len(target.GetTriple()), 0)
+
+# Add a first module, which is treated as an executuble, so it's
+# architecture will change the architecture of the target.
+target.AddModule(self.getBuildArtifact("a.out"), None, None)
+self.assertNotEqual(len(target.GetTriple()), 0)
Index: lldb/packages/Python/lldbsuite/test/commands/target/set-exec/Makefile
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/target/set-exec/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2016,12 +2016,15 @@
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.
 if (module_sp) {
+  bool isExecutable = false;
   ObjectFile *objfile = module_sp->GetObjectFile();
   if (objfile) {
 switch (objfile->GetType()) {
+case ObjectFile::eTypeExecutable: /// A normal executable
+  isExecutable = true;
+  LLVM_FALLTHROUGH;
 case ObjectFile::eTypeCoreFile: /// A core f

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:16
+"""Test adding images to the target."""
+self.build()
+

clayborg wrote:
> Create a yaml file and put it in the same directory as this file. Change this 
> line to be:
> ```
> src_dir = self.getSourceDir()
> yaml_path = os.path.join(src_dir, "a.yaml")
> obj_path = self.getBuildArtifact("a.out")
> self.yaml2obj(yaml_path, obj_path)
> ```
> Make an ELF file that that is for a remote platform where the triple and 
> platform won't match typical hosts.
Do you have any specific suggestions that also have freely available toolchain? 
I have immediate access only to Windows/Linux x86 and Synopsys ARC, and ARC elf 
files don't change the platform - it remains as "host" even when connecting to 
a GDB-server, plus obj2yaml segfaults on ARC elf files.



Comment at: 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:21
+self.assertTrue(target, VALID_TARGET)
+self.assertEqual(len(target.GetTriple()), 0)
+

clayborg wrote:
> Don't know if I would run this assert here. I could see a target that is 
> created with nothing possibly using the host architecture and host platform 
> by default. So maybe remote this assert where the target triple is not set.
As far as I can understand from the `TargetList::CreateTargetInternal`, new 
target will not inherit architecture from the platform when exe file is not 
specified and neither architecture nor platform are given explicitly. So it 
seems that assuming that triplet will be empty is safe.



Comment at: 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:26
+target.AddModule(self.getBuildArtifact("a.out"), None, None)
+self.assertNotEqual(len(target.GetTriple()), 0)

clayborg wrote:
> Check the exact triple of the remote executable you end up adding.
> 
> Also add a check that the platform gets updated. Platforms must be compatible 
> with the architecture of the target's main executable, so it is good to 
> verify that the platform gets updated as well. Something like:
> 
> ```
> platform = target.GetPlatform()
> set.assertEqual(platform.GetName(), "remote-linux")
> ```
> 
> This will make this test complete. If the platform isn't right we will need 
> to fix this as well.
The platform doesn't change with the current code. 
`Target::SetExecutableModule` does direct assignment of architecture and 
doesn't touch the platform, so this is needed:

```
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1405,7 +1405,7 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
 // If we haven't set an architecture yet, reset our architecture based on
 // what we found in the executable module.
 if (!m_arch.GetSpec().IsValid()) {
-  m_arch = executable_sp->GetArchitecture();
+  SetArchitecture(executable_sp->GetArchitecture(), true);
   LLDB_LOG(log,
"setting architecture to {0} ({1}) based on executable file",
m_arch.GetSpec().GetArchitectureName(),
```

It works for me in the test case, but I wonder if `SetArchitecture` wasn't used 
there in the first place for a good reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:16
+"""Test adding images to the target."""
+self.build()
+

labath wrote:
> anton.kolesov wrote:
> > clayborg wrote:
> > > Create a yaml file and put it in the same directory as this file. Change 
> > > this line to be:
> > > ```
> > > src_dir = self.getSourceDir()
> > > yaml_path = os.path.join(src_dir, "a.yaml")
> > > obj_path = self.getBuildArtifact("a.out")
> > > self.yaml2obj(yaml_path, obj_path)
> > > ```
> > > Make an ELF file that that is for a remote platform where the triple and 
> > > platform won't match typical hosts.
> > Do you have any specific suggestions that also have freely available 
> > toolchain? I have immediate access only to Windows/Linux x86 and Synopsys 
> > ARC, and ARC elf files don't change the platform - it remains as "host" 
> > even when connecting to a GDB-server, plus obj2yaml segfaults on ARC elf 
> > files.
> Would this do the job?
> ```
> --- !ELF
> FileHeader:
>   Class:   ELFCLASS64
>   Data:ELFDATA2LSB
>   Type:ET_DYN
>   Machine: EM_AARCH64
>   Entry:   0x06C0
> Sections:
>   - Name:.text
> Type:SHT_PROGBITS
> Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
> Address: 0x06C0
> AddressAlign:0x0008
> Content: DEADBEEF
> ...
> ```
It is definitely needed to change ET_DYN to ET_EXEC in this yaml, because the 
patch specifically modifies behavior only for executables. With that change 
this works on my x86_64/Linux, but I don't known how it will behave if test 
would be run on aarch64 host - will platform still change to "remote-linux" for 
this ELF or will it remain at "host"? I'm not sure how this process of platform 
selection works - can it distinguish between elf for aarch64-linux and elf for 
baremetal aarch64-elf?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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

[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added a comment.

My original patch was changing lldb-vscode - it was creating a new Target 
object with ELF given to a constructor. That patch, though definitely would 
need an update - it should delete the original empty target after creating the 
new one, but it was leaving it alive. To me this solution looks preferable, 
because I'm a fanboy for immutable objects in general, and in this specific 
case I don't fully understand all of the potential side effects of the 
SetExecutable call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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