[LLDB-MI] Properly detect missing mandatory arguments to MI commands

[enlight]

Summary

Previously if an MI command had X mandatory and Y optional arguments you could provide X or more optional arguments without providing any of the mandatory arguments, and the argument validation code wouldn't complain.

For example this would pass argument validation even though the mandatory address and count arguments are missing:

-data-read-memory-bytes --thread 1 --frame 0

Part of the problem was that an empty string was considered a valid value for a mandatory argument, which didn't make much sense.

[ki.stfu]

lgtm

[abidh]

Looks good.

test/tools/lldb-mi/data/TestMiData.py

def test_lldbmi_data_read_memory_bytes_local(self):
         # Test that the address argument is required
         self.runCmd('-data-read-memory-bytes')
         self.expect(r'\^error')
 
         # Test that the count argument is required
         self.runCmd('-data-read-memory-bytes &array')
         self.expect(r'\^error')
 
-        # Test that the address argument is required when other options are present
+        # Test that the address and count arguments are required when other options are present
         self.runCmd('-data-read-memory-bytes --thread 1')
         self.expect(r'\^error')
 
+        self.runCmd('-data-read-memory-bytes --thread 1 --frame 0')
+        self.expect(r'\^error')
+
         # Test that the count argument is required when other options are present
         self.runCmd('-data-read-memory-bytes --thread 1 &array')
         self.expect(r'\^error')
 
     @lldbmi_test
     @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
     @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
     def test_lldbmi_data_list_register_names(self):

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

def test_lldbmi_stack_select_frame(self):
         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 that -stack-select-frame requires 1 mandatory argument
         self.runCmd("-stack-select-frame")
-        self.expect("\^error,msg=\"Command 'stack-select-frame'\. Command Args\. Missing options, 1 or more required\"")
+        self.expect("\^error,msg=\"Command 'stack-select-frame'\. Command Args\. Validation failed. Mandatory args not found: frame\"")
 
         # Test that -stack-select-frame fails on invalid frame number
         self.runCmd("-stack-select-frame 99")
         self.expect("\^error,msg=\"Command 'stack-select-frame'\. Frame ID invalid\"")
 
         # Test that current frame is #0
         self.runCmd("-stack-info-frame")
         self.expect("\^done,frame=\{level=\"0\",addr=\"0x[0-9a-f]+\",func=\"main\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\"\}")

tools/lldb-mi/MICmdArgSet.cpp

 // Throws:  None.
 //--
 bool
 CMICmdArgSet::Validate(const CMIUtilString &vStrMiCmd, CMICmdArgContext &vwCmdArgsText)
 {
     m_cmdArgContext = vwCmdArgsText;
 
     // Iterate all the arguments or options required by a command
-    const MIuint nArgs = vwCmdArgsText.GetNumberArgsPresent();
-    MIuint nArgsMandatoryCnt = 0;
     SetCmdArgs_t::const_iterator it = m_setCmdArgs.begin();
     while (it != m_setCmdArgs.end())
     {
         const CMICmdArgValBase *pArg(*it);
-        const CMIUtilString &rArgName(pArg->GetName());
-        MIunused(rArgName);
-        if (pArg->GetIsMandatory())
-            nArgsMandatoryCnt++;
+
         if (!const_cast<CMICmdArgValBase *>(pArg)->Validate(vwCmdArgsText))
         {
-            if (pArg->GetIsMandatory() && !pArg->GetFound())
-                m_setCmdArgsThatAreMissing.push_back(const_cast<CMICmdArgValBase *>(pArg));
-            else if (pArg->GetFound())
+            if (pArg->GetFound())
             {
                 if (pArg->GetIsMissingOptions())
                     m_setCmdArgsMissingInfo.push_back(const_cast<CMICmdArgValBase *>(pArg));
                 else if (!pArg->GetValid())
                     m_setCmdArgsThatNotValid.push_back(const_cast<CMICmdArgValBase *>(pArg));
             }
+            else if (pArg->GetIsMandatory())
+                m_setCmdArgsThatAreMissing.push_back(const_cast<CMICmdArgValBase *>(pArg));
         }
+
         if (pArg->GetFound() && !pArg->GetIsHandledByCmd())
         {
             m_bIsArgsPresentButNotHandledByCmd = true;
             m_setCmdArgsNotHandledByCmd.push_back(const_cast<CMICmdArgValBase *>(pArg));
         }
 
         // Next
         ++it;
     }
 
-    // Check that one or more argument objects have any issues to report...
-
-    if (nArgs < nArgsMandatoryCnt)
-    {
-        SetErrorDescription(CMIUtilString::Format(MIRSRC(IDS_CMD_ARGS_ERR_N_OPTIONS_REQUIRED), nArgsMandatoryCnt));
-        return MIstatus::failure;
-    }
-
+    // report any issues with arguments/options
     if (IsArgsPresentButNotHandledByCmd())
         WarningArgsNotHandledbyCmdLogFile(vStrMiCmd);
 
     return ValidationFormErrorMessages(vwCmdArgsText);
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Having validated the command's options text and failed for some reason form

tools/lldb-mi/MICmdArgValConsume.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValConsume::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     // Consume the optional file, line, linenum arguments till the mode '--' argument
     const CMIUtilString::VecString_t vecOptions(vwArgContext.GetArgs());
     CMIUtilString::VecString_t::const_iterator it = vecOptions.begin();
     while (it != vecOptions.end())
     {
         const CMIUtilString & rTxt( *it );
         
-  if ( rTxt.compare( "--" ) == 0 )
+        if ( rTxt.compare( "--" ) == 0 )
         {
             m_bFound = true;
             m_bValid = true;
-      return MIstatus::success;
-  }
+            return MIstatus::success;
+        }
   
-  if ( !vwArgContext.RemoveArg( rTxt ) )
-      return MIstatus::failure;
+        if ( !vwArgContext.RemoveArg( rTxt ) )
+            return MIstatus::failure;
 
         // Next
         ++it;
     }
 
     return MIstatus::failure;
 }
 

tools/lldb-mi/MICmdArgValFile.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValFile::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     // The GDB/MI spec suggests there is only parameter
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
         const CMIUtilString &rFile(vwArgContext.GetArgsLeftToParse());
         if (IsFilePath(rFile))
         {

tools/lldb-mi/MICmdArgValListOfN.cpp

 {
     if (m_eArgType >= eArgValType_count)
     {
         m_eArgType = eArgValType_invalid;
         return MIstatus::failure;
     }
 
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     const CMIUtilString &rArg(vwArgContext.GetArgsLeftToParse());
     if (IsListOfN(rArg) && CreateList(rArg))
     {
         m_bFound = true;
         m_bValid = true;
         vwArgContext.RemoveArg(rArg);
         return MIstatus::success;

tools/lldb-mi/MICmdArgValNumber.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValNumber::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
         const CMIUtilString &rArg(vwArgContext.GetArgsLeftToParse());
         if (IsArgNumber(rArg) && ExtractNumber(rArg))
         {
             m_bFound = true;
             m_bValid = true;

tools/lldb-mi/MICmdArgValOptionLong.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValOptionLong::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
         const CMIUtilString &rArg(vwArgContext.GetArgsLeftToParse());
         if (IsArgLongOption(rArg) && ArgNameMatch(rArg))
         {
             m_bFound = true;
 

tools/lldb-mi/MICmdArgValPrintValues.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValPrintValues::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     const CMIUtilString strArg(vwArgContext.GetArgs()[0]);
     if (IsArgPrintValues(strArg) && ExtractPrintValues(strArg))
     {
         m_bFound = true;
         m_bValid = true;
         m_argValue = GetPrintValues();
         vwArgContext.RemoveArg(strArg);

tools/lldb-mi/MICmdArgValString.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValString::Validate(CMICmdArgContext &vrwArgContext)
 {
     if (vrwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (m_bHandleQuotedString)
         return ValidateQuotedText(vrwArgContext);
 
     return ValidateSingleText(vrwArgContext);
 }
 
 //++ ------------------------------------------------------------------------------------

tools/lldb-mi/MICmdArgValThreadGrp.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdArgValThreadGrp::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
         const CMIUtilString &rArg(vwArgContext.GetArgsLeftToParse());
         if (IsArgThreadGrp(rArg) && ExtractNumber(rArg))
         {
             m_bFound = true;
             m_bValid = true;

tools/lldb-mi/MICmdCmdData.cpp

CMICmdCmdDataReadMemoryBytes::Execute(void)
     MIuint64 nAddrOffset = 0;
     if (pArgAddrOffset->GetFound() && !pArgAddrOffset->GetExpectedOption<CMICmdArgValNumber, MIuint64>(nAddrOffset))
     {
         SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_OPTION_NOT_FOUND),
                  m_cmdData.strMiCmd.c_str(), m_constStrArgByteOffset.c_str()));
         return MIstatus::failure;
     }
 
-    // FIXME: shouldn't have to ensure mandatory arguments are present, that should've been handled
-    // in ParseArgs(), unfortunately that seems kinda sorta broken right now if options are provided
-    // but mandatory arguments are missing, so here we go...
-    if (!pArgAddrExpr->GetFound())
-    {
-        SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ARGS_ERR_VALIDATION_MANDATORY), m_constStrArgAddrExpr.c_str()));
-        return MIstatus::failure;
-    }
-
-    if (!pArgNumBytes->GetFound())
-    {
-        SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ARGS_ERR_VALIDATION_MANDATORY), m_constStrArgNumBytes.c_str()));
-        return MIstatus::failure;
-    }
-
     CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
     lldb::SBProcess sbProcess = rSessionInfo.GetProcess();
     if (!sbProcess.IsValid())
     {
         SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_PROCESS), m_cmdData.strMiCmd.c_str()));
         return MIstatus::failure;
     }
 

tools/lldb-mi/MICmnResources.h

enum
     IDS_DRIVER_ERR_MAINLOOP,
     IDS_DRIVER_ERR_LOCAL_DEBUG_NOT_IMPL,
     IDS_DRIVER_ERR_LOCAL_DEBUG_INIT,
 
     IDS_DRIVER_WAITING_STDIN_DATA,
 
     IDS_STDERR_ERR_NOT_ALL_DATA_WRITTEN,
 
-    IDS_CMD_ARGS_ERR_N_OPTIONS_REQUIRED,
     IDS_CMD_ARGS_ERR_OPTION_NOT_FOUND,
     IDS_CMD_ARGS_ERR_VALIDATION_MANDATORY,
     IDS_CMD_ARGS_ERR_VALIDATION_INVALID,
     IDS_CMD_ARGS_ERR_VALIDATION_MAN_INVALID,
     IDS_CMD_ARGS_ERR_VALIDATION_MISSING_INF,
     IDS_CMD_ARGS_ERR_CONTEXT_NOT_ALL_EATTEN,
     IDS_CMD_ARGS_ERR_PREFIX_MSG,
 

tools/lldb-mi/MICmnResources.cpp

const CMICmnResources::SRsrcTextData CMICmnResources::ms_pResourceId2TextData[] = {
     {
      IDS_DRIVER_ERR_DRIVER_STATE_ERROR, "Driver. Driver running state error. Cannot go to next state from present state as not allowed",
     },
     {IDS_DRIVER_WAITING_STDIN_DATA, "Driver. Main thread suspended waiting on Stdin Monitor to resume main thread"},
     {IDS_DRIVER_ERR_MAINLOOP, "Driver. Error in do main loop. %s"},
     {IDS_DRIVER_ERR_LOCAL_DEBUG_NOT_IMPL, "Driver. --executable argument given. Local debugging is not implemented."},
     {IDS_DRIVER_ERR_LOCAL_DEBUG_INIT, "Driver. --executable argument given. Initialising local debugging failed."},
     {IDS_STDERR_ERR_NOT_ALL_DATA_WRITTEN, "Stderr. Not all data was written to stream. The data '%s'"},
-    {IDS_CMD_ARGS_ERR_N_OPTIONS_REQUIRED, "Command Args. Missing options, %d or more required"},
     {IDS_CMD_ARGS_ERR_OPTION_NOT_FOUND, "Command Args. Option '%s' not found"},
     {IDS_CMD_ARGS_ERR_VALIDATION_MANDATORY, "Mandatory args not found: %s"},
     {IDS_CMD_ARGS_ERR_VALIDATION_INVALID, "Invalid args: %s"},
     {IDS_CMD_ARGS_ERR_VALIDATION_MAN_INVALID, "Mandatory args not found: %s. Invalid args: %s"},
     {IDS_CMD_ARGS_ERR_VALIDATION_MISSING_INF, "Args missing additional information: %s"},
     {IDS_CMD_ARGS_ERR_CONTEXT_NOT_ALL_EATTEN, "Not all arguments or options were recognised: %s"},
     {IDS_CMD_ARGS_ERR_PREFIX_MSG, "Command Args. Validation failed. "},
     {IDS_VARIANT_ERR_USED_BASECLASS, "Variant container: Variant object used the base class. See CMIUtilVariant"},