Skip to content

[lldb-dap] Ensure we do not print the close sentinel when closing stdout. #126833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashgti merged 3 commits into from
Feb 13, 2025
Merged

[lldb-dap] Ensure we do not print the close sentinel when closing stdout. #126833

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af07cba
[lldb-dap] Ensure we do not print the close sentinel when closing std…
ashgti Feb 12, 2025
941df40
Applying reviewer suggestions and updating the existing tests to ensu…
ashgti Feb 12, 2025
cf6f4c9
Update lldb/tools/lldb-dap/OutputRedirector.cpp
ashgti Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions lldb/tools/lldb-dap/OutputRedirector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "DAP.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include <cstring>
#include <system_error>
#if defined(_WIN32)
#include <fcntl.h>
Expand All @@ -25,6 +26,10 @@ using llvm::Expected;
using llvm::inconvertibleErrorCode;
using llvm::StringRef;

namespace {
char kCloseSentinel[] = "\0";
} // namespace

namespace lldb_dap {

int OutputRedirector::kInvalidDescriptor = -1;
Expand Down Expand Up @@ -59,7 +64,10 @@ Error OutputRedirector::RedirectTo(std::function<void(StringRef)> callback) {
while (!m_stopped) {
ssize_t bytes_count = ::read(read_fd, &buffer, sizeof(buffer));
// EOF detected.
if (bytes_count == 0)
if (bytes_count == 0 ||
Copy link
Collaborator

@labath labath Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't make this an inline suggestion, but this would flow better if you replaced this with the following code after the -1 check:

StringRef data(buffer, bytes_count);
if (data.empty() || (m_stopped && data == kCloseSentinel)) break;

I'm also noting this will not always remove the sentinel as it can end up attached to some other output. That could be handled by something like

if (m_stopped) data = data.consume_back(kCloseSentinel);
if (data.empty()); break;

.. but that has the potential to remove an unrelated piece of text. Since neither of them is perfect, I'll leave it up to you to choose.

Copy link
Contributor Author

@ashgti ashgti Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the second suggestion and added some tests to try to validate we don't incorrectly consume any null bytes produced by the debuggee.

(bytes_count == sizeof(kCloseSentinel) &&
std::memcmp(buffer, kCloseSentinel, sizeof(kCloseSentinel)) == 0 &&
m_fd == kInvalidDescriptor))
break;
if (bytes_count == -1) {
// Skip non-fatal errors.
Expand All @@ -85,8 +93,7 @@ void OutputRedirector::Stop() {
// Closing the pipe may not be sufficient to wake up the thread in case the
// write descriptor is duplicated (to stdout/err or to another process).
// Write a null byte to ensure the read call returns.
char buf[] = "\0";
(void)::write(fd, buf, sizeof(buf));
(void)::write(fd, kCloseSentinel, sizeof(kCloseSentinel));
::close(fd);
m_forwarder.join();
}
Expand Down