Add low-frame/high-frame options to -stack-list-arguments (MI)

[ki.stfu]

Summary

Add low-frame/high-frame options to -stack-list-arguments

All tests pass on OS X.

[ki.stfu]

Add tests

[abidh]

Looks good apart from a few comments. Please handle those before committing.

tools/lldb-mi/MICmdCmdStack.cpp

568

You have already checked the GetFound() in if. Why you need it again inside.

575

As I understand, you are trying to check in this else that either low or high is set but not both. Please put a comment as it is not obvious.

592–600

The description of these options says
"It is an error if low-frame is larger than the actual number of frames."

So that check should also be done here.

[ki.stfu]

Fix remarks

[ki.stfu]

tools/lldb-mi/MICmdCmdStack.cpp

568

stupid typo. thx!

[clayborg]

Looks good

test/tools/lldb-mi/stack/TestMiStack.py

def test_lldbmi_stack_list_arguments(self):
 
         # Run to main
         self.runCmd("-break-insert -f main")
         self.expect("\^done,bkpt={number=\"1\"")
         self.runCmd("-exec-run")
         self.expect("\^running")
         self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
-        # Test -stack-list-arguments: use 0 or --no-values
+        # Test that -stack-list-arguments lists empty stack arguments if range is empty
+        self.runCmd("-stack-list-arguments 0 1 0")
+        self.expect("\^done,stack-args=\[\]")
+
+        # Test that -stack-list-arguments lists stack arguments without values
+        # (and that low-frame and high-frame are optional)
         self.runCmd("-stack-list-arguments 0")
         self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[name=\"argc\",name=\"argv\"\]}")
         self.runCmd("-stack-list-arguments --no-values")
         self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[name=\"argc\",name=\"argv\"\]}")
 
-        # Test -stack-list-arguments: use 1 or --all-values
-        self.runCmd("-stack-list-arguments 1")
-        self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}")
-        self.runCmd("-stack-list-arguments --all-values")
-        self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}")
+        # Test that -stack-list-arguments lists stack arguments with all values
+        self.runCmd("-stack-list-arguments 1 0 0")
+        self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}\]")
+        # FIXME: first 0 is treated as --no-values
+        self.runCmd("-stack-list-arguments --all-values 0 0")
+        #self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}\]")
 
-        # Test -stack-list-arguments: use 2 or --simple-values
-        self.runCmd("-stack-list-arguments 2")
-        self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}")
-        self.runCmd("-stack-list-arguments --simple-values")
+        # Test that -stack-list-arguments lists stack arguments with simple values
+        self.runCmd("-stack-list-arguments 2 0 1")
         self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}")
+        # FIXME: first 0 is treated as --no-values
+        self.runCmd("-stack-list-arguments --simple-values 0 1")
+        #self.expect("\^done,stack-args=\[frame={level=\"0\",args=\[{name=\"argc\",value=\"1\"},{name=\"argv\",value=\".*\"}\]}")
+
+        # Test that an invalid low-frame is handled 
+        # FIXME: -1 is treated as unsigned int
+        self.runCmd("-stack-list-arguments 0 -1 0")
+        #self.expect("\^error")
+        self.runCmd("-stack-list-arguments 0 0")
+        self.expect("\^error,msg=\"Command 'stack-list-arguments'. Thread frame range invalid\"")
+
+        # Test that an invalid high-frame is handled
+        # FIXME: -1 is treated as unsigned int
+        self.runCmd("-stack-list-arguments 0 0 -1")
+        #self.expect("\^error")
+
+        # Test that a missing low-frame or high-frame is handled
+        self.runCmd("-stack-list-arguments 0 0")
+        self.expect("\^error,msg=\"Command 'stack-list-arguments'. Thread frame range invalid\"")
+
+        # Test that an invalid low-frame is handled 
+        self.runCmd("-stack-list-arguments 0 0")
+        self.expect("\^error,msg=\"Command 'stack-list-arguments'. Thread frame range invalid\"")
 
     @lldbmi_test
     @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
     @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
     @skipIfLinux # llvm.org/pr22841: lldb-mi tests fail on all Linux buildbots
     def test_lldbmi_stack_list_locals(self):
         """Test that 'lldb-mi --interpreter' can shows local variables."""
 

tools/lldb-mi/MICmdCmdStack.h

class CMICmdCmdStackListArguments : public CMICmdBase
   private:
     bool m_bThreadInvalid; // True = yes invalid thread, false = thread object valid
     CMICmnMIValueList m_miValueList;
     const CMIUtilString m_constStrArgThread;      // Not specified in MI spec but Eclipse gives this option
     const CMIUtilString m_constStrArgPrintValues;
     const CMIUtilString m_constStrArgNoValues;
     const CMIUtilString m_constStrArgAllValues;
     const CMIUtilString m_constStrArgSimpleValues;
+    const CMIUtilString m_constStrArgFrameLow;
+    const CMIUtilString m_constStrArgFrameHigh;
 };
 
 //++ ============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "stack-list-locals".
 // Gotchas: None.
 // Authors: Illya Rudkin 24/03/2014.
 // Changes: None.

tools/lldb-mi/MICmdCmdStack.cpp

 CMICmdCmdStackListArguments::CMICmdCmdStackListArguments(void)
     : m_bThreadInvalid(false)
     , m_miValueList(true)
     , m_constStrArgThread("thread")
     , m_constStrArgPrintValues("print-values")
     , m_constStrArgNoValues("no-values")
     , m_constStrArgAllValues("all-values")
     , m_constStrArgSimpleValues("simple-values")
+    , m_constStrArgFrameLow("low-frame")
+    , m_constStrArgFrameHigh("high-frame")
 {
     // Command factory matches this name with that received from the stdin stream
     m_strMiCmd = "stack-list-arguments";
 
     // Required by the CMICmdFactory when registering *this command
     m_pSelfCreatorFn = &CMICmdCmdStackListArguments::CreateSelf;
 }
 

 CMICmdCmdStackListArguments::ParseArgs(void)
 {
     bool bOk =
         m_setCmdArgs.Add(*(new CMICmdArgValOptionLong(m_constStrArgThread, false, true, CMICmdArgValListBase::eArgValType_Number, 1)));
     bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValNumber(m_constStrArgPrintValues, false, true)));
     bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValOptionLong(m_constStrArgNoValues, false, true)));
     bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValOptionLong(m_constStrArgAllValues, false, true)));
     bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValOptionLong(m_constStrArgSimpleValues, false, true)));
+    bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValNumber(m_constStrArgFrameLow, false, true)));
+    bOk = bOk && m_setCmdArgs.Add(*(new CMICmdArgValNumber(m_constStrArgFrameHigh, false, true)));
     return (bOk && 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.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdCmdStackListArguments::Execute(void)
 {
     CMICMDBASE_GETOPTION(pArgThread, OptionLong, m_constStrArgThread);
     CMICMDBASE_GETOPTION(pArgPrintValues, Number, m_constStrArgPrintValues);
     CMICMDBASE_GETOPTION(pArgNoValues, OptionLong, m_constStrArgNoValues);
     CMICMDBASE_GETOPTION(pArgAllValues, OptionLong, m_constStrArgAllValues);
     CMICMDBASE_GETOPTION(pArgSimpleValues, OptionLong, m_constStrArgSimpleValues);
+    CMICMDBASE_GETOPTION(pArgFrameLow, Number, m_constStrArgFrameLow);
+    CMICMDBASE_GETOPTION(pArgFrameHigh, Number, m_constStrArgFrameHigh);
 
     // Retrieve the --thread option's thread ID (only 1)
     MIuint64 nThreadId = UINT64_MAX;
     if (pArgThread->GetFound())
     {
         if (!pArgThread->GetExpectedOption<CMICmdArgValNumber, MIuint64>(nThreadId))
         {
             SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_OPTION_NOT_FOUND), m_cmdData.strMiCmd.c_str(), m_constStrArgThread.c_str()));
CMICmdCmdStackListArguments::Execute(void)
     else if (pArgSimpleValues->GetFound())
         eVarInfoFormat = CMICmnLLDBDebugSessionInfo::eVariableInfoFormat_SimpleValues;
     else
     {
         SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_PRINT_VALUES), m_cmdData.strMiCmd.c_str()));
         return MIstatus::failure;
     }
 
+    MIuint nFrameLow = 0;
+    MIuint nFrameHigh = UINT32_MAX;
+    if (pArgFrameLow->GetFound() && pArgFrameHigh->GetFound())
+    {
+        nFrameLow = pArgFrameLow->GetValue();
+        nFrameHigh = pArgFrameHigh->GetValue() + 1;
+    }
+    else if (pArgFrameLow->GetFound() || pArgFrameHigh->GetFound())
+    {
+        // Only low-frame or high-frame was specified but both are required
+        SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_THREAD_FRAME_RANGE_INVALID), m_cmdData.strMiCmd.c_str()));
+        return MIstatus::failure;
+    }
+
     CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
     lldb::SBProcess sbProcess = rSessionInfo.GetProcess();
     lldb::SBThread thread = (nThreadId != UINT64_MAX) ? sbProcess.GetThreadByIndexID(nThreadId) : sbProcess.GetSelectedThread();
     m_bThreadInvalid = !thread.IsValid();
     if (m_bThreadInvalid)
         return MIstatus::success;
 
     const lldb::StopReason eStopReason = thread.GetStopReason();
     if ((eStopReason == lldb::eStopReasonNone) || (eStopReason == lldb::eStopReasonInvalid))
     {
         m_bThreadInvalid = true;
         return MIstatus::success;
     }
 
     const MIuint nFrames = thread.GetNumFrames();
-    for (MIuint i = 0; i < nFrames; i++)
+    if (nFrameLow >= nFrames)
+    {
+        // The low-frame is larger than the actual number of frames
+        SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_THREAD_FRAME_RANGE_INVALID), m_cmdData.strMiCmd.c_str()));
+        return MIstatus::failure;
+    }
+
+    nFrameHigh = std::min(nFrameHigh, nFrames);
+    for (MIuint i = nFrameLow; i < nFrameHigh; i++)
     {
         lldb::SBFrame frame = thread.GetFrameAtIndex(i);
         CMICmnMIValueList miValueList(true);
         const MIuint maskVarTypes = CMICmnLLDBDebugSessionInfo::eVariableType_Arguments;
         if (!rSessionInfo.MIResponseFormVariableInfo(frame, maskVarTypes, eVarInfoFormat, miValueList))
             return MIstatus::failure;
         const CMICmnMIValueConst miValueConst(CMIUtilString::Format("%d", i));
         const CMICmnMIValueResult miValueResult("level", miValueConst);