Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
12 changes: 0 additions & 12 deletions lldb/source/Host/common/Host.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,6 @@ void Host::SystemLog(Severity severity, llvm::StringRef message) {
}
syslog(level, "%s", message.data());
}
#else
void Host::SystemLog(Severity severity, llvm::StringRef message) {
switch (severity) {
case lldb::eSeverityInfo:
case lldb::eSeverityWarning:
llvm::outs() << message;
break;
case lldb::eSeverityError:
llvm::errs() << message;
break;
}
}
#endif
#endif

Expand Down
67 changes: 67 additions & 0 deletions lldb/source/Host/windows/Host.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/StructuredData.h"

#include "llvm/ADT/StringRef.h"
#include "llvm/Support/ConvertUTF.h"
#include "llvm/Support/ManagedStatic.h"

// Windows includes
#include <tlhelp32.h>
#include <windows.h>

using namespace lldb;
using namespace lldb_private;
Expand Down Expand Up @@ -302,3 +305,67 @@ Environment Host::GetEnvironment() {
}
return env;
}

class WindowsEventLog {
public:
WindowsEventLog() { handle = RegisterEventSource(nullptr, L"LLDB"); }

Choose a reason for hiding this comment

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

Suggested change
WindowsEventLog() { handle = RegisterEventSource(nullptr, L"LLDB"); }
WindowsEventLog() : handle(RegisterEventSource(nullptr, L"LLDB")) {}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks 👍


~WindowsEventLog() {
if (handle)
DeregisterEventSource(handle);
}

HANDLE GetHandle() const { return handle; }

private:
HANDLE handle;
};

static llvm::ManagedStatic<WindowsEventLog> event_log;

static LPCWSTR AnsiToUtf16(const char *ansi) {
if (!ansi)
return nullptr;
const int length = strlen(ansi);
const int unicode_length =
MultiByteToWideChar(CP_ACP, 0, ansi, length, nullptr, 0);

Choose a reason for hiding this comment

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

We can omit length entirely here and replace it with -1. Apparently that tells MultiByteToWideChar that the string is null-terminated, and it will add the null-terminator itself.

So you don't need to reserve that extra + 1 length and don't need to null-terminate on line `334.

Concretely, could we just change the length parameters in both calls to MultiByteToWideChar to -1?

Tbf, I'm not super familiar with these APIs

Choose a reason for hiding this comment

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

If we do this I would add a comment to this function saying it expects a null-terminated string

Copy link
Author

Choose a reason for hiding this comment

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

I've reused the snippet in llvm-project/third-party/unittest/googletest/src/gtest.cc. I imagine that it assumed the char * could be missing a null terminator.

I've changed the cbMultiByte parameter to take a -1 instead of the length and removed the +1 as well.

This works fine when testing on Windows.

WCHAR *unicode = new WCHAR[unicode_length + 1];
MultiByteToWideChar(CP_ACP, 0, ansi, length, unicode, unicode_length);
unicode[unicode_length] = 0;
return unicode;
}

void Host::SystemLog(Severity severity, llvm::StringRef message) {
HANDLE h = event_log->GetHandle();
LPCWSTR wide_message = AnsiToUtf16(message.str().c_str());

if (!h || !wide_message) {
switch (severity) {
Copy link

@Michael137 Michael137 Jul 17, 2025

Choose a reason for hiding this comment

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

if !h but we allocated wide_message, then we would leak memory. Can we make the UTF-16 string a unique_ptr?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks 👍

case lldb::eSeverityInfo:
case lldb::eSeverityWarning:
llvm::outs() << message;
return;
case lldb::eSeverityError:
llvm::errs() << message;
return;
}
}

WORD event_type;

switch (severity) {
case lldb::eSeverityWarning:
event_type = EVENTLOG_WARNING_TYPE;
break;
case lldb::eSeverityError:
event_type = EVENTLOG_ERROR_TYPE;
break;
case lldb::eSeverityInfo:
default:
event_type = EVENTLOG_INFORMATION_TYPE;
}

ReportEventW(h, event_type, 0, 0, nullptr, 1, 0, &wide_message, nullptr);

delete[] wide_message;
}