Page MenuHomePhabricator

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

Authored by anton.kolesov on Mar 6 2019, 1:51 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

anton.kolesov created this revision.Mar 6 2019, 1:51 AM
anton.kolesov edited the summary of this revision. (Show Details)Mar 6 2019, 1:52 AM
abidh accepted this revision.Mar 25 2019, 4:39 AM

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

This revision is now accepted and ready to land.Mar 25 2019, 4:39 AM

Added a simple test case.

clayborg requested changes to this revision.Apr 5 2019, 7:06 AM
clayborg added a subscriber: clayborg.
clayborg added inline comments.
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #193863)

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

This revision now requires changes to proceed.Apr 5 2019, 7:06 AM

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 added inline comments.Apr 9 2019, 1:42 PM
lldb/tools/lldb-mi/MICmdCmdData.cpp
420 ↗(On Diff #194376)

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 added inline comments.Apr 16 2019, 7:42 AM
lldb/tools/lldb-mi/MICmdCmdData.cpp
420 ↗(On Diff #194376)

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

clayborg added inline comments.Apr 16 2019, 7:56 AM
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #194376)

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

char pPathBuffer[PATH_MAX];
anton.kolesov added inline comments.Apr 22 2019, 3:46 AM
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #194376)

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 added inline comments.Apr 22 2019, 7:05 AM
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #194376)

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 added inline comments.Apr 26 2019, 6:04 AM
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #194376)

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

clayborg added inline comments.Apr 26 2019, 2:03 PM
lldb/tools/lldb-mi/MICmdCmdData.cpp
419 ↗(On Diff #194376)

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

Allocate path buffer on stack instead of heap.

clayborg accepted this revision.Apr 29 2019, 9:49 AM

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.

This revision is now accepted and ready to land.Apr 29 2019, 9:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 21, 6:20 AM