Initial CoreClr Android logcat log integration. #113416
Conversation
There was a problem hiding this comment.
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
lateralusX
changed the title
|
Tagging subscribers to this area: @mangod9 |
lateralusX
force-pushed
the
lateralusX/hookup-coreclr-android-logging
branch
from
c50079c to
f0162da
Compare
What is an example of logging that you plan to convert? I think this PR covers it all. |
There are still printf's in utilcode and vm folder that could be hooked up to the minipal_log API's. |
|
If we want to capture PAL debug build logs in logcat, they are in |
I have a follow up PR that adjust additional files under utilcode, pal and vm folder, I make sure to add this one into that PR as well. |
|
CI failures seem to be known issues and fails on many different PR\s. are we happy with the changes so it can be approved and merged? |
|
Faliure in "Build Libraries Test Run release coreclr windows x64 Debug" seems to be due to a recently re-enabled flaky test, #103449, re-enabled by #113604. Error is present on other PR's as well, so unrelated. wasm failures are known test errors and not caused by this PR. @janvorli, ok with PR after updated based on your review feedback? |
| if (exitCode == (UINT)COR_E_FAILFAST) | ||
| { | ||
| PrintToStdErrA("Process terminated. "); | ||
| message.Append(W("Process terminated. ")); |
There was a problem hiding this comment.
One last thing. I would still prefer these two short messages to be printed separately to get at least these to the output when things are in a really bad state.
The rest looks good.
There was a problem hiding this comment.
OK, for Android that will mean they will end up on a separate log line compared to how things are on stdout, but maybe it would make sense to actually do them on separate line always, so always do "Process terminated.\n" even for stdout. That would make the output look a little different, so instead of:
Process terminated. {errorSource}
{errorMessage or errorCode}
It would show up like this:
Process terminated.
{errorSource if available}
{errorMessage or errorCode}
Initial CoreClr Android logcat integration was removed from cf2aae6. This PR adds a unified approach re-routing runtime console logging though a central log API defined in
native/minipal/log.hpotential shared between components as well as runtimes.This initial PR starts out defining a new header under
native/minipal/log.hthat includes a straightforward API to log formatted and raw output using different priority. It includes regularprintf/vprintfstyle functions as well as low level none crt directwritecapabilities, usable in high speed or async safe logging.API closely mimics Mono's logging API defined in
glib.hand the idea is to potentially rewire Mono's logging to use this new API going forward potentially adding more capabilities intonative/minipal/log.hAbility to log different log message using different priority is inherited from Mono and maps close to logcat priority levels, where different priorities can be filtered directly in logcat, creating virtual log streams. API includes a way to log directly to virtual standard error/output streams in case no specific need for fine grained priority-based logging exists. On platforms supporting stderr/stdout this will be a 1:1 mapping, but on platforms like logcat, stderr/stdout will be mapped to similar priority levels (error, info).
There is no buffering when calling logcat log functions, each log call will end up as a new log entry. Code that relies on ability to do multiple logging calls and expect to get a new line only when writing
\ninto the log buffer, won't behave as expected when using logcat. This PR adjust a couple of places where such assumptions are made, appending logging into a string buffer before passed tominipal_log_writeAPI's. Logcat has an internal buffer limitation of 4068 bytes, buffers larger than that will be truncated. To mitigate this logging targeting Android includes a mechanism splitting up the bigger buffer into smaller chunks based on\nin log message, if there is no\nin bigger log message, message will still be logged in chunks, but each chunk ending up as a separate logcat message. Internal chunk size has been defined to 4000 bytes, making room for future internal logcat buffer reduction. Regular console logging enforces a buffer size limit as well on low level write calls, this limitation was directly inherited from previous implementation insrc/coreclr/vm/util.cppintonative/minipal/log.c, if buffer exceeds chunk size, it will be split into multiple chunks where each chunk is written "as is" to underlyingwritefunction.On Mono, formatted print style logcat implementation uses dynamic memory allocation +
vsnprintfand low level__android_log_write. Current implementation innative/minipal/log.hadapt a similar schema but tries to first use a smaller stack buffer, inline with Androids internal implementation of__android_log_vprintuse of 1024 bytes buffer. If log message is below 1024, implementation will match__android_log_vprintand use stack memory, but if exceeding 1024, implementation fallbacks to dynamic memory allocation, format message into dynamic memory buffer and then pass it throughminipal_log_writeto correctly handle max payload limitations as described above.This initial PR adapts a smaller set of source files under
src/coreclrto use the new logging API as a first step adaption. It makes sure the LOG macro as well as exception handling gets emitted through the new log API ending up in logcat. PR also adapts jit logging as well as diagnostic servers pause message, currently written directly to console.This PR will be followed by an additional PR adapting more sources under
src/coreclr/vmto use the new logging API enabling more log messages to end up in logcat on Android. Once the initial pass is done, adaption to new logging API will probably happen on a case-by-case basis.