[lldb-mi] Re-implement data-info-line command.

[apolyakov]

Summary

Now data-info-line command uses SB API instead of HandleCommand.

[aprantl]

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

376 ↗

(On Diff #154534)

Why are you changing the regexp here? '.' matches any character, so to match the literal . you need to escape it with a backslash.

tools/lldb-mi/MICmdCmdData.cpp

28 ↗

(On Diff #154534)

I don't think that comment is necessary.

1691 ↗

(On Diff #154534)

@clayborg: Is there something better we could do instead of doing a linear search through all debug line entries?

[apolyakov]

Returned accidentally removed backslashes in self.expect pattern, removed comment from:
#include <string> // For std::to_string.

[apolyakov]

tools/lldb-mi/MICmdCmdData.cpp

1691 ↗

(On Diff #154534)

I think we can get rid of this by adding a new method to SBModule - ResolveSymbolContextsForFileSpec, which will use a method with the same from Module.

[aprantl]

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

22 ↗

(On Diff #154623)

It would be *awesome* if we could also convert this entire to a lit/FileCheck. Looks like all the tests in this file are basically skipped everywhere because it's so unreliable...

[apolyakov]

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

22 ↗

(On Diff #154623)

It's problematic to convert data-info-line test to a lit one since we don't know addresses of a source lines. It means that we can't do this:
-data-info-line *0xsome_address

[aprantl]

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

22 ↗

(On Diff #154623)

I see. You are right, we can't change make one command input depend on previous output in a LIT-based test. But it looks like this is only needed for testing one form of the data-info-line command. We could extract everything else into a LIT-based test that runs on every platform and still greatly improve the test coverage.

[apolyakov]

It would be good to convert to LIT as more tests as we can. I'll do this in one of a next patches. Let's continue with this one?

[apolyakov]

Converted data-info-line python test to a lit one.

lldb/trunk/lit/tools/lldb-mi/data/data-info-line.test

+# XFAIL: windows
+# -> llvm.org/pr24452
+#
+# RUN: %cc -o %t %p/inputs/data-info-line.c -g
+# RUN: %lldbmi %t < %s | FileCheck %s
+
+# Test lldb-mi -data-info-line command.
+
+# Check that we have a valid target created via '%lldbmi %t'.
+# CHECK: ^done
+
+-break-insert main
+# CHECK: ^done,bkpt={number="1"
+
+-exec-run
+# CHECK: ^running
+# CHECK: *stopped,reason="breakpoint-hit"
+
+-data-info-line *0x0
+# Test that -data-info-line fails when invalid address is specified.
+# CHECK: ^error,msg="Command 'data-info-line'. Error: The LineEntry is absent or has an unknown format."
+
+-data-info-line unknown_file:1
+# Test that -data-info-line fails when file is unknown.
+# CHECK: ^error,msg="Command 'data-info-line'. Error: The LineEntry is absent or has an unknown format."
+
+-data-info-line data-info-line.c:bad_line
+# Test that -data-info-line fails when line has invalid format.
+# CHECK: ^error,msg="Command 'data-info-line'. Error: The LineEntry is absent or has an unknown format."
+
+-data-info-line data-info-line.c:0
+# Test that -data-info-line fails when invalid line is specified.
+# CHECK: ^error,msg="Command 'data-info-line'. Error: The LineEntry is absent or has an unknown format."
+
+-data-info-line data-info-line.c:2
+# CHECK: ^done,start="0x{{[0-9a-f]+}}",end="0x{{[0-9a-f]+}}",file="{{.*}}data-info-line.c",line="{{[0-9]+}}"

lldb/trunk/lit/tools/lldb-mi/data/inputs/data-info-line.c

+int main(void) {
+  int x = 0;
+  return 12345 + x;
+}

lldb/trunk/lit/tools/lldb-mi/data/lit.local.cfg

+config.suffixes = ['.test']

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

def test_lldbmi_data_list_register_values(self):
         self.runCmd("-data-list-register-values x 0")
         self.expect(
             "\^done,register-values=\[{number=\"0\",value=\"0x[0-9a-f]+\"}\]")
 
     @skipIfRemote   # We do not currently support remote debugging via the MI.
     @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
     @skipIfDarwin   # pexpect is known to be unreliable on Darwin
     @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races
-    def test_lldbmi_data_info_line(self):
-        """Test that 'lldb-mi --interpreter' works for -data-info-line."""
-
-        self.spawnLldbMi(args=None)
-
-        # Load executable
-        self.runCmd("-file-exec-and-symbols %s" % self.myexe)
-        self.expect("\^done")
-
-        # 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\"")
-
-        # Get the address of main and its line
-        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)
-        line = line_number('main.cpp', '// FUNC_main')
-
-        # Test that -data-info-line works for address
-        self.runCmd("-data-info-line *%#x" % addr)
-        self.expect(
-            "\^done,start=\"0x0*%x\",end=\"0x[0-9a-f]+\",file=\".+?main.cpp\",line=\"%d\"" %
-            (addr, line))
-
-        # Test that -data-info-line works for file:line
-        self.runCmd("-data-info-line main.cpp:%d" % line)
-        self.expect(
-            "\^done,start=\"0x0*%x\",end=\"0x[0-9a-f]+\",file=\".+?main.cpp\",line=\"%d\"" %
-            (addr, line))
-
-        # Test that -data-info-line fails when invalid address is specified
-        self.runCmd("-data-info-line *0x0")
-        self.expect(
-            "\^error,msg=\"Command 'data-info-line'\. Error: The LineEntry is absent or has an unknown format\.\"")
-
-        # Test that -data-info-line fails when file is unknown
-        self.runCmd("-data-info-line unknown_file:1")
-        self.expect(
-            "\^error,msg=\"Command 'data-info-line'\. Error: The LineEntry is absent or has an unknown format\.\"")
-
-        # Test that -data-info-line fails when line has invalid format
-        self.runCmd("-data-info-line main.cpp:bad_line")
-        self.expect(
-            "\^error,msg=\"error: invalid line number string 'bad_line'")
-        self.runCmd("-data-info-line main.cpp:0")
-        self.expect("\^error,msg=\"error: zero is an invalid line number")
-
-    @skipIfRemote   # We do not currently support remote debugging via the MI.
-    @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
-    @skipIfDarwin   # pexpect is known to be unreliable on Darwin
-    @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races
     def test_lldbmi_data_evaluate_expression(self):
         """Test that 'lldb-mi --interpreter' works for -data-evaluate-expression."""
 
         self.spawnLldbMi(args=None)
 
         # Load executable
         self.runCmd("-file-exec-and-symbols %s" % self.myexe)
         self.expect("\^done")

lldb/trunk/tools/lldb-mi/MICmdCmdData.h

 //              For an introduction to adding a new command see
 //              CMICmdCmdSupportInfoMiCmdQuery
 //              command class as an example.
 //
 
 #pragma once
 
 // Third party headers:
-#include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBError.h"
 
 // In-house headers:
 #include "MICmdBase.h"
 #include "MICmnLLDBDebugSessionInfoVarObj.h"
 #include "MICmnMIValueList.h"
 #include "MICmnMIValueTuple.h"
+#include "MICmnMIResultRecord.h"
 
 //++
 //============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "data-evaluate-expression".
 //--
 class CMICmdCmdDataEvaluateExpression : public CMICmdBase {
   // Statics:
public:
   bool Execute() override;
   bool Acknowledge() override;
   bool ParseArgs() override;
   // From CMICmnBase
   /* dtor */ ~CMICmdCmdDataInfoLine() override;
 
   // Attributes:
 private:
-  lldb::SBCommandReturnObject m_lldbResult;
   const CMIUtilString m_constStrArgLocation;
+  CMICmnMIResultRecord m_resultRecord;
 };

lldb/trunk/tools/lldb-mi/MICmdCmdData.cpp

 //              CMICmdCmdDataListRegisterNames      implementation.
 //              CMICmdCmdDataListRegisterValues     implementation.
 //              CMICmdCmdDataListRegisterChanged    implementation.
 //              CMICmdCmdDataWriteMemoryBytes       implementation.
 //              CMICmdCmdDataWriteMemory            implementation.
 //              CMICmdCmdDataInfoLine               implementation.
 
 // Third Party Headers:
-#include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBInstruction.h"
 #include "lldb/API/SBInstructionList.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBThread.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Regex.h"
 #include <inttypes.h> // For PRIx64
+#include <string>
 
 // In-house headers:
 #include "MICmdArgValConsume.h"
 #include "MICmdArgValListOfN.h"
 #include "MICmdArgValNumber.h"
 #include "MICmdArgValOptionLong.h"
 #include "MICmdArgValOptionShort.h"
 #include "MICmdArgValString.h"
 #include "MICmdArgValThreadGrp.h"
 #include "MICmdCmdData.h"
 #include "MICmnLLDBDebugSessionInfo.h"
 #include "MICmnLLDBDebugSessionInfoVarObj.h"
 #include "MICmnLLDBDebugger.h"
 #include "MICmnLLDBProxySBValue.h"
 #include "MICmnLLDBUtilSBValue.h"
 #include "MICmnMIResultRecord.h"
 #include "MICmnMIValueConst.h"
 
+namespace {
+CMIUtilString IntToHexAddrStr(uint32_t number) {
+  return CMIUtilString("0x" + llvm::Twine::utohexstr(number).str());
+}
+} // namespace
+
 //++
 //------------------------------------------------------------------------------------
 // Details: CMICmdCmdDataEvaluateExpression constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--

 //------------------------------------------------------------------------------------
 // Details: CMICmdCmdDataInfoLine constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmdCmdDataInfoLine::CMICmdCmdDataInfoLine()
-    : m_constStrArgLocation("location") {
+    : m_constStrArgLocation("location"),
+      m_resultRecord(m_cmdData.strMiCmdToken,
+                     CMICmnMIResultRecord::eResultClass_Done) {
   // Command factory matches this name with that received from the stdin stream
   m_strMiCmd = "data-info-line";
 
   // Required by the CMICmdFactory when registering *this command
   m_pSelfCreatorFn = &CMICmdCmdDataInfoLine::CreateSelf;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMICmdCmdDataInfoLine destructor.
 // Type:    Overrideable.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
-CMICmdCmdDataInfoLine::~CMICmdCmdDataInfoLine() {}
+CMICmdCmdDataInfoLine::~CMICmdCmdDataInfoLine() = default;
 
 //++
 //------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The parses the command line
 // options
 //          arguments to extract values for each of those arguments.
 // Type:    Overridden.
 // Args:    None.

 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool CMICmdCmdDataInfoLine::Execute() {
   CMICMDBASE_GETOPTION(pArgLocation, String, m_constStrArgLocation);
 
+  lldb::SBLineEntry line;
+  bool found_line = false;
   const CMIUtilString &strLocation(pArgLocation->GetValue());
-  CMIUtilString strCmdOptionsLocation;
+  lldb::SBTarget target = CMICmnLLDBDebugSessionInfo::Instance().GetTarget();
+
   if (strLocation.at(0) == '*') {
     // Parse argument:
     // *0x12345
-    //  ^^^^^^^ -- address
-    const CMIUtilString strAddress(strLocation.substr(1));
-    strCmdOptionsLocation =
-        CMIUtilString::Format("--address %s", strAddress.c_str());
+    // ^^^^^^^^^ -- address
+    lldb::addr_t address = 0x0;
+    if (llvm::StringRef(strLocation.substr(1)).getAsInteger(0, address)) {
+      SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SOME_ERROR),
+                                     m_cmdData.strMiCmd.c_str(),
+                                     "Failed to parse address."));
+      return MIstatus::failure;
+    }
+    line = target.ResolveFileAddress(address).GetLineEntry();
+    // Check that found line is valid.
+    if (line.GetLine())
+      found_line = true;
   } else {
     const size_t nLineStartPos = strLocation.rfind(':');
     if ((nLineStartPos == std::string::npos) || (nLineStartPos == 0) ||
         (nLineStartPos == strLocation.length() - 1)) {
-      SetError(
-          CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_LOCATION_FORMAT),
-                                m_cmdData.strMiCmd.c_str(), strLocation.c_str())
-              .c_str());
+      SetError(CMIUtilString::Format(
+          MIRSRC(IDS_CMD_ERR_INVALID_LOCATION_FORMAT),
+          m_cmdData.strMiCmd.c_str(), strLocation.c_str()));
       return MIstatus::failure;
     }
     // Parse argument:
     // hello.cpp:5
     // ^^^^^^^^^ -- file
     //           ^ -- line
-    const CMIUtilString strFile(strLocation.substr(0, nLineStartPos));
-    const CMIUtilString strLine(strLocation.substr(nLineStartPos + 1));
-    strCmdOptionsLocation =
-        CMIUtilString::Format("--file \"%s\" --line %s",
-                              strFile.AddSlashes().c_str(), strLine.c_str());
+    const CMIUtilString &strFile(strLocation.substr(0, nLineStartPos));
+    uint32_t numLine = 0;
+    llvm::StringRef(strLocation.substr(nLineStartPos + 1))
+        .getAsInteger(0, numLine);
+    lldb::SBSymbolContextList sc_cu_list =
+        target.FindCompileUnits(lldb::SBFileSpec(strFile.c_str(), false));
+    for (uint32_t i = 0, e = sc_cu_list.GetSize(); i < e; ++i) {
+      const lldb::SBCompileUnit &cu =
+        sc_cu_list.GetContextAtIndex(i).GetCompileUnit();
+      // Break if we have already found requested line.
+      if (found_line)
+        break;
+      for (uint32_t j = 0, e = cu.GetNumLineEntries(); j < e; ++j) {
+        const lldb::SBLineEntry &curLine = cu.GetLineEntryAtIndex(j);
+        if (curLine.GetLine() == numLine) {
+          line = curLine;
+          found_line = true;
+          break;
         }
-  const CMIUtilString strCmd(CMIUtilString::Format(
-      "target modules lookup -v %s", strCmdOptionsLocation.c_str()));
-
-  CMICmnLLDBDebugSessionInfo &rSessionInfo(
-      CMICmnLLDBDebugSessionInfo::Instance());
-  const lldb::ReturnStatus rtn =
-      rSessionInfo.GetDebugger().GetCommandInterpreter().HandleCommand(
-          strCmd.c_str(), m_lldbResult);
-  MIunused(rtn);
-
-  return MIstatus::success;
       }
-
-//++
-//------------------------------------------------------------------------------------
-// Details: Helper function for parsing a line entry returned from lldb for the
-// command:
-//              target modules lookup -v <location>
-//          where the line entry is of the format:
-//              LineEntry: \[0x0000000100000f37-0x0000000100000f45\):
-//              /path/file:3[:1]
-//                           start              end                   file
-//                           line column(opt)
-// Args:    input - (R) Input string to parse.
-//          start - (W) String representing the start address.
-//          end   - (W) String representing the end address.
-//          file  - (W) String representing the file.
-//          line  - (W) String representing the line.
-// Return:  bool - True = input was parsed successfully, false = input could not
-// be parsed.
-// Throws:  None.
-//--
-static bool ParseLLDBLineEntry(const char *input, CMIUtilString &start,
-                               CMIUtilString &end, CMIUtilString &file,
-                               CMIUtilString &line) {
-  // Note: Ambiguities arise because the column is optional, and
-  // because : can appear in filenames or as a byte in a multibyte
-  // UTF8 character.  We keep those cases to a minimum by using regex
-  // to work on the string from both the left and right, so that what
-  // is remains is assumed to be the filename.
-
-  // Match LineEntry using regex.
-  static llvm::Regex g_lineentry_nocol_regex(llvm::StringRef(
-      "^ *LineEntry: \\[(0x[0-9a-fA-F]+)-(0x[0-9a-fA-F]+)\\): (.+):([0-9]+)$"));
-  static llvm::Regex g_lineentry_col_regex(
-      llvm::StringRef("^ *LineEntry: \\[(0x[0-9a-fA-F]+)-(0x[0-9a-fA-F]+)\\): "
-                      "(.+):([0-9]+):[0-9]+$"));
-  //                                ^1=start         ^2=end               ^3=f
-  //                                ^4=line ^5=:col(opt)
-
-  llvm::SmallVector<llvm::StringRef, 6> match;
-
-  // First try matching the LineEntry with the column,
-  // then try without the column.
-  const bool ok = g_lineentry_col_regex.match(input, &match) ||
-                  g_lineentry_nocol_regex.match(input, &match);
-  if (ok) {
-    start = match[1];
-    end = match[2];
-    file = match[3];
-    line = match[4];
     }
-  return ok;
+  }
+  if (!found_line) {
+    SetError(CMIUtilString::Format(
+        MIRSRC(IDS_CMD_ERR_SOME_ERROR), m_cmdData.strMiCmd.c_str(),
+        "The LineEntry is absent or has an unknown format."));
+    return MIstatus::failure;
+  }
+  // Start address.
+  m_resultRecord.Add(CMICmnMIValueResult(
+      "start", CMICmnMIValueConst(IntToHexAddrStr(
+               line.GetStartAddress().GetFileAddress()))));
+  // End address.
+  m_resultRecord.Add(CMICmnMIValueResult(
+      "end", CMICmnMIValueConst(IntToHexAddrStr(
+             line.GetEndAddress().GetFileAddress()))));
+  // File.
+  std::unique_ptr<char[]> upPath(new char[PATH_MAX]);
+  line.GetFileSpec().GetPath(upPath.get(), PATH_MAX);
+  m_resultRecord.Add(CMICmnMIValueResult(
+      "file", CMICmnMIValueConst(CMIUtilString(upPath.get()))));
+  // Line.
+  m_resultRecord.Add(CMICmnMIValueResult(
+      "line", CMICmnMIValueConst(std::to_string(line.GetLine()))));
+  return MIstatus::success;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command prepares a MI Record
 // Result
 //          for the work carried out in the Execute().
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool CMICmdCmdDataInfoLine::Acknowledge() {
-  if (m_lldbResult.GetErrorSize() > 0) {
-    const CMICmnMIValueConst miValueConst(m_lldbResult.GetError());
-    const CMICmnMIValueResult miValueResult("msg", miValueConst);
-    const CMICmnMIResultRecord miRecordResult(
-        m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Error,
-        miValueResult);
-    m_miResultRecord = miRecordResult;
-    return MIstatus::success;
-  } else if (m_lldbResult.GetOutputSize() > 0) {
-    CMIUtilString::VecString_t vecLines;
-    const CMIUtilString strLldbMsg(m_lldbResult.GetOutput());
-    const MIuint nLines(strLldbMsg.SplitLines(vecLines));
-
-    for (MIuint i = 0; i < nLines; ++i) {
-      // String looks like:
-      // LineEntry: \[0x0000000100000f37-0x0000000100000f45\):
-      // /path/to/file:3[:1]
-      const CMIUtilString &rLine(vecLines[i]);
-      CMIUtilString strStart;
-      CMIUtilString strEnd;
-      CMIUtilString strFile;
-      CMIUtilString strLine;
-
-      if (!ParseLLDBLineEntry(rLine.c_str(), strStart, strEnd, strFile,
-                              strLine))
-        continue;
-
-      const CMICmnMIValueConst miValueConst(strStart);
-      const CMICmnMIValueResult miValueResult("start", miValueConst);
-      CMICmnMIResultRecord miRecordResult(
-          m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Done,
-          miValueResult);
-      const CMICmnMIValueConst miValueConst2(strEnd);
-      const CMICmnMIValueResult miValueResult2("end", miValueConst2);
-      miRecordResult.Add(miValueResult2);
-      const CMICmnMIValueConst miValueConst3(strFile);
-      const CMICmnMIValueResult miValueResult3("file", miValueConst3);
-      miRecordResult.Add(miValueResult3);
-      const CMICmnMIValueConst miValueConst4(strLine);
-      const CMICmnMIValueResult miValueResult4("line", miValueConst4);
-      miRecordResult.Add(miValueResult4);
-
-      // MI print "%s^done,start=\"%d\",end=\"%d\"",file=\"%s\",line=\"%d\"
-      m_miResultRecord = miRecordResult;
-
-      return MIstatus::success;
-    }
-  }
-
-  // MI print "%s^error,msg=\"Command '-data-info-line'. Error: The LineEntry is
-  // absent or has an unknown format.\""
-  const CMICmnMIValueConst miValueConst(CMIUtilString::Format(
-      MIRSRC(IDS_CMD_ERR_SOME_ERROR), m_cmdData.strMiCmd.c_str(),
-      "The LineEntry is absent or has an unknown format."));
-  const CMICmnMIValueResult miValueResult("msg", miValueConst);
-  const CMICmnMIResultRecord miRecordResult(
-      m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Error,
-      miValueResult);
-  m_miResultRecord = miRecordResult;
-
+  m_miResultRecord = m_resultRecord;
   return MIstatus::success;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: Required by the CMICmdFactory when registering *this command. The
 // factory
 //          calls this function to create an instance of *this command.
 // Type:    Static method.
 // Args:    None.
 // Return:  CMICmdBase * - Pointer to a new command.
 // Throws:  None.
 //--
 CMICmdBase *CMICmdCmdDataInfoLine::CreateSelf() {
   return new CMICmdCmdDataInfoLine();
 }