This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Core] Fix "sticky" long progress messages
ClosedPublic

Authored by mib on May 2 2022, 10:40 AM.

Details

Summary

When the terminal window is too small, lldb would wrap progress messages
accross multiple lines which would break the progress event handling
code that is supposed to clear the message once the progress is completed.

This causes the progress message to remain on the screen, sometimes partially,
which can be confusing for the user.

To fix this issue, this patch trims the progress message to the terminal
width taking into account the progress counter leading the message for
finite progress events and also the trailing ....

rdar://91993836

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.May 2 2022, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 10:40 AM
mib requested review of this revision.May 2 2022, 10:40 AM
mib edited the summary of this revision. (Show Details)May 2 2022, 10:42 AM

Can we add a pexpect test for this?

lldb/source/Core/Debugger.cpp
1873–1875

Should this account for the control characters? Do they cause a line wrap?

mib added inline comments.May 2 2022, 12:37 PM
lldb/source/Core/Debugger.cpp
1873–1875

In my understanding, if the control character is complete, it shouldn't print anything to the screen so they shouldn't cause a line wrap

mib updated this revision to Diff 427692.May 6 2022, 11:35 AM

Add test

JDevlieghere added inline comments.May 6 2022, 11:41 AM
lldb/source/Core/Debugger.cpp
1873–1875

Instead of computing the length string like this, would it make sense to write the message to a temporary stream/buffer and then trim the string dumping it to the real async output stream? I'm worried about the two accidentally getting out of sync.

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py
25–34

I don't understand how this guarantees that we cut off the string at the exact width of the window?

JDevlieghere added inline comments.May 18 2022, 12:21 PM
lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py
30–31

I understand the motivation, but I think it's a bad idea to intentionally introduce non-determinism in a test. Let's just make this a constant or repeat the test for two (fixed) widths.

37–60

Why not use self.child.expect_exact with the exact sequence (including escape characters) that you're trying to match? This seems to reimplementing part of the pexpect functionality and I'm not sure we actually need it.

mib updated this revision to Diff 430548.May 18 2022, 6:11 PM
mib marked 5 inline comments as done.

Address @JDevlieghere comments

JDevlieghere added inline comments.May 18 2022, 6:15 PM
lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py
50–51

These two things are pretty Apple specific so maybe use the skipUnlessDarwin decorator? I think the locating symbol files will be printed on all platforms, but if we need to skip the test on Linux anyway that doesn't really buy us much to reduce the test coverage.

JDevlieghere accepted this revision.May 18 2022, 6:15 PM

LGTM with the decorator

This revision is now accepted and ready to land.May 18 2022, 6:15 PM
mib marked an inline comment as done.May 18 2022, 6:17 PM
mib added inline comments.
lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py
50–51

Indeed! Thanks!

mib updated this revision to Diff 430552.May 18 2022, 6:19 PM
mib marked an inline comment as done.

Only run test on Darwin platforms

This revision was automatically updated to reflect the committed changes.