[Lldb-commits] [PATCH] D24711: [lldb-mi] Fix implementation for a few mi commands

[Aetf via Phabricator via lldb-commits]

aetf added a comment.

Hi, sorry for the long delay. It has been a busy semester.

Added unit tests are

- `MiGdbSetShowTestCase.test_lldbmi_gdb_set_disassembly_flavor` for disassembly 
flavor settings. Note this one doesn't pass currently due to 
https://llvm.org/bugs/show_bug.cgi?id=31485
- `MiEnvironmentCdTestCase.test_lldbmi_environment_cd` for `-environment-cd` 
command

Extended test case:

- `MiSyntaxTestCase.test_lldbmi_output_grammar` to include a command generating 
console stream records

Fixed test case:

- `MiExecTestCase.test_lldbmi_exec_arguments_set` which breaks because of the 
console stream records

I tested against latest trunk version (290647), and all tests passed with 5 
expected failures and 3 unexpected success:

- `MiGdbSetShowTestCase.test_lldbmi_gdb_set_target_async_off`
- `MiInterpreterExecTestCase.test_lldbmi_settings_set_target_run_args_after`
- `MiSyntaxTestCase.test_lldbmi_process_output`

I don't have the commit access, but I can create separate RRs if necessary. 
(should the target select error fix go to it's own RR?)




Comment at: tools/lldb-mi/MICmdCmdMiscellanous.cpp:515
+  return MIstatus::failure;
   }
 

abidh wrote:
> It is not really an OutofBand record but rather an output of the command. Why 
> not simple add prepend an ~
The output of the command should be a Stream record which is an OutofBand 
record according to the spec [1]. I agree it's no more than prepending '~' and 
quoting the string. But why not just do what the spec says? ;-)

[1] 
https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Stream-Records.html#GDB_002fMI-Stream-Records



Comment at: tools/lldb-mi/MICmdCmdTarget.cpp:126
   lldb::SBStream errMsg;
+  error.GetDescription(errMsg);
   if (!process.IsValid()) {

abidh wrote:
> This does not seem related to any bug fix.
I didn't create a bug report for this since it's not a big deal. But it's 
rather annoying when debugging because the actual error description never gets 
to the output.


https://reviews.llvm.org/D24711



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D24711: [lldb-mi] Fix implementation for a few mi commands

[Aetf via Phabricator via lldb-commits]

aetf updated this revision to Diff 82654.
aetf marked an inline comment as done.
aetf added a comment.

Add unit tests, add error checking for SetInternalVariable


https://reviews.llvm.org/D24711

Files:
  packages/Python/lldbsuite/test/tools/lldb-mi/TestMiEnvironmentCd.py
  packages/Python/lldbsuite/test/tools/lldb-mi/TestMiGdbSetShow.py
  packages/Python/lldbsuite/test/tools/lldb-mi/control/TestMiExec.py
  packages/Python/lldbsuite/test/tools/lldb-mi/main.cpp
  packages/Python/lldbsuite/test/tools/lldb-mi/syntax/TestMiSyntax.py
  tools/lldb-mi/MICmdCmdEnviro.cpp
  tools/lldb-mi/MICmdCmdGdbSet.cpp
  tools/lldb-mi/MICmdCmdGdbSet.h
  tools/lldb-mi/MICmdCmdGdbShow.cpp
  tools/lldb-mi/MICmdCmdGdbShow.h
  tools/lldb-mi/MICmdCmdMiscellanous.cpp
  tools/lldb-mi/MICmdCmdTarget.cpp
  tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
  tools/lldb-mi/MICmnMIOutOfBandRecord.h

Index: tools/lldb-mi/MICmnMIOutOfBandRecord.h
===
--- tools/lldb-mi/MICmnMIOutOfBandRecord.h
+++ tools/lldb-mi/MICmnMIOutOfBandRecord.h
@@ -65,7 +65,9 @@
 eOutOfBand_ThreadSelected,
 eOutOfBand_TargetModuleLoaded,
 eOutOfBand_TargetModuleUnloaded,
-eOutOfBand_TargetStreamOutput
+eOutOfBand_TargetStreamOutput,
+eOutOfBand_ConsoleStreamOutput,
+eOutOfBand_LogStreamOutput
   };
 
   // Methods:
Index: tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
===
--- tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
+++ tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
@@ -48,6 +48,10 @@
 return "library-unloaded";
   case CMICmnMIOutOfBandRecord::eOutOfBand_TargetStreamOutput:
 return "";
+  case CMICmnMIOutOfBandRecord::eOutOfBand_ConsoleStreamOutput:
+return "";
+  case CMICmnMIOutOfBandRecord::eOutOfBand_LogStreamOutput:
+return "";
   }
   assert(false && "unknown CMICmnMIOutofBandRecord::OutOfBand_e");
   return NULL;
@@ -86,6 +90,10 @@
 return "=";
   case CMICmnMIOutOfBandRecord::eOutOfBand_TargetStreamOutput:
 return "@";
+  case CMICmnMIOutOfBandRecord::eOutOfBand_ConsoleStreamOutput:
+return "~";
+  case CMICmnMIOutOfBandRecord::eOutOfBand_LogStreamOutput:
+return "&";
   }
   assert(false && "unknown CMICmnMIOutofBandRecord::OutOfBand_e");
   return NULL;
Index: tools/lldb-mi/MICmdCmdTarget.cpp
===
--- tools/lldb-mi/MICmdCmdTarget.cpp
+++ tools/lldb-mi/MICmdCmdTarget.cpp
@@ -123,6 +123,7 @@
 
   // Verify that we have managed to connect successfully
   lldb::SBStream errMsg;
+  error.GetDescription(errMsg);
   if (!process.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_PLUGIN),
m_cmdData.strMiCmd.c_str(),
Index: tools/lldb-mi/MICmdCmdMiscellanous.cpp
===
--- tools/lldb-mi/MICmdCmdMiscellanous.cpp
+++ tools/lldb-mi/MICmdCmdMiscellanous.cpp
@@ -496,14 +496,22 @@
 //--
 bool CMICmdCmdInterpreterExec::Acknowledge() {
   if (m_lldbResult.GetOutputSize() > 0) {
-CMIUtilString strMsg(m_lldbResult.GetOutput());
-strMsg = strMsg.StripCREndOfLine();
-CMICmnStreamStdout::TextToStdout(strMsg);
+const CMIUtilString line(m_lldbResult.GetOutput());
+const bool bEscapeQuotes(true);
+CMICmnMIValueConst miValueConst(line.Escape(bEscapeQuotes));
+CMICmnMIOutOfBandRecord miOutOfBandRecord(CMICmnMIOutOfBandRecord::eOutOfBand_ConsoleStreamOutput, miValueConst);
+const bool bOk = CMICmnStreamStdout::TextToStdout(miOutOfBandRecord.GetString());
+if (!bOk)
+  return MIstatus::failure;
   }
   if (m_lldbResult.GetErrorSize() > 0) {
-CMIUtilString strMsg(m_lldbResult.GetError());
-strMsg = strMsg.StripCREndOfLine();
-CMICmnStreamStderr::LLDBMsgToConsole(strMsg);
+const CMIUtilString line(m_lldbResult.GetError());
+const bool bEscapeQuotes(true);
+CMICmnMIValueConst miValueConst(line.Escape(bEscapeQuotes));
+CMICmnMIOutOfBandRecord miOutOfBandRecord(CMICmnMIOutOfBandRecord::eOutOfBand_LogStreamOutput, miValueConst);
+const bool bOk = CMICmnStreamStdout::TextToStdout(miOutOfBandRecord.GetString());
+if (!bOk)
+  return MIstatus::failure;
   }
 
   const CMICmnMIResultRecord miRecordResult(
Index: tools/lldb-mi/MICmdCmdGdbShow.h
===
--- tools/lldb-mi/MICmdCmdGdbShow.h
+++ tools/lldb-mi/MICmdCmdGdbShow.h
@@ -78,6 +78,7 @@
   bool OptionFnTargetAsync(const CMIUtilString::VecString_t &vrWords);
   bool OptionFnPrint(const CMIUtilString::VecString_t &vrWords);
   bool OptionFnLanguage(const CMIUtilString::VecString_t &vrWords);
+  bool OptionFnDisassemblyFlavor(const CMIUtilString::VecString_t &vrWords);
   bool OptionFnFallback(const CMIUtilString::VecString_t &vrWords);
 
   // Attributes:
Index: tools/lldb-mi/MICmdCmdGdbShow.cpp
===

[Lldb-commits] [PATCH] D24711: [lldb-mi] Fix implementation for a few mi commands

[Aetf via Phabricator via lldb-commits]

aetf marked 2 inline comments as done.
aetf added a comment.

What do I do next? Could you help me commit and push this? since I don't have 
write access.




Comment at: packages/Python/lldbsuite/test/tools/lldb-mi/main.cpp:22
 {
-int a  = 10;
+int a = 10;
+

abidh wrote:
> This declaration looks redundant.
It's used in `TestMiGdbSetShow.test_lldbmi_gdb_set_ouptut_radix`.


https://reviews.llvm.org/D24711



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits