[ci-fix] Needs review: Normalize -1 bufferSize in StreamReader/StreamWriter String ctors (refs #128062)#129360
Closed
github-actions[bot] wants to merge 2 commits into
Closed
Conversation
…r/StreamWriter Under PGO, the JIT may optimize away the bufferSize == -1 conversion in the Stream-based constructor when it is inlined into the String-based constructor. This causes ThrowIfNegativeOrZero to fire for the -1 sentinel value. Defensively normalize -1 to DefaultBufferSize in the String-based constructors before passing to the Stream-based constructor, making the code robust against PGO optimization decisions. Fixes the NegativeOneBufferSize_ShouldNotThrowException test failure in the runtime-coreclr libraries-pgo pipeline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-io |
Member
|
I don't see a point in trying to workaround a codegen bug like this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workflow artifact: ci-fix
Artifact kind: help
Linked KBE: #128062
Note
This is an AI/Copilot-generated best-effort fix attempt that I could not fully validate. It is a starting point for a maintainer, not a finished change. Please review the analysis below before merging.
Root cause (best analysis)
StreamReader_StringCtorTests.NegativeOneBufferSize_ShouldNotThrowExceptionand the equivalentStreamWritertest fail under PGO (runtime-coreclr libraries-pgopipeline) because the JIT, guided by PGO data, optimizes away theif (bufferSize == -1) bufferSize = DefaultBufferSize;branch in the Stream-based constructor when it is inlined into the String-based constructor.The failing log line:
The String-based constructors (
StreamReader(string, Encoding, bool, int)andStreamWriter(string, bool, Encoding, int)) pass theirbufferSizeparameter (which may be -1) directly to the Stream-based constructor via constructor chaining. The Stream-based constructor handles -1 by converting toDefaultBufferSizebefore callingThrowIfNegativeOrZero. Under PGO inlining, this conversion branch is (incorrectly) optimized away.Attempted fix
Normalize the -1 sentinel to
DefaultBufferSizein the String-based constructors before passing to the Stream-based constructor, so the inlined code never sees -1 at theThrowIfNegativeOrZerocall site regardless of PGO optimization decisions. This is a defense-in-depth fix — the Stream-based constructor still handles -1 correctly for direct callers.Changes:
StreamReader.csline 200:bufferSize→bufferSize == -1 ? DefaultBufferSize : bufferSizeStreamWriter.csline 148: same patternWhat is unverified / where I need help
@EgorBo: The issue is labeledarea-CodeGen-coreclrand assigned to EgorBo, suggesting the team views this as a codegen issue. This defensive fix is a product-code hardening that makes the code PGO-safe without touching the JIT.Validation
dotnet build src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.csprojNegativeOneBufferSize_ShouldNotThrowExceptioncallsnew StreamReader(path, Encoding.UTF8, true, -1)which exercises exactly this constructor path. With the fix, -1 is converted to DefaultBufferSize before the inlined Stream ctor sees it, soThrowIfNegativeOrZeronever fires.Evidence
Help wanted
area-CodeGen-coreclr):@EgorBo,@dotnet/jit-contribFiled by
ci-failure-fix. Comment here or on the workflow file to suggest changes;ci-failure-scan-feedbackreads in-scope feedback daily and opens (or updates) a PR with prompt edits.