Page MenuHomePhabricator

Mark the current column when displaying the context of the current breakpoint on the terminal.
ClosedPublic

Authored by tfiala on May 31 2016, 2:11 PM.

Details

Summary

Here's a fun little idea plus a preliminary patch implementing it.

When I'm debugging programs I often wonder what exactly will happen when I step-in. This is particularly a problem with code that has lots of control flow happening on a single line, such as a C++ for loop with iterators, a function call with a lambda or block definition, or plain old nested function calls.

For example, let's say I'm stopped inside lldb in SourceManager.cpp:93 — I would really like to know which function I'll be stepping into next. Will the next "step" take me into get(), new, File(), or reset()?

   90  	    // If file_sp is no good or it points to a non-existent file, reset it.
   91  	    if (!file_sp || !file_sp->GetFileSpec().Exists())
   92  	    {
-> 93  	        file_sp.reset (new File (file_spec, target_sp.get()));
   94  	
   95  	        if (debugger_sp)
   96  	            debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
(lldb) step

Of course a debugger cannot predict the future, but what it can do is tell me exactly where I am stopped now!
Compilers like clang already include column information in the debug info by default. The attached patch makes use of this by adding an underline attribute to the character on the current column to indicate the exact breakpoint on the current line (here simulated with a caret):

   90      // If file_sp is no good or it points to a non-existent file, reset it.
   91      if (!file_sp || !file_sp->GetFileSpec().Exists())
   92      {
-> 93          file_sp.reset (new File (file_spec, target_sp.get()));
                                  ^
   94  
   95          if (debugger_sp)
   96              debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
(lldb) step

With this markup I may now assume that "step" will take me into the File() constructor. Just what I wanted to know.

This is of course just scratching the surface of what we could do with column information, but probably a good starting point. Having a more fine-grained visualization, for example, it might be interesting to have "next" take me to the next "is_stmt" in the line table instead of always to the next line, and so on.

Let me know what you think!

Diff Detail

Event Timeline

aprantl updated this revision to Diff 59117.May 31 2016, 2:11 PM
aprantl retitled this revision from to Mark the current column when displaying the context of the current breakpoint on the terminal..
aprantl updated this object.
aprantl added reviewers: jingham, clayborg.
aprantl set the repository for this revision to rL LLVM.
aprantl added a subscriber: lldb-commits.
jingham requested changes to this revision.May 31 2016, 2:57 PM
jingham edited edge metadata.

This looks fine. Can you add a setting in the "stop-line" vein that will turn on and off the column listing. Seems odd to control all the other stuff, but not this.

This revision now requires changes to proceed.May 31 2016, 2:57 PM

I was also wondering if there was a preferred way to detect whether LLDB is connected to a dumb terminal. I noticed that Stream has an eANSI flag, but it seemed to be always zero.

aprantl updated this revision to Diff 59141.May 31 2016, 3:51 PM
aprantl edited edge metadata.
aprantl removed rL LLVM as the repository for this revision.
aprantl added a subscriber: friss.

Added a use-column-info setting.

jingham requested changes to this revision.May 31 2016, 4:29 PM
jingham edited edge metadata.

I wouldn't call the setting "use-column-info". It would be better to call it "stop-print-column-info" or something that makes it clear this is just for printing. If we get better column info & start using it for stepping, etc, we might want to provide a way to turn that off too, but independently from this stop-info printing.

This revision now requires changes to proceed.May 31 2016, 4:29 PM
clayborg edited edge metadata.May 31 2016, 4:30 PM

"display-column-info"?

All the other controls for printing the stop info have "stop-*". It would be clearer to keep doing this, I think.

Jim

aprantl updated this revision to Diff 59154.May 31 2016, 4:56 PM
aprantl edited edge metadata.

Rename the property to "stop-show-column" and query the stream whether ansi escape sequences are allowed.

jingham accepted this revision.May 31 2016, 5:44 PM
jingham edited edge metadata.

Looks fine to me.

This revision is now accepted and ready to land.May 31 2016, 5:44 PM
aprantl updated this revision to Diff 59264.Jun 1 2016, 12:09 PM
aprantl edited edge metadata.

I have one last question:
While trying to add a testcase I noticed that DisplaySourceLinesWithLineNumbers is still exported into Python with the old signature. What is the correct way to update the Python bindings?

tfiala added a subscriber: tfiala.Aug 12 2016, 12:51 PM

I'm going to help Adrian land this patch.

I'm looking into this now.

I have one last question:
While trying to add a testcase I noticed that DisplaySourceLinesWithLineNumbers is still exported into Python with the old signature. What is the correct way to update the Python bindings?

On the LLVM.org side, the bindings are usually updated fine if all the swig input files are updated (so the .i files I think would be relevant here).

This will need another change.

Existing SBAPI methods cannot have their signatures modified per the SBAPI contract. We'll need to add an entirely new method on SBSourceManager with the new signature.

I'll just add newer version of DisplaySourceLinesWithLineNumbers() with a slightly modified name, maybe:
DisplaySourceLinesWithLineNumbersAndColumnInfo(...).

aprantl abandoned this revision.Aug 17 2016, 8:58 AM

I'm abandon this review so tfiala can claim it.

tfiala commandeered this revision.Aug 18 2016, 11:35 PM
tfiala added a reviewer: aprantl.

Grabbing it.

I had to get that DarwinLog support wrapped up (it was in process for something like a month). Now that I wrapped that up, I'll get back to this.

tfiala reclaimed this revision.Sep 12 2016, 11:21 AM

Reclaiming. I'll be working on this now.

This revision is now accepted and ready to land.Sep 12 2016, 11:21 AM

This will unfortunately get a little messy due to the code reformatting. My first patch up for it will be to get the existing impl refreshed for the code reformatting.

tfiala updated this revision to Diff 71180.Sep 13 2016, 8:59 AM

Updated patch, tweaked and verified it is working, rebased against r281243.

I'd like to add some unittests for this, so this isn't ready quite yet.

tfiala updated this revision to Diff 71253.EditedSep 13 2016, 4:14 PM

This change does the following:

  • Removes storing the ANSI escape support flag on the stream and directly consults the Debugger::GetUseColor() method. We should be able to change this during a debug session (via settings set use-color {true|false}) and have it take effect immediately.
  • Adds a test case for the use-color case. (The previous change only had a test for the no-color case).

This is ready for a proper review.

tfiala updated this revision to Diff 71262.Sep 13 2016, 4:34 PM

Minor tweak:

  • Shut off color usage in the shared object unloading test. One of the screen-scraped results it is checking for is not set up to handle the ansi underline sequence on the column where the process is stopped.
jingham requested changes to this revision.Sep 13 2016, 4:49 PM
jingham edited edge metadata.

See inlined comments. Otherwise this is great.

include/lldb/Core/SourceManager.h
34–36

A target always has a debugger, so given a target you can always get to the debugger. If you have another use case where you need to make this File, but only have a DebuggerSP, can you make a constructor that takes a FileSpec & a DebuggerSP, but not a target? That seems clearer to me.

source/Commands/CommandObjectSource.cpp
1155–1158

I think I will occasionally want to see the source listing w/o the column stop info (e.g. to cut & paste it somewhere) so it might be convenient to have a command option that overrides the setting. It's always annoying to have to go change & reset a setting to do this, plus then I can't make aliases for the different behaviors.

This revision now requires changes to proceed.Sep 13 2016, 4:49 PM

See inlined comments. Otherwise this is great.

Great, I'll fix those up. I like the command-line option idea.

Greg had a few pieces of feedback as well that we just discussed:

  • Change the on/off switch for column marking to support the following states:
    • terminal-code-only (i.e. only do terminal code highlighting, not the text caret)
    • terminal-code + text caret
    • text caret only
    • off
  • Add a pre and post terminal-code-format format string option for the marker. This allows the user to modify what kind of highlighting the user can do. It will default to the way I'm doing it now, which is ansi underline.
tfiala added inline comments.Sep 14 2016, 9:24 AM
packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
163

Upon further reflection, there is a logically more appropriate approach. The better one is to explicitly turn off column marking and not count on the fact that the column marker happens on the line below the stop match text. (This test initially failed because the stop text that is scraped was getting confused by the newly-added ansi escape sequences around the first character).

Coming back to this today.

tfiala updated this revision to Diff 71996.EditedSep 20 2016, 4:07 PM
tfiala edited edge metadata.

This change addresses Jim's and Greg's suggestions.

  • stop-show-column now is a quad-state:
    • ansi-or-caret (default): Shows ANSI if color is enabled; otherwise, uses the text-based caret.
    • ansi-only: ANSI highlighting is used if color is enabled; otherwise, no column marking.
    • caret-only: text-based caret on the following line will always be used when showing stop column.
    • none: no column marking is done.
  • stop-show-column-decorator-pre: this is a format entity (with tab completion for ${ansi.*} codes). It defaults to the start code for underlining (${ansi.underline}), the default column stop marker display style when using ANSI highlighting.
  • stop-show-column-decorator-post: this is the format entity for disabling whatever was enabled in the "-pre" setting. This defaults to ${ansi.normal}. This would need to be changed if color codes were used for background/foreground of anything other than inverse.
  • other minor odds and ends, like changing the constructor of SourceManager::File to either take a Target or a Debugger, but not both.
tfiala marked an inline comment as done.Sep 20 2016, 9:59 PM
tfiala added inline comments.
source/Core/Disassembler.cpp
292–294

I will leave this for a potential follow-up patch at another time. At that point, I would suggest enabling stop line and column marking on the "source list" command, which currently doesn't mark up a displayed line that coincides with any currently-stopped thread.

I'll strip out this TODO on the final check-in.

clayborg requested changes to this revision.Sep 21 2016, 9:45 AM
clayborg edited edge metadata.

A few naming requests.

source/Core/Debugger.cpp
151

Make this "ansi" instead of "ansi-only"?

154

Make this "caret" instead?

198

Make this "stop-show-column-ansi-prefix"?

203

Make this "stop-show-column-ansi-suffix"?

This revision now requires changes to proceed.Sep 21 2016, 9:45 AM
tfiala updated this revision to Diff 72084.Sep 21 2016, 10:32 AM
tfiala edited edge metadata.

Updates for Greg's latest comments.

clayborg accepted this revision.Sep 21 2016, 10:35 AM
clayborg edited edge metadata.

Looks good.

tfiala accepted this revision.Sep 23 2016, 9:20 AM
tfiala added a reviewer: tfiala.

(I'll close this out as soon as Jim accepts).

jingham accepted this revision.Sep 23 2016, 10:25 AM
jingham edited edge metadata.
This revision is now accepted and ready to land.Sep 23 2016, 10:25 AM
tfiala closed this revision.Sep 23 2016, 2:37 PM

Thanks, Jim!