[windows][lldb] implement system logging on Windows #11017
Conversation
Before
After
|
|
@swift-ci please test |
|
@swift-ci please test |
lldb/source/Host/windows/Host.cpp
Outdated
| } | ||
|
|
||
| WORD event_type; | ||
| WORD event_id; |
There was a problem hiding this comment.
It used to be set in the switch case but I forgot to remove the declaration. Fixed!
|
@swift-ci please test |
lldb/source/Host/windows/Host.cpp
Outdated
| return nullptr; | ||
| const int length = strlen(ansi); | ||
| const int unicode_length = | ||
| MultiByteToWideChar(CP_ACP, 0, ansi, length, nullptr, 0); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If we do this I would add a comment to this function saying it expects a null-terminated string
There was a problem hiding this comment.
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.
lldb/source/Host/windows/Host.cpp
Outdated
|
|
||
| class WindowsEventLog { | ||
| public: | ||
| WindowsEventLog() { handle = RegisterEventSource(nullptr, L"LLDB"); } |
There was a problem hiding this comment.
| WindowsEventLog() { handle = RegisterEventSource(nullptr, L"LLDB"); } | |
| WindowsEventLog() : handle(RegisterEventSource(nullptr, L"LLDB")) {} |
lldb/source/Host/windows/Host.cpp
Outdated
| LPCWSTR wide_message = AnsiToUtf16(message.str().c_str()); | ||
|
|
||
| if (!h || !wide_message) { | ||
| switch (severity) { |
There was a problem hiding this comment.
if !h but we allocated wide_message, then we would leak memory. Can we make the UTF-16 string a unique_ptr?
The implementation of However I think that the comment is not accurate anymore: there is an implementation for Darwin through |
Yea it would make sense to have this upstream. And yea i think that comment is stale |
The linux implementation was removed 9 months ago because the use of system log is too heterogeneous on Linux, from this comment. Windows logging was made a NOOP upstream here: llvm#112052. It seems like Linux and Windows system logging were made NOOPs less than a year ago. Should we revert both of them (or at least Windows), now that we can log to the System's Log on Windows? |
|
Should this really do to the event logger? Are these actionable by the system administrators? I think it's better to push these into the lossy logger like the kernel debug log. Use |
The Event Viewer on Windows seems like the closest thing to System Logs on macOS and syslog.h utilities on Linux, which are already implemented. It was suggested in this comment.
It would display the same messages that get displayed on Linux and macOS (i.e swift version, etc). They are very useful on macOS.
I'm not sure I understand what you mean by kernel debug log. Is that in lldb?
From my understanding of the windows documentation, this function passes the output to the debugger, not the system logs or an equivalent. If I understand correctly, this is a major difference with the already implemented behaviors on Linux and macOS. |
Yes, if there's an appropriate place for the system log to go, then we should definitely use that upstream. I only removed it because the things we wanted to log to the system log would have caused too much disruption when printing to stdio. |
This patch makes LLDB use system logging on Windows rather than piping to the standard output.
Darwin, Linux and FreeBSD based systems pipe the health logs to system events. On Windows that's not the case, everything is redirected to the standard output/error. This leads to confusing diagnostics messages when running the REPL for example. This is fixes with this PR.
rdar://156039517