Add -p and -r options to lldb-mi command -file-exec-file-and-symbols to support iOS debugging on macOS

[clayborg]

Looks good except the launch code. See my inlined comment. BTW: to make inline comments in the code, you click and drag in the line numbers column and let go and then you can comment on specific bits of code.

tools/lldb-mi/MICmdCmdExec.cpp

96–110 ↗

(On Diff #21599)

You shouldn't have to do this, you should be able to leave the code like it was before.

[ki.stfu]

I didn't see these options in doc: https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-File-Commands.html. Is it well-known options?

I use the the following code instead of this:

-interpreter-exec console "platform select remote-ios --sysroot \"/Users/IliaK/Library/Developer/Xcode/iOS DeviceSupport/8.1.2 (12B440)\" --build 12B440 --version 8.1.2"
-interpreter-exec console "target create --arch arm64 \"~/Project1.app\""

[ki.stfu]

See my remarks below.

tools/lldb-mi/MICmdCmdFile.cpp

88

Why not eArgValType_StringQuotedNumberPath?

113

Without args is better.

144–153

looks like a hack.

[abidh]

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

[ki.stfu]

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

[abidh]

In D8210#138699, @ki.stfu wrote:

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

These options are not standard MI options. ChuckR is probably trying to add in lldb-mi what he see in "Target Create" command in lldb.

[ki.stfu]

In D8210#138716, @abidh wrote:

In D8210#138699, @ki.stfu wrote:

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

These options are not standard MI options. ChuckR is probably trying to add in lldb-mi what he see in "Target Create" command in lldb.

Then I dislike this patch. I'm not sure that we should add custom options. Is there another way to do it?

[jingham]

I'm not responsible for the MI code or its uses, so this is only a slight opinion:

It seems to me if a client has to call "interpreter-exec" with lldb-specific commands, you are already adding lldb specific options to the MI interface, just in a messy way. It would be cleaner to have some way to ask if the MI flavor is lldb, and then just add in the lldb options in an MI-pretty way. Of course, if you did it this way you should also document the extensions.

Jim

[ChuckR]

In D8210#138718, @ki.stfu wrote:

In D8210#138716, @abidh wrote:

In D8210#138699, @ki.stfu wrote:

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

These options are not standard MI options. ChuckR is probably trying to add in lldb-mi what he see in "Target Create" command in lldb.

Then I dislike this patch. I'm not sure that we should add custom options. Is there another way to do it?

I could not find another way to do this. I agree with jignham that if the answer is to use interpret-exec, then there is a gap in the MI options. interpret-exec- output is harder to parse and is means the consumer of lldb-mi must add additional logic. While these are not officially documented flags in the gdb-mi docs, I do not think that should stop us from adding to lldb-mi where needed. I am happy to talk about a more organized way to add this ability, but simply saying this isn't in the gdb docs so we can't do it doesn't seem like a good answer.

[abidh]

In D8210#139039, @ChuckR wrote:

In D8210#138718, @ki.stfu wrote:

In D8210#138716, @abidh wrote:

In D8210#138699, @ki.stfu wrote:

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

These options are not standard MI options. ChuckR is probably trying to add in lldb-mi what he see in "Target Create" command in lldb.

Then I dislike this patch. I'm not sure that we should add custom options. Is there another way to do it?

I could not find another way to do this. I agree with jignham that if the answer is to use interpret-exec, then there is a gap in the MI options. interpret-exec- output is harder to parse and is means the consumer of lldb-mi must add additional logic. While these are not officially documented flags in the gdb-mi docs, I do not think that should stop us from adding to lldb-mi where needed. I am happy to talk about a more organized way to add this ability, but simply saying this isn't in the gdb docs so we can't do it doesn't seem like a good answer.

In D8210#139039, @ChuckR wrote:

In D8210#138718, @ki.stfu wrote:

In D8210#138716, @abidh wrote:

In D8210#138699, @ki.stfu wrote:

In D8210#138690, @abidh wrote:

I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?

I don't insist on it if these options are well-known. As I said I don't see them in description of -file-exec-and-symbols command.

These options are not standard MI options. ChuckR is probably trying to add in lldb-mi what he see in "Target Create" command in lldb.

Then I dislike this patch. I'm not sure that we should add custom options. Is there another way to do it?

I could not find another way to do this. I agree with jignham that if the answer is to use interpret-exec, then there is a gap in the MI options. interpret-exec- output is harder to parse and is means the consumer of lldb-mi must add additional logic. While these are not officially documented flags in the gdb-mi docs, I do not think that should stop us from adding to lldb-mi where needed. I am happy to talk about a more organized way to add this ability, but simply saying this isn't in the gdb docs so we can't do it doesn't seem like a good answer.

I am all for adding new options if that make sense. But these options should be properly thought out as they get baked into the clients and then become difficult to change. So please address the comments by the other reviews and add a new text file in lldb-mi which documents these new options.

[ChuckR]

Updated based on code review feedback. We confirmed that the interpret-exec command does not suit our needs, and have documented the flags that we have added to file-exec-and-symbols.

[ki.stfu]

If be honest I'd prefer to add this comment into tools/lldb-mi/MICmdCmdFile.h. In that case everyone who are interesting in "-file-exec-and-symbols" will see that it has non-standard options.

There are 2 small fixes (see below):

tools/lldb-mi/MICmdCmdFile.h

72

A little fix:

const CMIUtilString m_constStrArgNamedPlatformName; // Added to support iOS platform selection

73

A little fix:

const CMIUtilString m_constStrArgNamedRemotePath; // Added to support iOS device remote file location

[abidh]

In D8210#139555, @ki.stfu wrote:

If be honest I'd prefer to add this comment into tools/lldb-mi/MICmdCmdFile.h. In that case everyone who are interesting in "-file-exec-and-symbols" will see that it has non-standard options.

There are 2 small fixes (see below):

We may have more extensions in future so it would be good to have one place where all are documented. Your point is valid too. So I think we should add a comment in MICmdCmdFile.h that this command has some new options. Please see the MIExtensions.txt for details.

[clayborg]

Looks good lldb::SB API interaction perspective, I will let others decide the MI implications.

[ChuckR]

Added comment on FileExecAndSymbols to see MIExtensions.txt. Fixed capitalization in comments.

[abidh]

This patch adds some extra empty lines, please remove them. Also handle one inline comment and then feel free to commit.

tools/lldb-mi/MIExtensions.txt

11

I think remove the contents from "if breakpoints are set" onward (last 2 lines) as those lines are not making sense when you have not described that you can give this command without file. Also the purpose is to document the extension.

[ChuckR]

I am still new to this and I cannot commit directly. Should I submit a new patch with these minor changes, or will somebody with commit access make them and submit on my behalf?

[abidh]

In D8210#139990, @ChuckR wrote:

I am still new to this and I cannot commit directly. Should I submit a new patch with these minor changes, or will somebody with commit access make them and submit on my behalf?

I will commit it for you after making those few changes.

[ChuckR]

In D8210#140012, @abidh wrote:

In D8210#139990, @ChuckR wrote:

I am still new to this and I cannot commit directly. Should I submit a new patch with these minor changes, or will somebody with commit access make them and submit on my behalf?

I will commit it for you after making those few changes.

Thanks!

[abidh]

Committed in 232077.

tools/lldb-mi/MICmdCmdFile.h

 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "file-exec-and-symbols".
 //          This command does not follow the MI documentation exactly.
-// Gotchas: None.
+// Gotchas: This command has additonal flags that were not available in GDB MI.
+//          See MIextensions.txt for details.
 // Authors: Illya Rudkin 25/02/2014.
 // Changes: None.
 //--

     const CMIUtilString m_constStrArgNameFile;
     const CMIUtilString
         m_constStrArgThreadGrp; // Not handled by *this command. Not specified in MI spec but Eclipse gives this option sometimes
+    const CMIUtilString m_constStrArgNamedPlatformName; // Added to support iOS platform selection
+    const CMIUtilString m_constStrArgNamedRemotePath; // Added to support iOS device remote file location
 };

tools/lldb-mi/MICmdCmdFile.cpp

 #include "MICmnLLDBDebugSessionInfo.h"
 #include "MIUtilFileStd.h"
 #include "MICmdArgValFile.h"
+#include "MICmdArgValString.h"
 #include "MICmdArgValOptionLong.h"
+#include "MICmdArgValOptionShort.h"
+
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmdCmdFileExecAndSymbols constructor.

 CMICmdCmdFileExecAndSymbols::CMICmdCmdFileExecAndSymbols(void)
     : m_constStrArgNameFile("file")
     , m_constStrArgThreadGrp("thread-group")
+    , m_constStrArgNamedPlatformName("p")
+    , m_constStrArgNamedRemotePath("r")
 {
     // Command factory matches this name with that received from the stdin stream
     m_strMiCmd = "file-exec-and-symbols";

     bool bOk = m_setCmdArgs.Add(
         *(new CMICmdArgValOptionLong(m_constStrArgThreadGrp, false, false, CMICmdArgValListBase::eArgValType_ThreadGrp, 1)));
     bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValFile(m_constStrArgNameFile, true, true)));
+    bOk = bOk &&
+    m_setCmdArgs.Add(*(new CMICmdArgValOptionShort(m_constStrArgNamedPlatformName, false, true,
+                                                   CMICmdArgValListBase::eArgValType_String, 1)));
+    bOk = bOk &&
+    m_setCmdArgs.Add(*(new CMICmdArgValOptionShort(m_constStrArgNamedRemotePath, false, true,
+                                                   CMICmdArgValListBase::eArgValType_StringQuotedNumberPath, 1)));
     return (bOk && ParseValidateCmdOptions());
 }
 

 CMICmdCmdFileExecAndSymbols::Execute(void)
 {
     CMICMDBASE_GETOPTION(pArgNamedFile, File, m_constStrArgNameFile);
+    CMICMDBASE_GETOPTION(pArgPlatformName, OptionShort, m_constStrArgNamedPlatformName);
+    CMICMDBASE_GETOPTION(pArgRemotePath, OptionShort, m_constStrArgNamedRemotePath);
+
     CMICmdArgValFile *pArgFile = static_cast<CMICmdArgValFile *>(pArgNamedFile);
     const CMIUtilString &strExeFilePath(pArgFile->GetValue());
+    bool bPlatformName = pArgPlatformName->GetFound();
+    CMIUtilString platformName;
+    if (bPlatformName)
+    {
+        pArgPlatformName->GetExpectedOption<CMICmdArgValString, CMIUtilString>(platformName);
+    }
+
     CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
     lldb::SBDebugger &rDbgr = rSessionInfo.GetDebugger();
     lldb::SBError error;
     const MIchar *pTargetTriple = nullptr; // Let LLDB discover the triple required
-    const MIchar *pTargetPlatformName = "";
+    const MIchar *pTargetPlatformName = platformName.c_str();
     const bool bAddDepModules = false;
     lldb::SBTarget target = rDbgr.CreateTarget(strExeFilePath.c_str(), pTargetTriple, pTargetPlatformName, bAddDepModules, error);
     CMIUtilString strWkDir;

 
         SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_FNFAILED), m_cmdData.strMiCmd.c_str(), "SetCurrentPlatformSDKRoot()"));
         return MIstatus::failure;
+    
+    }
+    if (pArgRemotePath->GetFound())
+    {
+        CMIUtilString remotePath;
+        pArgRemotePath->GetExpectedOption<CMICmdArgValString, CMIUtilString>(remotePath);
+        lldb::SBModule module = target.FindModule(target.GetExecutable());
+        if (module.IsValid())
+        {
+            module.SetPlatformFileSpec(lldb::SBFileSpec(remotePath.c_str()));
+        }
     }
+    
     lldb::SBStream err;
     if (error.Fail())
     {

tools/lldb-mi/MIExtensions.txt

+-file-exec-and-symbols now takes two new (optional) options:
+
+Synopsis
+
+  -file-exec-and-symbols <file> [-p <platform>] [-r <remote-file>]
+
+Specify the executable file to be debugged. This file is the one from which the symbol table is also read. 
+When debugging remote targets specify a remote-file for execution and a file from which symbols are read. 
+The optional platform is the name of the platform, e.g., "remote-ios" or "ios-simulator". The remote-file 
+is the on-device path to the exe. If breakpoints are set when using this command with no arguments, gdb will 
+produce error messages. Otherwise, no output is produced, except a completion notification.