Skip to content

Conversation

@ChAoSUnItY
Copy link
Collaborator

@ChAoSUnItY ChAoSUnItY commented Mar 27, 2025

This patch adds snprintf and refines printf family funcs by changing function signature to return an integer to match their original definitions defined by specifications.

The second parameter "size" is expected to accept only zero or positive integer, otherwise as C11 specification states, it would be undefined behavior.

Behavior of current snprintf implementation

Quoting C11, chpater §7.21.6.5, The snprintf() function:

int snprintf(char * restrict s, size_t n,const char * restrict format, ...);

[...] If n is zero, nothing is written, and s may be a null pointer.

If n is negative, it's undefined behavior, and it would likely cause program to crash due to null terminator's assignment.

About reusability

Currently shecc does not support va_list-related functionality, we may want to replace __format_to_buf with variadic-function-variant of printf family functions to furthur enhance support and reusability.

Note on previous Pull Request

The previous Pull Request is outdated for several commits, and it's opened on master branch, so I decides to reopen here and resolve conflicts locally.

Summary by Bito

This pull request introduces a new snprintf function that complies with C11 standards, enhancing size parameter handling to prevent undefined behavior. It also refines the printf family of functions by changing their return types to integers and updates constants for larger data sizes. Comprehensive unit tests have been added to ensure correctness.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

@ChAoSUnItY ChAoSUnItY force-pushed the snprintf branch 6 times, most recently from 3dadfd4 to 8d100be Compare March 27, 2025 19:19
@ChAoSUnItY
Copy link
Collaborator Author

ChAoSUnItY commented Mar 27, 2025

I've modified $(BUILD_SESSION) task in MakeFile by replacing $(PRINTF) with $(VECHO), due to a weird stage1 artifact building failure, which is caused by segmentation fault. I am not sure the cause of this since it does not exit abnormally when manually compiling artifact, and it also would generate stage1 artifact even exit abnormally while executing make target.

@jserv
Copy link
Collaborator

jserv commented Mar 28, 2025

Generate a compilation of potential code snippets that could benefit from implementing snprintf().

@ChAoSUnItY
Copy link
Collaborator Author

ChAoSUnItY commented Mar 28, 2025

The weird is back as soon as I refactored file inclusion logic with snprintf. It's still not reproducible under gdb environment.

@DrXiao
Copy link
Collaborator

DrXiao commented Mar 28, 2025

@ChAoSUnItY
FYI: I have fetched the commit from icgmilk:snprintf, found the potential cause, and fixed it. (57f61d1)
(All GitHub actions passed after applying my patch.)

After using valgrind, it indicated that the stack size for liveness analysis might be insufficient. I enlarged the default size, and the issue was resolved.

$ valgrind out/shecc -o shecc src/main.c
...
 ==24861== Process terminating with default action of signal 11 (SIGSEGV)
 ==24861==  Access not within mapped region at address 0xFFFFFFFE6DE41EB0
+==24861==    at 0x1144D0: bb_add_killed_var (ssa.c:473)
 ==24861==    by 0x114E67: bb_solve_globals (ssa.c:537)
 ==24861==    by 0x113BCC: bb_forward_traversal (ssa.c:40)
 ==24861==    by 0x114F04: solve_globals (ssa.c:552)
 ==24861==    by 0x116552: ssa_build (ssa.c:1205)
 ==24861==    by 0x11C8D1: main (main.c:91)
 ==24861==  If you believe this happened as a result of a stack
 ==24861==  overflow in your program's main thread (unlikely but
 ==24861==  possible), you can try to increase the size of the
 ==24861==  main thread stack using the --main-stacksize= flag.
 ==24861==  The main thread stack size used in this run was 8388608.

shecc/src/ssa.c

Line 473 in 6196ab1

bb->live_kill[bb->live_kill_idx++] = var;

@ChAoSUnItY
Copy link
Collaborator Author

Thanks for the solution, but for near future development's sake, and as introduction of #184, the memory issue won't be heavily affected by this PR. Thus, I'll leave current changes to hard limits of various data structure as is, since these bugs are either sometimes hard to reproduce or being not so straightforward.

Of course, it would be good to replace all fixed-size data structures with extensible ones, I'll later assign my team member and I address this kind of issue.

@ChAoSUnItY ChAoSUnItY force-pushed the snprintf branch 2 times, most recently from a7b6b3a to 98ed520 Compare April 5, 2025 05:05
@ChAoSUnItY ChAoSUnItY force-pushed the snprintf branch 4 times, most recently from 9eacb61 to 61aec72 Compare April 5, 2025 12:17
Copy link
Collaborator

@DrXiao DrXiao left a comment

Choose a reason for hiding this comment

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

Because the proposed changes not only implement snprintf() but also refine the implementations of functions such as printf() and sprintf(), I think the git commit messages could be improved to describe the details of these changes.

@ChAoSUnItY ChAoSUnItY changed the title Add snprintf implementation Add snprintf implementation and refine printf family funcs Apr 5, 2025
@ChAoSUnItY ChAoSUnItY force-pushed the snprintf branch 2 times, most recently from f1548d3 to c5e14b0 Compare April 6, 2025 15:11
@ChAoSUnItY ChAoSUnItY force-pushed the snprintf branch 4 times, most recently from e3350d2 to f0eceb6 Compare April 8, 2025 13:05
This patch adds snprintf and refines printf family funcs by changing
function signature to return an integer to match their original
definitions defined by specifications.

The second parameter "size" for snprintf is expected to
accept only zero or positive integer, otherwise as C11 specification
states, it would be undefined behavior.

Co-authored-by: Jim Hsu <[email protected]>
@jserv jserv merged commit 58ae764 into sysprog21:master Apr 8, 2025
6 checks passed
@jserv
Copy link
Collaborator

jserv commented Apr 8, 2025

Thank @ChAoSUnItY for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants