Handle the options and parameters separator in every MI command

[abidh]

Summary

As per the following link, the "--" separator can appear between the options
and parameters of any MI command. Previously this separator was only
handled by the -data-disassemble MI command. I have moved the relevant
code into CMICmdBase so that any MI command can handle the
aforementioned separator.

https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Input-Syntax.html#GDB_002fMI-Input-Syntax

[ki.stfu]

tools/lldb-mi/MICmdBase.cpp

102

The -data-disassemble command has '--' mandatory argument, but now it's always optional argument. Should it use m_ConsumeArgMandatory as it works for --thread/--frame etc?

tools/lldb-mi/MICmdCmdData.cpp

280

Please expand MiDataTestCase.test_lldbmi_data_disassemble test to check that it still gives an error without '--' argument.

[abidh]

Handled review comment.

Added a test for -data-disassemble without "--" separator.

[abidh]

tools/lldb-mi/MICmdBase.cpp

102

I changed this intentionally because "--" should not be mandatory. The MI specifications does not make it mandatory and in GDB, disassemble command works with and without it. So this change bring the same behavior in lldb-mi.

tools/lldb-mi/MICmdCmdData.cpp

280

I have added a test that check the -data-disassemble without "--" as it is not mandatory.

[ki.stfu]

lgtm

tools/lldb-mi/MICmdBase.cpp

102

Well, I thought it's mandatory argument according to its spec: https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Data-Manipulation.html

-data-disassemble
   [ -s start-addr -e end-addr ]
 | [ -f filename -l linenum [ -n lines ] ]
 -- mode

But I think we can follow to the GDB syntax.

tools/lldb-mi/MICmdCmdData.cpp

280

ok, thanks.

[abidh]

tools/lldb-mi/MICmdBase.cpp

102

If you read the following, you will see that "--" is not mandatory. It says "Options occur first in the parameter list and can be delimited from normal parameters using ‘--’ (this is useful when some parameters begin with a dash)."

https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Input-Syntax.html#GDB_002fMI-Input-Syntax

[ki.stfu]

tools/lldb-mi/MICmdBase.cpp

102

As for me, it says that dashes aren't mandatory in general cases (i.e. unless otherwise specified in command's spec). In case of -data-disassemble, we know that mode is either 0, 1, 2, or 3, and therefore according to your link, the syntax would be:

-data-disassemble [ -s start-addr -e end-addr ] | [ -f filename -l linenum [ -n lines ] ] [--] mode

(i.e. "--" is optional)

But the spec says that '--' is mandatory argument and I perceive it as a special case of usage '--'. Therefore I supposed that it's mandatory argument.

packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py

def test_lldbmi_data_disassemble(self):
         # Get an address for disassembling: use main
         self.runCmd("-data-evaluate-expression main")
         self.expect("\^done,value=\"0x[0-9a-f]+ \(a.out`main at main.cpp:[0-9]+\)\"")
         addr = int(self.child.after.split("\"")[1].split(" ")[0], 16)
 
         # Test -data-disassemble: try to disassemble some address
         self.runCmd("-data-disassemble -s %#x -e %#x -- 0" % (addr, addr + 0x10))
         self.expect("\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," % addr)
         
+        # Test -data-disassemble without "--"
+        self.runCmd("-data-disassemble -s %#x -e %#x 0" % (addr, addr + 0x10))
+        self.expect("\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," % addr)
+
         # Run to hello_world
         self.runCmd("-break-insert -f hello_world")
         self.expect("\^done,bkpt={number=\"2\"")
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # Get an address for disassembling: use hello_world

tools/lldb-mi/MICmdBase.h

protected:
     CMICmnLLDBDebugSessionInfo &
         m_rLLDBDebugSessionInfo;  // Access to command sharing information or data across any and all command based derived classes.
     bool m_bHasResultRecordExtra; // True = Yes command produced additional MI output to its 1 line response, false = no extra MI output
                                   // formed.
     CMICmdArgSet m_setCmdArgs;    // The list of arguments *this command needs to parse from the options string to carry out work.
     const CMIUtilString m_constStrArgThreadGroup;
     const CMIUtilString m_constStrArgThread;
     const CMIUtilString m_constStrArgFrame;
+    const CMIUtilString m_constStrArgConsume;
 
     // These 3 members can be used by the derived classes to make any of
     // "thread", "frame" or "thread-group" mandatory.
     bool m_ThreadGrpArgMandatory;
     bool m_ThreadArgMandatory;
     bool m_FrameArgMandatory;
 };
 

tools/lldb-mi/MICmdBase.cpp

 //===-- MICmdBase.cpp -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // In-house headers:
 #include "MICmdBase.h"
 #include "MICmnMIValueConst.h"
 #include "MICmnLLDBDebugSessionInfo.h"
 #include "MICmdArgValOptionLong.h"
+#include "MICmdArgValConsume.h"
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmdBase constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmdBase::CMICmdBase()
     : m_pSelfCreatorFn(nullptr)
     , m_rLLDBDebugSessionInfo(CMICmnLLDBDebugSessionInfo::Instance())
     , m_bHasResultRecordExtra(false)
     , m_constStrArgThreadGroup("thread-group")
     , m_constStrArgThread("thread")
     , m_constStrArgFrame("frame")
+    , m_constStrArgConsume("--")
     , m_ThreadGrpArgMandatory(false)
     , m_ThreadArgMandatory(false)
     , m_FrameArgMandatory(false)
 {
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmdBase destructor.

 // Throws:  None.
 //--
 void
 CMICmdBase::AddCommonArgs()
 {
     m_setCmdArgs.Add(new CMICmdArgValOptionLong(m_constStrArgThreadGroup, m_ThreadGrpArgMandatory, true, CMICmdArgValListBase::eArgValType_ThreadGrp, 1));
     m_setCmdArgs.Add(new CMICmdArgValOptionLong(m_constStrArgThread, m_ThreadArgMandatory, true, CMICmdArgValListBase::eArgValType_Number, 1));
     m_setCmdArgs.Add(new CMICmdArgValOptionLong(m_constStrArgFrame, m_FrameArgMandatory, true, CMICmdArgValListBase::eArgValType_Number, 1));
+    m_setCmdArgs.Add(new CMICmdArgValConsume(m_constStrArgConsume, false));
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: The invoker requires this function. A command must be given working data and
 //          provide data about its status or provide information to other objects.
 // Type:    Overridden.
 // Args:    None.
 // Return:  None.

tools/lldb-mi/MICmdCmdData.h

public:
     bool ParseArgs() override;
     // From CMICmnBase
     /* dtor */ ~CMICmdCmdDataDisassemble() override;
 
     // Attributes:
   private:
     const CMIUtilString m_constStrArgAddrStart; // MI spec non mandatory, *this command mandatory
     const CMIUtilString m_constStrArgAddrEnd;   // MI spec non mandatory, *this command mandatory
-    const CMIUtilString m_constStrArgConsume;
     const CMIUtilString m_constStrArgMode;
     CMICmnMIValueList m_miValueList;
 };
 
 //++ ============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "data-read-memory-bytes".
 //--

tools/lldb-mi/MICmdCmdData.cpp

 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmdCmdDataDisassemble::CMICmdCmdDataDisassemble()
     : m_constStrArgAddrStart("s")
     , m_constStrArgAddrEnd("e")
-    , m_constStrArgConsume("--")
     , m_constStrArgMode("mode")
     , m_miValueList(true)
 {
     // Command factory matches this name with that received from the stdin stream
     m_strMiCmd = "data-disassemble";
 
     // Required by the CMICmdFactory when registering *this command
     m_pSelfCreatorFn = &CMICmdCmdDataDisassemble::CreateSelf;

 //--
 bool
 CMICmdCmdDataDisassemble::ParseArgs()
 {
     m_setCmdArgs.Add(
         new CMICmdArgValOptionShort(m_constStrArgAddrStart, true, true, CMICmdArgValListBase::eArgValType_StringQuotedNumber, 1));
     m_setCmdArgs.Add(
         new CMICmdArgValOptionShort(m_constStrArgAddrEnd, true, true, CMICmdArgValListBase::eArgValType_StringQuotedNumber, 1));
-    m_setCmdArgs.Add(new CMICmdArgValConsume(m_constStrArgConsume, true));
     m_setCmdArgs.Add(new CMICmdArgValNumber(m_constStrArgMode, true, true));
     return ParseValidateCmdOptions();
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command does work in this function.
 //          The command is likely to communicate with the LLDB SBDebugger in here.
 // Type:    Overridden.