[lldb] Make sure that a Progress "completed" update is always reported at destruction

[royitaqi]

Make all Progress destructions to cause progressEnd events, regardless of the value of m_completed before the destruction.

Currently, a Progress instance with m_completed != 0 && m_complete != m_total will cause a progressUpdate event (not progressEnd) at destruction (see Process.cpp and lldb-dap/ProgressEvent.cpp). This contradicts with the classdoc:

a progress completed update is reported even if the user doesn't explicitly cause one to be sent.

See this discourse discussion for more details.

Test

Confirmed that the tests pass with the patch, and don't pass without the patch.

ninja lldb-unit-test-deps
tools/lldb/unittests/Core/LLDBCoreTests --gtest_filter="Progress*"

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Currently, a Progress instance with m_completed != 0 && m_complete != m_total will cause a progressUpdate event (not progressEnd) at destruction. This contradicts with the classdoc:
> a progress completed update is reported even if the user doesn't explicitly cause one to be sent.

Changing it so that any Progress destruction will cause an progressEnd event regardless of the value of m_completed before destruction.

See this discourse discussion for more details.

---

Full diff: https://github.com/llvm/llvm-project/pull/102097.diff

1 Files Affected:

• (modified) lldb/source/Core/Progress.cpp (+1-2)

diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 1a779e2ddf924..e0ba1a63c508e 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -45,8 +45,7 @@ Progress::~Progress() {
   // Make sure to always report progress completed when this object is
   // destructed so it indicates the progress dialog/activity should go away.
   std::lock_guard<std::mutex> guard(m_mutex);
-  if (!m_completed)
-    m_completed = m_total;
+  m_completed = m_total;
   ReportProgress();
 
   // Report to the ProgressManager if that subsystem is enabled.

[clayborg]

Was this why we saw some Swift progress dialogs hanging around forever?

Looks ok to me. @JDevlieghere feel free to add any more reviewers that have been working on progress dialogs.

[JDevlieghere]

LGTM. Could we cover this scenario by the existing unit test?

[royitaqi]

LGTM. Could we cover this scenario by the existing unit test?

Yes, definitely.

At the time of the patch I didn't find any existing unit test files by searching for "ProgressTest.cpp". Just now I realize there is the ProgressReportTest.cpp. I will read it and try to add a test for this scenario.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[royitaqi]

@JDevlieghere A unit test has been added. I have confirmed that it fails before the patch, and succeeds after the patch.

[JDevlieghere]

Thanks @royitaqi! Once the formatting is fixed this is ready to be merged.