break-insert for lldb-mi no longer requires 'on'
puremourning opened this issue · 15 comments
In this line, we have a comment that break-insert requires on for lldb-mi:
This is no longer true (presumably, it has been fixed upstream in lldb-mi): https://github.com/lldb-tools/lldb-mi (maybe this change: lldb-tools/lldb-mi@e448e35#diff-16ecaed41b16ef0b1fd18b3f74b1a2f2).
I think fixing this will allow us to fix debugging on macOS using vscode-cpptools: microsoft/vscode-cpptools#3829
Taking the latest version of lldb-mi from GitHub, and building it:
- Including
-break-insert -f on maindoes not work:
lldbmitest) BenMBP:attach ben$ ../../build/src/lldb-mi
(gdb)
-file-exec-and-symbols ../testdata/simple/simple
^done
(gdb)
=library-loaded,id="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",target-name="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",host-name="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",symbols-loaded="1",symbols-path="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple.dSYM/Contents/Resources/DWARF/simple",loaded_addr="-",size="4096"
-break-insert -f on main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0xffffffffffffffff",func="??",file="??",fullname="??/??",line="0",pending=["on"],times="0",original-location="on"}
(gdb)
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0xffffffffffffffff",func="??",file="??",fullname="??/??",line="0",pending=["on"],times="0",original-location="on"}
(gdb)
As you can see, the breakpoint is pending and has no file/line info.
- Including
-break-insert -f mainworks:
(lldbmitest) BenMBP:attach ben$ ../../build/src/lldb-mi
(gdb)
-file-exec-and-symbols ../testdata/simple/simple
^done
(gdb)
=library-loaded,id="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",target-name="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",host-name="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple",symbols-loaded="1",symbols-path="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple.dSYM/Contents/Resources/DWARF/simple",loaded_addr="-",size="4096"
-break-insert -f main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000100000f26",func="main",file="simple.cpp",fullname="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple.cpp",line="15",pending=["main"],times="0",original-location="main"}
(gdb)
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000100000f26",func="main",file="simple.cpp",fullname="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple.cpp",line="15",pending=["main"],times="0",original-location="main"}
(gdb)
as you can see, this works and resolves the breakpoint, and we can see that the breakpoint is actually triggered, hence this lldb-mi could be used with MIEngine (via vscode-cpptools) to fix the macOS breakpoints-not-triggering issue (referenced above):
(gdb)
-exec-run
<snip uninteresting stuff>
(gdb)
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={level="0",addr="0x0000000100000f26",func="main",args=[{name="argc",value="1"}],file="simple.cpp",fullname="/Users/ben/Development/llvm-project/lldb-mi/test/testdata/simple/simple.cpp",line="15"},thread-id="1",stopped-threads="all"
(gdb)
I would send a PR to remove this lldb-mi workaround, but I can't actually build this project as I don't have Windows.
@puremourning Thank you for looking into this!
Would you be willing to try out a patched MIEngine with your lldb-mi? The difference is that 870 to 874 have been removed. I am currently kicking off a release build of that branch.
Sure thing! If you drop me a link to the build artefacts (presumably I can get them from azure?) I'll drop them in an re-test.
I actually bought a copy of windows, installed it in a VM and built MIEngine myself. Setting miDebuggerPath to point at a newly build lldb-mi and copying the built binaries to the Mac and firing up vimspector...
Success! Breakpoint hit!
there's a secondary problem that extrenalConsole no longer works properly. But I think that just needs an environment variable set. I'll look into that.
here's the current diff:
--- a/src/MICore/CommandFactories/lldb.cs
+++ b/src/MICore/CommandFactories/lldb.cs
@@ -46,29 +46,6 @@ namespace MICore
}
}
- protected override StringBuilder BuildBreakInsert(string condition, bool enabled)
- {
- // LLDB's use of the pending flag requires an optional parameter or else it fails.
- // We will use "on" for now.
- // TODO: Fix this on LLDB-MI's side
- string pendingFlag = "-f on ";
-
- StringBuilder cmd = new StringBuilder("-break-insert ");
- cmd.Append(pendingFlag);
-
- if (condition != null)
- {
- cmd.Append("-c \"");
- cmd.Append(condition);
- cmd.Append("\" ");
- }
- if (!enabled)
- {
- cmd.Append("-d ");
- }
- return cmd;
- }
-
public override async Task<Results> VarCreate(string expression, int threadId, uint frameLevel, enum_EVALFLAGS dwFlags, ResultClass resultClass = ResultClass.done)
{
string quoteEscapedExpression = EscapeQuotes(expression);
diff --git a/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs b/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs
index a3747cb..d46e510 100755
--- a/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs
+++ b/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs
@@ -866,13 +866,7 @@ namespace Microsoft.MIDebugEngine
private string GetBreakInsertMainCommand()
{
// Allow main breakpoint to be pending.
- string breakInsertMainFormat = "-break-insert -f{0} main";
- if (this.MICommandFactory.Mode == MIMode.Lldb)
- {
- // break-insert -f requires 'on' in lldb scenario
- return string.Format(CultureInfo.InvariantCulture, breakInsertMainFormat, " on");
- }
- return string.Format(CultureInfo.InvariantCulture, breakInsertMainFormat, string.Empty);
+ return "-break-insert -f main";
}
private void CheckCygwin(List<LaunchCommand> commands, LocalLaunchOptions localLaunchOptions)It may take a while before we can ship a Microsoft Signed LLDB-MI. But if users are willing to build and use their own as a workaround, we can take this change and ship a new MIEngine.
However, we will need to have both -break-insert since we may still have users using the current lldb-mi that is shipped on older versions of MacOS. So if we want this change, MIEngine should send -break-insert -f on main and if that fails, we can send -break-insert -f main
The challenge is detecting "failure". When we send break-insert -f on main in the fixed lldb, that's actually just trying to create a breakpoint at the function on, which might exist.
This is absolutely wonderful that after a whole year of this issue festering, you two wizards are finally peeling it open.
Why not detect the version of LLDB-mi at the -break-insert -f main callsite?
- If version(LLDB-mi-being-used) <= version(current-LLDB-mi-release) use
-break-insert -f on main - Else use
-break-insert -f main
I apologise if this is an ignorant suggestion.
@p-i- Thank you for your suggestion!
Looks like version in the latest lldb-mi returns a correctly formattted MI response while in the 3.5 version it does not.
LLDB-MI 3.5:
(gdb)
version
lldb-350.99.0
^done
latest LLDB-MI:
(gdb)
version
~"lldb version 8.0.1\n"
^done
On the face of it, I guess we can work with that. If the version response is garbage, assume it’s old lldb-mi and send the ‘compatible’ break-insert request. Then again it’s actually the lldb-mi version not the lldb version that we care about. At least since the split.
Actually, checking the version may not be needed.
I think attempting -break-insert -f main first and then sending -break-insert -f on main if it fails works.
On lldb-mi 3.5, you get an Unrecognized command for -break-insert -f main.
there's a secondary problem that extrenalConsole no longer works properly. But I think that just needs an environment variable set. I'll look into that.
I think the issue is this:
MIEngine sends -gdb-set new-console on and currently lldb-mi does not handle that command. We have a change I will push upstream.
I'll edit this comment with the diff's needed.
Yeah I noticed that. It’s simple enough to add that settting, but the Gdb docs say that new-console is only supported on windows And the tty command is supposed to be used elsewhere. Though that’s unsatisfactory too.
You can Actually just set LLDB_LAUNCH_FLAG_LAUNCH_IN_TTY environment car instead which works. maybe MIEngine should just set it when ExternalConsole is true.
@WardenGnaw I tried testing your fixLLDB branch, it doesn't work - it just hangs. I think this is because it sends the command too early, or it never reads the command result ? Not sure.
Here's the output:
1: (124) LaunchOptions<LocalLaunchOptions xmlns='http://schemas.microsoft.com/vstudio/MDDDebuggerOptions/2014'
1: (146) LaunchOptions ExePath='/Users/ben/Development/vim/src/vim'
1: (147) LaunchOptions WorkingDirectory='/Users/ben/Development/vim'
1: (147) LaunchOptions ExeArguments='-Nu NONE'
1: (147) LaunchOptions MIMode='lldb'
1: (147) LaunchOptions MIDebuggerPath='/Users/ben/Development/llvm-project/lldb-mi/build/src/lldb-mi'
1: (147) LaunchOptions WaitDynamicLibLoad='false'
1: (147) LaunchOptions ExternalConsole='true'
1: (147) LaunchOptions>
1: (147) LaunchOptions <Environment>
1: (147) LaunchOptions <EnvironmentEntry Name='VIMRUNTIME' Value='/Users/ben/Development/vim/runtime' />
1: (147) LaunchOptions </Environment>
1: (147) LaunchOptions</LocalLaunchOptions>
1: (233) Starting: "/Users/ben/Development/llvm-project/lldb-mi/build/src/lldb-mi" --interpreter=mi
1: (255) DebuggerPid=98248
1: (758) ->(gdb)
1: (772) <-1001-break-insert -f main
1: (773) ->1001^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0xffffffffffffffff",func="??",file="??",fullname="??/??",line="0",pending=["main"],times="0",original-location="main"}
1: (774) ->(gdb)
... no further....
Looks to me like it's never returning from RequiresOnKeywordForBreakInsert()
Here's my quick implementation of the new-console command for the record:
commit d48fc781fabff4bc1b60256673c6ed072814b78c (HEAD -> master, pm/master)
Author: Ben Jackson <puremourning@gmail.com>
Date: Sat Jan 4 14:37:46 2020 +0000
Support new-console GDB command.
This is a simple way to make applications launch in a new Terminal on
macOS. There's actually a LLDB launch flag lldb:eLaunchFlagLaunchInTTY,
but it wasn't obvious to me how/where to store launcg options in
lldb-mi, so setting the environment variable works just as well.
diff --git a/src/MICmdCmdGdbSet.cpp b/src/MICmdCmdGdbSet.cpp
index 162e3d5..9330a91 100644
--- a/src/MICmdCmdGdbSet.cpp
+++ b/src/MICmdCmdGdbSet.cpp
@@ -16,6 +16,7 @@
#include "MICmnLLDBDebugSessionInfo.h"
#include "MICmnMIResultRecord.h"
#include "MICmnMIValueConst.h"
+#include <cstdlib>
// Instantiations:
const CMICmdCmdGdbSet::MapGdbOptionNameToFnGdbOptionPtr_t
@@ -28,6 +29,7 @@ const CMICmdCmdGdbSet::MapGdbOptionNameToFnGdbOptionPtr_t
{"solib-search-path", &CMICmdCmdGdbSet::OptionFnSolibSearchPath},
{"disassembly-flavor", &CMICmdCmdGdbSet::OptionFnDisassemblyFlavor},
{"fallback", &CMICmdCmdGdbSet::OptionFnFallback},
+ {"new-console", &CMICmdCmdGdbSet::OptionFnNewConsole},
{"breakpoint", &CMICmdCmdGdbSet::OptionFnBreakpoint}};
//++
@@ -208,6 +210,41 @@ bool CMICmdCmdGdbSet::GetOptionFn(const CMIUtilString &vrPrintFnName,
return false;
}
+bool CMICmdCmdGdbSet::OptionFnNewConsole(
+ const CMIUtilString::VecString_t &vrWords) {
+ bool bNewConsole = false;
+ bool bOk = true;
+
+ if (vrWords.size() > 1)
+ // Too many arguments.
+ bOk = false;
+ else if (vrWords.size() == 0)
+ // If no arguments, default is "on".
+ bNewConsole = true;
+ else if (CMIUtilString::Compare(vrWords[0], "on"))
+ bNewConsole = true;
+ else if (CMIUtilString::Compare(vrWords[0], "off"))
+ bNewConsole = false;
+ else
+ // Unrecognized argument.
+ bOk = false;
+
+ if (!bOk) {
+ // Report error.
+ m_bGbbOptionFnHasError = true;
+ m_strGdbOptionFnError = MIRSRC(IDS_CMD_ERR_GDBSET_OPT_NEWCONSOLE);
+ return MIstatus::failure;
+ }
+
+ // TDDO: Like, set the environment variable ?
+ // Should we maybe set the launch flag too ?
+ if (bNewConsole) {
+ ::setenv( "LLDB_LAUNCH_FLAG_LAUNCH_IN_TTY", "TRUE", false );
+ }
+
+ return MIstatus::success;
+}
+
//++
// Details: Carry out work to complete the GDB set option 'target-async' to
// prepare
diff --git a/src/MICmdCmdGdbSet.h b/src/MICmdCmdGdbSet.h
index c88f997..38f35ae 100644
--- a/src/MICmdCmdGdbSet.h
+++ b/src/MICmdCmdGdbSet.h
@@ -81,6 +81,7 @@ private:
bool OptionFnDisassemblyFlavor(const CMIUtilString::VecString_t &vrWords);
bool OptionFnBreakpoint(const CMIUtilString::VecString_t &vrWords);
bool OptionFnFallback(const CMIUtilString::VecString_t &vrWords);
+ bool OptionFnNewConsole(const CMIUtilString::VecString_t &vrWords);
// Attributes:
private:
diff --git a/src/MICmnResources.cpp b/src/MICmnResources.cpp
index 998c79a..6fe2eaf 100644
--- a/src/MICmnResources.cpp
+++ b/src/MICmnResources.cpp
@@ -439,6 +439,8 @@ const CMICmnResources::SRsrcTextData
{IDS_CMD_ERR_INFO_PRINTFN_FAILED, "The request '%s' failed."},
{IDS_CMD_ERR_GDBSET_OPT_TARGETASYNC,
"'target-async' expects \"on\" or \"off\""},
+ {IDS_CMD_ERR_GDBSET_OPT_NEWCONSOLE,
+ "'new-console' expects \"on\" or \"off\""},
{IDS_CMD_ERR_GDBSET_OPT_BREAKPOINT,
"'breakpoint' expects \"pending on\" or \"pending off\""},
{IDS_CMD_ERR_GDBSET_OPT_SOLIBSEARCHPATH,
diff --git a/src/MICmnResources.h b/src/MICmnResources.h
index 111b0c3..195784b 100644
--- a/src/MICmnResources.h
+++ b/src/MICmnResources.h
@@ -263,6 +263,7 @@ enum {
IDS_CMD_ERR_INFO_PRINTFN_NOT_FOUND,
IDS_CMD_ERR_INFO_PRINTFN_FAILED,
IDS_CMD_ERR_GDBSET_OPT_TARGETASYNC,
+ IDS_CMD_ERR_GDBSET_OPT_NEWCONSOLE,
IDS_CMD_ERR_GDBSET_OPT_BREAKPOINT,
IDS_CMD_ERR_GDBSET_OPT_SOLIBSEARCHPATH,
IDS_CMD_ERR_GDBSET_OPT_PRINT_BAD_ARGS,Woo \o/. with the above PR (#950) and the latest lldb-mi including the new-console patch vimspector regression tests finally pass on macOS Catalina!
BenMBP:vimspector ben$ ./run_tests
Testing with VIMSPECTOR_MIMODE=lldb
%SETUP - Building test programs...
~/.vim/bundle/vimspector/tests/testdata/cpp/simple ~/.vim/bundle/vimspector
rm -f simple variables
rm -rf simple.dSYM variables.dSYM
c++ -g -O0 -std=c++17 simple.cpp -o simple
~/.vim/bundle/vimspector
%DONE - built test programs
Running Vimspector Vim tests
%RUN: breakpoints.test.vim
%PASS: breakpoints.test.vim PASSED
%RUN: language_go.test.vim
%PASS: language_go.test.vim PASSED
%RUN: language_python.test.vim
%PASS: language_python.test.vim PASSED
%RUN: tabpage.test.vim
%PASS: tabpage.test.vim PASSED
All done.
Now, to fix vscode-python which doesn't work with node 13.
This was fixed by #952