Repository navigation
fix(libc): keep host logging on the console when a guest reopens stdout or stderr - #13390
medazizktata wants to merge 2 commits into
Conversation
|
@medazizktata Reviewed at e5671a8. Please handle preservation failure at the new Linux stdout keep before reopening. FileStream.hpp:242 calls KeepHostStandardOutput_nid_no_patch; General.cpp:623-632 throws std::system_error on fcntl/fdopen failures such as EMFILE or ENOMEM. freopen_nid_postfix catches bad_alloc and filesystem_error, so this exception escapes the guest C ABI. The PR's failure limitation describes an ignored -1, but these failures throw instead. Please use an explicit failure policy that does not let a new host preservation failure escape as a C++ exception, and add a bounded failure-injection regression for the first stdout keep (guest outcome and host-stream state). Keep the normal guest-file/host-console separation test. No requirement to exhaust the review host's descriptor table. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the approach and the shared-console test look right. One blocker, already raised by @oneandonlydean and not yet addressed:
FileStream.hpp:197-198and:242callKeepHostStandardOutput_nid_no_patch()/KeepHostStandardError_nid_no_patch()fromReopen. On LinuxKeepHostStandardOutput_nid_no_patch(General.cpp:623-632) throwsstd::system_erroronfcntl/fdopenfailure (EMFILE, ENOMEM), it does not return -1.freopen_nid_postfixonly catchesbad_allocandfilesystem_error, so a failed keep escapes the guest C ABI. The "Not tested: a failing Keep returns -1" note in the body does not match the code.
Please choose an explicit policy (for example catch the keep failure inReopenand have thefreopenfail with the errno, or continue without the keep) so no new exception can leavefreopen, and state it in the PR.- Add a bounded regression for the failing first stdout keep that checks the guest outcome (return value, errno) and the host stream state afterwards. There is no need to exhaust the descriptor table; a hook or a closed descriptor 1 is enough.
Keep the existing guest-file/host-console separation test. Happy to re-review once pushed.
e5671a8 to
4cf81e1
Compare
|
@oneandonlydean @adriadam10 Thanks, you're both right: on Linux the keep throws, and my "returns -1 and is ignored" only held on Windows. The stderr keep from #4792 throws the same way. Pushed 4cf81e1:
Tested on
Deliberately breaking the test so it stops filling the table after one descriptor makes it fail 3/3 on "the descriptor table was not filled" and "the host standard output was replaced". Restored, it passes. Not tested: a failing |
malekaifa
left a comment
There was a problem hiding this comment.
Reviewed exact head 4cf81e1d1d4aebae35b813dd8dc89a118d72beba.
The earlier keep-failure blocker is addressed in the current source: on non-Windows, Reopen catches std::system_error from preserving stdout/stderr, restores the prior errno, and continues with the documented best-effort policy instead of allowing an exception to escape the guest freopen ABI. The Linux-only regression exercises that path. On Windows, using the raw UCRT standard-stream identity for IsHostStandard() is consistent with keeping host logging on the preserved duplicate while allowing repeated guest reopens; the existing standard-stream suite covers successful redirect, failed reopen, closed/reopened stdout, and update mode.
Independent Windows 11 / WinLibs GCC 15.2.0 Release validation on this exact head:
- built
guest_freopen_host_log_testscleanly; guest_freopen_host_log: 5/5 consecutive passes;- built
guest_standard_stream_testsandguest_freopen_host_standard_testscleanly; - all five
guest_standard_streams*cases: 3 repetitions each, 15/15 passes; guest_freopen_host_standard: 3/3 passes;tools/check_conventions.py --base origin/main: pass;git diff --check origin/main...HEAD: pass;git merge-tree --write-tree HEAD origin/mainagainst currentmain16345cad6a: clean.
Hosted checks on the exact head currently show Windows, conventions, library portability, report, and relinker jobs green; macOS write-watch jobs are still pending. I did not independently execute the Linux keep-failure test, so that part of this review is source-level plus the submitted regression rather than a local Linux run.
No remaining blocker found in this change.
AI-assisted: yes (OpenAI ChatGPT).
What
After a guest
freopenofstdoutorstderr, AnyPS5's own log lines followed the guest stream into the title's file (#13328). On Windows both streams were affected, on Linux onlystdout. A guestdup2onto descriptor 1 or 2 already keeps the host streams first (#7807), and so doessetvbuf(#10020);freopendid not, except for Linuxstderr(#4792).Changes, all in
core/libs/prx/libc/include/FileStream.hpp:FileStream::Reopenkeeps the host stream before the guest's file replaces descriptor 1 or 2:KeepHostStandardOutput_nid_no_patch()orKeepHostStandardError_nid_no_patch()when the stream is the C runtime's stdout or stderr;KeepHostStandardOutput_nid_no_patch()for descriptor 1, next to the existingKeepHostStandardError_nid_no_patch()for descriptor 2.IsHostStandard()compares against__acrt_iob_func(0/1/2)instead ofstdin/stdout/stderr. Inside libc those macros resolve, through the forcedHostStandardStreams.hpp, to the kept host stream once a keep has run. After the first keep, a guest standard stream therefore no longer counted as standard, and a secondfreopenof it took thefclosepath and closed the C runtime's stream.guest_standard_streamscatches that.KeepHostStandardError_nid_no_patchwas declared outside Windows only.std::system_error(no free descriptor, no memory),Reopenrestoreserrnoand the guest'sfreopengoes ahead; only AnyPS5's own output then follows the guest's file. This covers the existing stderr keep too. On Windows a failed keep already returns -1 and is ignored.KeepHostStandardOutput_nid_no_patchthrows (General.cpp). A WebAssembly build keeps today's behaviour, and the WebAssembly entry indocs/dev/debt/libc.mdnow says that host stdout follows a guestfreopenthere.docs/dev/GAME_TESTING.md: the line added by #13024 saidrun.logends at the entry point for these titles. It now says that the title's own output, including its exit reason, is not inrun.log; the pointer toapp.logstays.Test: new ctest
guest_freopen_host_log(core/libs/tests/GuestFreopenHostLog.cpp, registered incore/libs/prx/libc/CMakeLists.txt, Windows and Linux).freopenitsstdoutandstderrto two files and write a line to each, then writes one host line to each stream.Second test, Linux only:
guest_freopen_host_log_keep_failure, a mode of the same executable. In a fresh process it fills descriptors 3 and up under a loweredRLIMIT_NOFILEand closes descriptor 0, so the first stdout keep fails withEMFILEwhile the guest's file can still be opened. It checks that no exception leavesfreopen, thatfreopensucceeds with the guest's line in its file, and that the host'sstdoutwas not replaced.Tested
All on
maina5939f10aunless stated. The branch is now onf91137142e. Between the two, this code only gained the Emscripten guard, so on Linux I builtlibc.prxwith and without it there: both builds are byte-identical, libc was recompiled for the second, and the test-only build differs from both. Onf91137142ethe test still fails 3/3 on the stdout checks without the fix, and the 9 related tests pass 5/5 with it.After review, with the keep-failure
catch, onf91137142e:guest_freopen_host_log_keep_failurefails 3/3 without thecatchand passes 5/5 with it; Linuxctest -E agc1872/1872, Windows 1954/1954.guest_freopen_host_logguest_freopen_host_log,guest_freopen_host_standard, the fiveguest_standard_streams*,guest_host_standard_error_windows,guest_host_standard_buffering_windows,guest_stdio_variadic)ctest -E agcguest_freopen_host_logctest -E agcDeliberate breaks, Windows:
guest_freopen_host_logfails 3/3 on all four host checks;IsHostStandard():guest_standard_streamsaborts 3/3 (0xc0000409).Deliberate break, Linux: removing the new stdout keep makes
guest_freopen_host_logfail 3/3 on the stdout checks.Restoring the file byte-for-byte passes again on both. Build logs show libc relinked in every step.
Real titles, Windows, RTX 4060: Trophy Manager (PPSA99842) and PSCord (PPSA99190), each converted once and run in two copies that differ only in the libraries (test only / this change), with
APS5_PROFILE_DRAW=1. All runs were still running when stopped.10 minutes,
main66b8e907e:[present]on consoleapp.log[present], 45[src/…], 2[pipeline-cache], plus[recipe],[draw-cache]…[present], 202[src/…], 2[pipeline-cache]…With the change, the console and
app.logshare no line tag;app.logholds only the title's own lines ([TROPHY],[HUI],[ps5-gallium],[dc]…).120 s with
ANYPS5_LOG_FILE(feat(libc): copy the runtime log into a file with ANYPS5_LOG_FILE #12715),maina5939f10a:app.log[present], 45[src/…]…[present], 142[src/…]…The extra line in each copy is its
[log] ANYPS5_LOG_FILE: appending…header; every tag on the console is in the copy. This is also a first run ofANYPS5_LOG_FILEon Windows, whichdocs/user/USAGE.mdlists as compiled but not run.tools/check_conventions.py --base origin/main: passes.Not tested:
ANYPS5_LOG_FILEon Linux;fdopenin the Linux keep (ENOMEM), and a failing stderr keep; both take the samecatchas the tested stdout case;freopenof stdout or stderr after an earlier keep now detaches the C runtime's stream instead of closing it; the existing closed-stream tests pass, but there is no test for this case.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)FileStream.hpp, so whichever lands second has a small text conflict)freopenof stdout or stderr, AnyPS5's own log goes into the title's file (Windows: both streams; Linux: stdout) #13328