Page MenuHomePhabricator

Preliminary patch to support prompt interpolation
Changes PlannedPublic

Authored by nealsid on Jul 28 2018, 9:50 PM.

Details

Summary

The infrastructure for interpolating format strings for the stacks and frames is very useful, so I thought it would also be useful to support this in interpolating the prompt string. So you can define a prompt like this:

(lldb) settings set prompt "${process.file.basename} T:${thread.index}/${thread.count} F:${frame.index1based}/${frame.count}) "

And the next prompt would be something like this:

casn T:1/1 F:1/1)

This is very preliminary as I am new to the LLDB code base, so I would welcome any and all feedback and suggestions. I plan to write tests and flesh out the error handling but wanted to get some eyes on this to help guide the remainder of the work, if people think it's useful. If not, I'm happy to tackle something else that could be more useful to the project.

Diff Detail

Repository
rL LLVM

Event Timeline

nealsid created this revision.Jul 28 2018, 9:50 PM
nealsid edited subscribers, added: lldb-commits; removed: llvm-commits.Jul 28 2018, 9:53 PM
zturner added a subscriber: zturner.
zturner added inline comments.
include/lldb/Host/Editline.h
187

I'd love to stop using the baton idiom if possible. can you make this function take an llvm::function_ref<StringRef (EditLine*)> instead? Then, in the class, store a std::function<StringRef (EditLine*)>. When you call SetPromptCallback, write SetPromptCallback([this](EditLine* L) { return this->PromptCallback(L); });

include/lldb/Interpreter/PromptInterpolation.h
27

As it stands, the class doesn't really do much. It's basically just one function, and the only purpose of the m_debugger member is to avoid having to pass the Debugger object as an argument to the function. It seems this could just be turned into a free function. And while we're at it, since that free function is only ever used inside of IOHandler.cpp, it can probably just be marked static and be a global function defined in that particular TU.

source/Core/IOHandler.cpp
451

This can return a StringRef

This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by making sure the frame and thread are not filled in when the process is running. It might also be racy. For example if you say "continue", hopefully the process will be resumed by the time the prompt is asked to refresh itself. Since we get events asynchronously this might be a problem.

include/lldb/Core/FormatEntity.h
86

Rename to FrameIndexID. It is similar to the thread index ID which is one based.

include/lldb/Core/IOHandler.h
456

If this will only contain a prompt that is a format string, you should store the compiled format so we don't have to parse it each time the prompt is displayed. Maybe this should be:

FormatEntity::Entry m_prompt_format;
include/lldb/Host/Editline.h
93

Remove whitespace

source/Core/FormatEntity.cpp
125

rename to "index_id" and fix enum to FrameIndexID

357

FrameIndexID

1443

FrameIndexID

source/Interpreter/PromptInterpolation.cpp
1

Not sure this file is needed?

33–46

Remove all of this and just call:

ExecutionContext exe_ctx = m_debugger.GetCommandInterpreter().GetExecutionContext();

It will do the right thing and leave the thread and frame out of the context if we are running. There is no need to fallback to using kFallbackPrompt. You can make your prompt hardened against values not being available. Lets say the thread count isn't available: you can just put extra {} around the string that contains the thread count like:

"{num threads = ${thread.count}}"

If any variable inside the extra scope is not available, it will be left out. So there should be no need to use a fallback if the prompt is made correctly.

50

This should be parsed once and kept around. Not parsed every time the prompt is displayed.

labath added a comment.Aug 1 2018, 1:33 AM

Sounds like a nice feature to have. In addition to the other feedback you've received, I'd suggest splitting out the addition of new format entities (frame count and friends) and the core interpolation feature into separate patches.

nealsid accepted this revision.Aug 3 2018, 1:47 PM
nealsid added inline comments.
include/lldb/Host/Editline.h
187

Sounds good to me. I'll split up the changes and sent a patch that migrates to std::function & removes baton from all callbacks in Editline first and then rebase this one on top of that.

This revision is now accepted and ready to land.Aug 3 2018, 1:47 PM
nealsid planned changes to this revision.Aug 3 2018, 1:49 PM

This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by making sure the frame and thread are not filled in when the process is running. It might also be racy. For example if you say "continue", hopefully the process will be resumed by the time the prompt is asked to refresh itself. Since we get events asynchronously this might be a problem.

Nice - TBH, I haven't used LLDB in awhile so having the prompt displayed while the target is running wasn't on my list of test cases, but I'll definitely add it.
Perhaps there could also be an indicator like '*' in the prompt when the process is currently running so the user will know it's potentially out of date.