Skip to content

Fix inconsistent newline format when logging exceptions to console. #114692

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lateralusX
Copy link
Member

The introduction of minipal_log_write API's have flushed out some inconsistency in newline format used on Windows. Previously the Windows implementation logged exceptions to console using WriteFile together with STD_ERR_HANDLE. It turns out that the file mode of this handle is binary, while the file mode for regular stdout/stderr is text mode. minipal_log_write API uses underlying _write instead of WriteFile and the fd associated with stdout/stderr, but since they are in text mode, pre-formatted data intended for binary output will expand \r\n to \r\r\n that could cause side effects due to additional \r character in the stream. The use of WriteFile handled this case, but we also mixed regular \n using the same API, that won't be expanded to \r\n when written to a file in binary mode leaving us with output using different line endings through the same logging.

Normally all logging performed from native code use \n and expects it to be replaced to \r\n on Windows as part of logging through a file in text mode. Some data formatted in managed code intended for output is already pre-formatted, using \r\n, intended for files in binary mode. One area where we mix line ending formats is the logging of unhandled exception call stacks, where we currently mix formatted newlines \r\n with \n.

This fix implement support in minipal_log_write to accept a new flag indicating that logging should be done through binary file mode, and since this is the default on none Windows platforms its a noop on all platforms except Windows. All logging that currently logs strings coming from managed code will be logged using binary mode, making sure pre-formatted \r\n won't be re-formatted.

Fixes #114579

The introduction of minipal_log_write API's have flushed out some
inconsistency in newline format used on Windows. Previously the
Windows implementation logged exceptions to console using WriteFile
together with STD_ERR_HANDLE. It turns out that the file mode of this
handle is binary, while the file mode for regular stdout/stderr is
text mode. minipal_log_write API uses underlying _write instead of
WriteFile and the fd associated with stdout/stderr, but since they
are in text mode, pre-formatted data intended for binary output
will expand \r\n to \r\r\n that could cause side effects due to
additional \r character in the stream. The use of WriteFile handled
this case, but we also mixed regular \n using the same API, that
won't be expanded to \r\n when written to a file in binary mode
leaving us with output using different line endings through the same
logging.

Normally all logging performed from native code use \n and expects
it to be replaced to \r\n on Windows as part of logging through
a file in text mode. Some data formatted in managed code intended for
output is already pre-formatted, using \r\n, intended for files in
binary mode. One area where we mix line ending formats is the logging
of unhandled exception call stacks, where we currently mix formatted
newlines \r\n with \n.

This fix implement support in minipal_log_write to accept a new flag
indicating that logging should be done through binary file mode,
and since this is the default on none Windows platforms its a noop on
all platforms except Windows. All logging that currently logs strings
coming from managed code will be logged using binary mode, making sure
pre-formatted \r\n won't be re-formatted.
@Copilot Copilot bot review requested due to automatic review settings April 15, 2025 15:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes inconsistent newline formatting when logging exceptions on Windows by introducing a new binary logging mode flag.

  • Adds a new flag (minipal_log_flags_binary) and related macros in log.h
  • Updates logging functions in log.c to choose between binary (WriteFile) and text (_write) logging
  • Modifies PrintToStdErr functions in util.hpp/util.cpp and related calls in excep.cpp and eepolicy.cpp to support binary logging mode

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/native/minipal/log.h Introduces binary flag and macros for flag manipulation
src/native/minipal/log.c Refactors logging functions to use the new binary flag appropriately
src/coreclr/vm/util.hpp Updates PrintToStdErr function signatures to accept a binary flag
src/coreclr/vm/util.cpp Implements changes in PrintToStdErr functions using the new flag
src/coreclr/vm/excep.cpp Adjusts exception logging to use binary mode for pre-formatted data
src/coreclr/vm/eepolicy.cpp Updates logging calls to pass the binary flag where appropriate
Comments suppressed due to low confidence (1)

src/coreclr/vm/excep.cpp:4798

  • [nitpick] Consider explicitly specifying the binaryOutputMode parameter in the PrintToStdErrA call (e.g., PrintToStdErrA("\n", TRUE)) for consistency with the preceding PrintToStdErrW call that uses binary output mode, unless the default behavior is intentional.
PrintToStdErrA("
");

#define minipal_log_flags_append(flags, flag) (flags = (minipal_log_flags)(flags | flag))
#define minipal_log_flags_remove(flags, flag) (flags = (minipal_log_flags)(flags & ~flag))
#define minipal_log_flags_get_level(flags) ((minipal_log_flags)(flags & ~(minipal_log_flags_binary)))
#define minipal_log_flags_is_binary(flags) ((flags) & minipal_log_flags_binary)
Copy link
Preview

Copilot AI Apr 15, 2025

Choose a reason for hiding this comment

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

Add a clarifying comment that explains the purpose of minipal_log_flags_binary: it indicates that logging should bypass newline translation on Windows by using binary file mode.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@lateralusX
Copy link
Member Author

Looking into taking this fix closer to minipal_log_write internals, stay tuned.

@lateralusX lateralusX marked this pull request as draft April 16, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Android logging support has broken a diagnostics exception handling test
1 participant