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

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.

Patch by Vadim Macagon. Thanks!

Test Plan:
./dotest.py -A x86_64 -C clang --executable $BUILDDIR/bin/lldb tools/lldb-mi/

No unexpected failures on my Ubuntu 14.10 64bit Virtualbox VM.

Reviewers: domipheus, ki.stfu, abidh

Reviewed By: ki.stfu, abidh

Subscribers: brucem, lldb-commits

Differential Revision: http://reviews.llvm.org/D10299

llvm-svn: 239297

14 files changed, 24 insertions, 49 deletions

diff --git a/lldb/test/tools/lldb-mi/data/TestMiData.py b/lldb/test/tools/lldb-mi/data/TestMiData.py
index bc973e41aea..3a28139d2e2 100644
--- a/lldb/test/tools/lldb-mi/data/TestMiData.py
+++ b/lldb/test/tools/lldb-mi/data/TestMiData.py
@@ -200,10 +200,13 @@ class MiDataTestCase(lldbmi_testcase.MiTestCaseBase):
         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')
diff --git a/lldb/test/tools/lldb-mi/stack/TestMiStack.py b/lldb/test/tools/lldb-mi/stack/TestMiStack.py
index 3f39a4e956b..acfe1484486 100644
--- a/lldb/test/tools/lldb-mi/stack/TestMiStack.py
+++ b/lldb/test/tools/lldb-mi/stack/TestMiStack.py
@@ -445,7 +445,7 @@ class MiStackTestCase(lldbmi_testcase.MiTestCaseBase):
 
         # 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")
diff --git a/lldb/tools/lldb-mi/MICmdArgSet.cpp b/lldb/tools/lldb-mi/MICmdArgSet.cpp
index 0e93866d30b..3d63a6138f7 100644
--- a/lldb/tools/lldb-mi/MICmdArgSet.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgSet.cpp
@@ -169,28 +169,24 @@ CMICmdArgSet::Validate(const CMIUtilString &vStrMiCmd, CMICmdArgContext &vwCmdAr
     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;
@@ -201,14 +197,7 @@ CMICmdArgSet::Validate(const CMIUtilString &vStrMiCmd, CMICmdArgContext &vwCmdAr
         ++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);
 
diff --git a/lldb/tools/lldb-mi/MICmdArgValConsume.cpp b/lldb/tools/lldb-mi/MICmdArgValConsume.cpp
index 48469f9cff3..c2fe9940d87 100644
--- a/lldb/tools/lldb-mi/MICmdArgValConsume.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValConsume.cpp
@@ -59,7 +59,7 @@ 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());
@@ -68,15 +68,15 @@ CMICmdArgValConsume::Validate(CMICmdArgContext &vwArgContext)
     {
         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;
diff --git a/lldb/tools/lldb-mi/MICmdArgValFile.cpp b/lldb/tools/lldb-mi/MICmdArgValFile.cpp
index 8809badc42c..b34c75ea818 100644
--- a/lldb/tools/lldb-mi/MICmdArgValFile.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValFile.cpp
@@ -60,7 +60,7 @@ 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
 
diff --git a/lldb/tools/lldb-mi/MICmdArgValListOfN.cpp b/lldb/tools/lldb-mi/MICmdArgValListOfN.cpp
index a6ca172e0ca..8e479d52e79 100644
--- a/lldb/tools/lldb-mi/MICmdArgValListOfN.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValListOfN.cpp
@@ -74,7 +74,7 @@ CMICmdArgValListOfN::Validate(CMICmdArgContext &vwArgContext)
     }
 
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     const CMIUtilString &rArg(vwArgContext.GetArgsLeftToParse());
     if (IsListOfN(rArg) && CreateList(rArg))
diff --git a/lldb/tools/lldb-mi/MICmdArgValNumber.cpp b/lldb/tools/lldb-mi/MICmdArgValNumber.cpp
index 2a3ead9f707..75e9700c874 100644
--- a/lldb/tools/lldb-mi/MICmdArgValNumber.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValNumber.cpp
@@ -66,7 +66,7 @@ bool
 CMICmdArgValNumber::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
diff --git a/lldb/tools/lldb-mi/MICmdArgValOptionLong.cpp b/lldb/tools/lldb-mi/MICmdArgValOptionLong.cpp
index 3e9e69a2387..e39a50e0fe3 100644
--- a/lldb/tools/lldb-mi/MICmdArgValOptionLong.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValOptionLong.cpp
@@ -108,7 +108,7 @@ bool
 CMICmdArgValOptionLong::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
diff --git a/lldb/tools/lldb-mi/MICmdArgValPrintValues.cpp b/lldb/tools/lldb-mi/MICmdArgValPrintValues.cpp
index 3e0bbadc15b..3030782a3a2 100644
--- a/lldb/tools/lldb-mi/MICmdArgValPrintValues.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValPrintValues.cpp
@@ -62,7 +62,7 @@ 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))
diff --git a/lldb/tools/lldb-mi/MICmdArgValString.cpp b/lldb/tools/lldb-mi/MICmdArgValString.cpp
index 90e70d79a6b..37e02787c25 100644
--- a/lldb/tools/lldb-mi/MICmdArgValString.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValString.cpp
@@ -108,7 +108,7 @@ bool
 CMICmdArgValString::Validate(CMICmdArgContext &vrwArgContext)
 {
     if (vrwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (m_bHandleQuotedString)
         return ValidateQuotedText(vrwArgContext);
diff --git a/lldb/tools/lldb-mi/MICmdArgValThreadGrp.cpp b/lldb/tools/lldb-mi/MICmdArgValThreadGrp.cpp
index 90b5586b704..c7e663c4721 100644
--- a/lldb/tools/lldb-mi/MICmdArgValThreadGrp.cpp
+++ b/lldb/tools/lldb-mi/MICmdArgValThreadGrp.cpp
@@ -62,7 +62,7 @@ bool
 CMICmdArgValThreadGrp::Validate(CMICmdArgContext &vwArgContext)
 {
     if (vwArgContext.IsEmpty())
-        return MIstatus::success;
+        return m_bMandatory ? MIstatus::failure : MIstatus::success;
 
     if (vwArgContext.GetNumberArgsPresent() == 1)
     {
diff --git a/lldb/tools/lldb-mi/MICmdCmdData.cpp b/lldb/tools/lldb-mi/MICmdCmdData.cpp
index 514c4a8cfb6..9be9f1bb6e0 100644
--- a/lldb/tools/lldb-mi/MICmdCmdData.cpp
+++ b/lldb/tools/lldb-mi/MICmdCmdData.cpp
@@ -617,21 +617,6 @@ CMICmdCmdDataReadMemoryBytes::Execute(void)
         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())
diff --git a/lldb/tools/lldb-mi/MICmnResources.cpp b/lldb/tools/lldb-mi/MICmnResources.cpp
index 4af2dee7c58..4fe0e5667ad 100644
--- a/lldb/tools/lldb-mi/MICmnResources.cpp
+++ b/lldb/tools/lldb-mi/MICmnResources.cpp
@@ -178,7 +178,6 @@ const CMICmnResources::SRsrcTextData CMICmnResources::ms_pResourceId2TextData[]
     {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"},
diff --git a/lldb/tools/lldb-mi/MICmnResources.h b/lldb/tools/lldb-mi/MICmnResources.h
index 724cd04a29d..a470bb54b2b 100644
--- a/lldb/tools/lldb-mi/MICmnResources.h
+++ b/lldb/tools/lldb-mi/MICmnResources.h
@@ -193,7 +193,6 @@ enum
 
     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,