[lldb-mi] Include full path in the -data-disassemble response

[anton.kolesov]

Summary

Simple "file" might not be enough for Eclipse CDT to find the file, therefore it is preferable for -data-disassemble response to include full absolute file path in the "fullname" field, similarly to how it is implemented in the CMICmnLLDBDebugSessionInfo::GetFrameInfo() function.

[abidh]

Looks ok but I would like a testcase to go with the change.

[anton.kolesov]

Added a simple test case.

[clayborg]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

This shouldn't be static. Probably not going to cause problems if this is always single threaded, but still you should fix this.

[anton.kolesov]

Replaced static local variable (as can be found in CMICmnLLDBDebugSessionInfo::GetFrameInfo) with a unique_ptr to the char array (as done in CMICmnLLDBDebuggerHandleEvents::MiHelpGetModuleInfo).

[clayborg]

lldb/tools/lldb-mi/MICmdCmdData.cpp

406

There is a variant of FileSpec::GetPath() that returns a std::string:

std::string path = lineEntry.GetFileSpec().GetPath();

Then you can get rid of the code above:

std::unique_ptr<char[]> pPathBuffer(new char[PATH_MAX]);

[anton.kolesov]

lldb/tools/lldb-mi/MICmdCmdData.cpp

406

LLDB MI uses SBFileSpec rather then FileSpec, and SBFileSpec doesn't have such a function.

[clayborg]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

Confused as to why we are calling malloc and free here for pPathBuffer? Why not just:

char pPathBuffer[PATH_MAX];

[anton.kolesov]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

I don't have a strong opinion on this, so to maintain consistency in the code I'm trying to use what other code in lldb-mi uses in similar situations - which is either unique_ptr or static local variable, but I presume it was decided that second approach is not good. FWIW, If I were to write code without regard of what is being done in the same project, then I would never ever had a variable named "miValueConst5".

[clayborg]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

I would just use a local variable on the stack as I suggested. static variables are not correct and should never be used in cases like this, so if you see other errors, might be a good idea to submit a patch and improve lldb-mi. I am all for consistency where it makes sense, but I am also for fixing issues when we see them.

[anton.kolesov]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

But what is wrong with the approach of using std::unique_ptr<char[]> local variable?

[clayborg]

lldb/tools/lldb-mi/MICmdCmdData.cpp

405

That forces a call to malloc(...) and free(...). No need to involve allocations on the heap for such simple things

[anton.kolesov]

Allocate path buffer on stack instead of heap.

[clayborg]

LGTM. If there are any other " std::unique_ptr<char[]>" instances you would like to fix after this patch, feel free to submit more fixes without need for review.

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

def test_lldbmi_data_disassemble(self):
             addr)
 
         # Test -data-disassemble without "--"
         self.runCmd("-data-disassemble -s %#x -e %#x 0" % (addr, addr + 0x10))
         self.expect(
             "\^done,asm_insns=\[{address=\"0x0*%x\",func-name=\"main\",offset=\"0\",size=\"[1-9]+\",inst=\".+?\"}," %
             addr)
 
+        # Test -data-disassemble with source line information
+        self.runCmd("-data-disassemble -s %#x -e %#x -- 1" % (addr, addr + 0x10))
+        self.expect(
+            '\^done,asm_insns=\[src_and_asm_line={line="\d+",file="main.cpp",'
+            'line_asm_insn=\[{address="0x0*%x",func-name="main",offset="0",size="[1-9]+",inst=".+?"}\],'
+            'fullname="%s"}' %
+            (addr, os.path.abspath("main.cpp")) )
+
         # Run to hello_world
         self.runCmd("-break-insert -f hello_world")
         self.expect("\^done,bkpt={number=\"2\"")
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # Get an address for disassembling: use hello_world

lldb/tools/lldb-mi/MICmdCmdData.cpp

for (size_t i = 0; i < nInstructions; i++) {
     const CMICmnMIValueResult miValueResult5("inst", miValueConst5);
     miValueTuple.Add(miValueResult5);
 
     if (nDisasmMode == 1) {
       lldb::SBLineEntry lineEntry = address.GetLineEntry();
       const MIuint nLine = lineEntry.GetLine();
       const char *pFileName = lineEntry.GetFileSpec().GetFilename();
       pFileName = (pFileName != nullptr) ? pFileName : pUnknown;
+      // Get a full path to the file.
+      char pathBuffer[PATH_MAX];
+      lineEntry.GetFileSpec().GetPath(pathBuffer, PATH_MAX);
 
-      // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ]}"
+      // MI "src_and_asm_line={line=\"%u\",file=\"%s\",line_asm_insn=[ ],
+      // fullname=\"%s\"}"
       const CMICmnMIValueConst miValueConst(
           CMIUtilString::Format("%u", nLine));
       const CMICmnMIValueResult miValueResult("line", miValueConst);
       CMICmnMIValueTuple miValueTuple2(miValueResult);
       const CMICmnMIValueConst miValueConst2(pFileName);
       const CMICmnMIValueResult miValueResult2("file", miValueConst2);
       miValueTuple2.Add(miValueResult2);
       const CMICmnMIValueList miValueList(miValueTuple);
       const CMICmnMIValueResult miValueResult3("line_asm_insn", miValueList);
       miValueTuple2.Add(miValueResult3);
+      const CMICmnMIValueConst miValueConst5(pathBuffer);
+      const CMICmnMIValueResult miValueResult5("fullname", miValueConst5);
+      miValueTuple2.Add(miValueResult5);
       const CMICmnMIValueResult miValueResult4("src_and_asm_line",
                                                miValueTuple2);
       m_miValueList.Add(miValueResult4);
     } else {
       m_miValueList.Add(miValueTuple);
     }
   }