Remove private implementation of printf #3020
Conversation
nfbot
added
the
Type: dependencies
WalkthroughThe changes involve enhancements to logging functionality across several source files, specifically focusing on memory allocation and garbage collection processes. Key modifications include the introduction of conditional logging based on the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
💤 Files not reviewed due to no reviewable changes (11)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments not posted (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
josesimoes
force-pushed
the
update-printf
branch
from
7bdd964 to
295c762
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/CLR/Diagnostics/Info_Safeprintf.cpp (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (8 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.h (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf_cfg.h (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/CLR/Helpers/nanoprintf/nanoprintf.c
- src/CLR/Helpers/nanoprintf/nanoprintf_cfg.h
Additional comments not posted (3)
src/CLR/Diagnostics/Info_Safeprintf.cpp (1)
14-14: LGTM!Changing the type of
charsfrominttosize_tis the correct approach as it aligns with the return type ofvsnprintf. This change improves type safety and prevents potential issues related to signed versus unsigned integer comparisons.src/CLR/Helpers/nanoprintf/nanoprintf.h (2)
62-62: LGTM! The updatedsnprintf_signature enhances safety and functionality.The changes to the
snprintf_function signature are beneficial for the following reasons:
- The inclusion of the buffer size parameter
nhelps prevent buffer overflow vulnerabilities by limiting the number of characters written to the buffer.- The format string parameter
fmtenables type-safe formatting of the output, reducing the chances of format string vulnerabilities.- The variadic parameter
...allows passing additional arguments to be formatted based on the format string, providing flexibility in formatting the output.- The return type change to
size_taligns with the standardsnprintfbehavior, returning the number of characters that would have been written if the buffer size was large enough.These improvements enhance the safety and functionality of the
snprintf_function, making it a more robust and reliable choice for formatted output.
63-63: LGTM! The updatedvsnprintf_signature enhances safety and functionality.The changes to the
vsnprintf_function signature are beneficial for the following reasons:
- The inclusion of the buffer size parameter
bufszhelps prevent buffer overflow vulnerabilities by limiting the number of characters written to the buffer.- The format string parameter
formatenables type-safe formatting of the output, reducing the chances of format string vulnerabilities.- The
va_listparametervlistallows passing variadic arguments to be formatted based on the format string, providing flexibility in formatting the output.- The return type change to
size_taligns with the standardvsnprintfbehavior, returning the number of characters that would have been written if the buffer size was large enough.These improvements enhance the safety and functionality of the
vsnprintf_function, making it a more robust and reliable choice for formatted output when working with variadic arguments.
josesimoes
force-pushed
the
update-printf
branch
from
5436435 to
5e66934
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/CLR/Core/Execution.cpp (1)
1614-1620: Looks reasonable but consider enhancing the comment.The way the exceptional case of memory allocation during GC for watchdog logging is handled looks fine. Logging an error and returning NULL is a graceful way to handle it.
However, consider expanding the comment with more details on:
- Why watchdog logging needs memory allocation
- What are the implications/risks of allowing this exceptional allocation
- Is there any other possible way to avoid needing allocation here
This will help future readers better understand this tricky scenario.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/CLR/Core/Execution.cpp (4 hunks)
- src/CLR/Core/GarbageCollector.cpp (1 hunks)
- src/CLR/Diagnostics/Info_Safeprintf.cpp (1 hunks)
- src/CLR/Diagnostics/Profiler.cpp (10 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf_cfg.h (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/CLR/Helpers/nanoprintf/nanoprintf_cfg.h
Additional comments not posted (20)
src/CLR/Diagnostics/Info_Safeprintf.cpp (2)
14-14: LGTM!Changing the type of
charsfrominttosize_tis a good improvement. It aligns the variable type with the return type ofvsnprintf, enhancing type safety and mitigating potential issues related to signed and unsigned integer comparisons.
17-21: LGTM!The change to set
charstoiBuffer - 1when the buffer is too small is a good improvement. It ensures that the function always null-terminates the output buffer, even when the buffer size is exceeded, enhancing the robustness of the function.Regarding the past review comments, they are not directly applicable to this change. The previous comments referred to a different condition (
chars >= 0vs.chars > 0), which is not the focus of the current change. This change does not introduce inconsistencies with other parts of the codebase, as it specifically addresses the issue of null-termination when the buffer size is exceeded.src/CLR/Core/GarbageCollector.cpp (1)
610-639: Excellent refactoring to improve performance and debug output clarity!The changes made to the
CheckMemoryPressurefunction provide the following benefits:
Improved performance by avoiding unnecessary string construction and printing when the memory tracing level is not set to verbose. This ensures that the detailed type name logging only occurs when needed, reducing overhead in normal operation.
Enhanced clarity of the debug output by separating the "DROPPING OBJECT" message from the detailed type information. This makes the output more readable and allows for quick identification of dropped objects, with the option to delve into the details when the verbose tracing level is enabled.
Great job optimizing this function while maintaining its functionality!
src/CLR/Diagnostics/Profiler.cpp (14)
Line range hint
18-31: LGTM!The function correctly initializes the profiler instance and its fields.
Line range hint
33-39: LGTM!The function correctly deletes the profiler instance and cleans up its state.
Line range hint
41-45: LGTM!The function correctly cleans up the profiler state by deleting the
m_streaminstance.
Line range hint
47-55: LGTM!The function correctly sends the memory layout information by writing the command header, heap location, and size to the stream.
Line range hint
57-104: LGTM!The function correctly dumps the heap information by sending the start marker, dumping root objects, iterating through heap clusters, dumping each object, sending the end marker with the total heap size, and flushing the stream data.
Line range hint
106-152: LGTM!The function correctly dumps the root objects by iterating through finalizers, app domains, assemblies, and threads, calling
DumpRootfor each object, and sending the stream data.
Line range hint
154-174: LGTM!The function correctly dumps a single root object by writing the command header, root object pointer, root type, and method index (if applicable) to the stream.
Line range hint
176-324: LGTM!The function correctly dumps a single object by writing the command header, object pointer, size, and data type to the stream. It handles various object types and their specific data, such as type index, object lock, fields, array elements, and additional references. The function also marks the end of the object's references.
Line range hint
326-349: LGTM!The function correctly finds the referenced object from a reference by iterating through the reference chain and handling various reference types. It returns the referenced object or
NULLif no referenced object is found.
Line range hint
351-354: LGTM!The function correctly marks the end of an object's references list by writing a
falsevalue to the stream.
Line range hint
356-359: LGTM!The function correctly writes a pointer value to the stream by calculating the offset from the start of the heap and packing and writing the offset value.
Line range hint
361-369: LGTM!The function correctly dumps a single reference to the stream by finding the referenced object, writing a
truevalue if a referenced object is found, and writing the referenced object pointer to the stream.
Line range hint
371-378: LGTM!The function correctly dumps a list of references to the stream by iterating through the list starting from the given
firstItempointer, callingDumpSingleReferencefor each reference, and continuing the iteration for the specifiedcountof references.
Line range hint
380-386: LGTM!The function correctly dumps a list of references from a double-linked list to the stream by iterating through the list using the
NANOCLR_FOREACH_NODEmacro and callingDumpSingleReferencefor each node.src/CLR/Core/Execution.cpp (3)
10-13: LGTM!The conditional compilation for Windows 64-bit and inclusion of portable integer type headers looks good.
184-185: Good catch!Replacing
%I64Xwith%" PRIx64"is the right way to print 64-bit integers portably. This improves code maintainability.
1724-1732: Good improvement to failure logging!The additional logging of allocation size and free memory on failures will be very helpful in debugging out-of-memory issues.
The two separate code paths to handle scheduling compaction or not also look good.
Also applies to: 1738-1745
josesimoes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/CLR/Core/Core.vcxproj (1)
Incomplete Removal of nanoprintf and Missing Integration of es-printf
The removal of
nanoprintf.cis incomplete as there are still references in the following project files:
CMake/Modules/FindNF_CoreCLR.cmakesrc/CLR/CorLib/CorLib.vcxprojsrc/CLR/Diagnostics/Diagnostics.vcxprojAdditionally, there are no references to
es-printfin the codebase, indicating that the integration ofes-printf v1.3may be missing.Please ensure that:
- All remaining references to
nanoprintfare removed from the project configuration files.es-printf v1.3is properly integrated into the project, including necessary include directories and dependencies.🔗 Analysis chain
Line range hint
1-220: Verify impact of nanoprintf removal and es-printf updateWhile no changes are visible in the provided code, the removal of
nanoprintf.c(if confirmed) and the update to es-printf v1.3 might have broader implications for the project. Please ensure that:
- All references to
nanoprintfhave been removed or replaced throughout the codebase.- Any new dependencies or changes required by es-printf v1.3 have been properly addressed.
- The functionality previously provided by
nanoprintfis now correctly handled by es-printf v1.3.To help verify these points, please run the following script:
This script will help identify any remaining references to
nanoprintf, show wherees-printfis being used, and list recent changes related to printf functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for nanoprintf references and es-printf usage # Search for nanoprintf references in the entire codebase echo "Searching for nanoprintf references:" grep -R "nanoprintf" . # Search for es-printf usage echo "Searching for es-printf usage:" grep -R "es-printf" . # Check for any printf-related changes in recent commits echo "Recent printf-related changes:" git log -n 10 --oneline --grep="printf" # List files changed in this PR echo "Files changed in this PR:" git diff --name-only HEAD~1Length of output: 4045
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- CMake/Modules/FindNF_CoreCLR.cmake (0 hunks)
- src/CLR/CorLib/CorLib.h (0 hunks)
- src/CLR/CorLib/corlib_native_System_Number.cpp (0 hunks)
- src/CLR/Core/Core.vcxproj (1 hunks)
- src/CLR/Core/Core.vcxproj.filters (0 hunks)
- src/CLR/Diagnostics/Diagnostics.h (0 hunks)
- src/CLR/Diagnostics/Info_Safeprintf.cpp (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (0 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.h (0 hunks)
- targets/AzureRTOS/_common/nanoCLR/nanoCRT.cpp (0 hunks)
- targets/ChibiOS/_nanoCLR/nanoCRT.cpp (0 hunks)
- targets/ESP32/_Network/NF_ESP32_OpenThread.cpp (0 hunks)
- targets/ESP32/_nanoCLR/nanoCRT.cpp (0 hunks)
- targets/FreeRTOS/_common/nanoCLR/nanoCRT.cpp (0 hunks)
- targets/TI_SimpleLink/_nanoCLR/nanoCRT.cpp (0 hunks)
💤 Files not reviewed due to no reviewable changes (13)
- CMake/Modules/FindNF_CoreCLR.cmake
- src/CLR/CorLib/CorLib.h
- src/CLR/CorLib/corlib_native_System_Number.cpp
- src/CLR/Core/Core.vcxproj.filters
- src/CLR/Diagnostics/Diagnostics.h
- src/CLR/Helpers/nanoprintf/nanoprintf.c
- src/CLR/Helpers/nanoprintf/nanoprintf.h
- targets/AzureRTOS/_common/nanoCLR/nanoCRT.cpp
- targets/ChibiOS/_nanoCLR/nanoCRT.cpp
- targets/ESP32/_Network/NF_ESP32_OpenThread.cpp
- targets/ESP32/_nanoCLR/nanoCRT.cpp
- targets/FreeRTOS/_common/nanoCLR/nanoCRT.cpp
- targets/TI_SimpleLink/_nanoCLR/nanoCRT.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLR/Diagnostics/Info_Safeprintf.cpp
🔇 Additional comments not posted (1)
src/CLR/Core/Core.vcxproj (1)
Line range hint
1-220: Summary and additional information requestThe review of
src/CLR/Core/Core.vcxprojhas highlighted a discrepancy between the AI-generated summary and the visible code changes. The main points of concern are:
- The reported removal of
nanoprintf.cfrom the project.- The potential impact of updating es-printf to v1.3.
To complete this review effectively, we need:
- Confirmation of the actual changes made to this file.
- Information on how the es-printf update affects the project structure and dependencies.
- Results from the verification scripts provided in the previous comments.
Once this information is available, we can provide a more accurate and comprehensive review of the changes.
Automated fixes for code style.
Automated fixes for code style.
- Update CMake, declarations and projects accordingly.
Automated fixes for code style.
- Update CMake, declarations and projects accordingly.
josesimoes
force-pushed
the
update-printf
branch
from
2e2a56e to
54706eb
Compare
This reverts commit b00c7fc.
Description
Motivation and Context
- Following attempt to migrate es-printf to v1.3 stumbled on unit tests failing because of MAX values for double. Tried get support from library author without success skirridsystems/es-printf#1 and skirridsystems/es-printf#2How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Summary by CodeRabbit