Fix CORECLR_NOTIFICATION_PROFILERS dropping entries without trailing ';'#130584
Open
steveisok wants to merge 3 commits into
Open
Fix CORECLR_NOTIFICATION_PROFILERS dropping entries without trailing ';'#130584steveisok wants to merge 3 commits into
steveisok wants to merge 3 commits into
Conversation
AttemptLoadProfilerList used SString::Find as the loop condition, so any profiler list section not followed by ';' - the last or only entry - was never processed. This silently dropped single-entry lists and the final entry of multi-entry lists, a regression from the .NET 8 wcstok_s-based parser. Restructure the loop so each section runs up to the next ';' or to the end of the list, and skip empty sections produced by consecutive or trailing separators. This also stops the previous code from reporting a spurious bad-path error for empty sections. Add a regression test: ProfilerTestRunner gains an optional appendNotificationSeparator flag (default preserves existing behavior), and the multiple profiler test now also runs with no trailing separator for both the CORECLR and DOTNET env var prefixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes CoreCLR’s CORECLR_NOTIFICATION_PROFILERS list parsing so the final (or only) entry is processed even when the list does not end with a trailing ;, and adds regression coverage to prevent future regressions in both CORECLR_ and DOTNET_-prefixed configurations.
Changes:
- Fix
ProfilingAPIUtility::AttemptLoadProfilerListto iterate sections up to the next;or end-of-string, and to skip empty sections (consecutive/trailing separators). - Extend
ProfilerTestRunner.Runwith an option to omit the trailing notification-profiler separator when constructing_NOTIFICATION_PROFILERS. - Update the multiple-profiler test to run the scenario both with and without a trailing separator for both env-var prefixes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/vm/profilinghelper.cpp | Corrects tokenization so the final list entry is not dropped when no trailing ; is present; ignores empty sections. |
| src/tests/profiler/common/ProfilerTestRunner.cs | Adds appendNotificationSeparator to generate notification-profiler lists without a trailing delimiter for regression coverage. |
| src/tests/profiler/multiple/multiple.cs | Runs the multiple-notification-profiler scenario with and without trailing separators for both CORECLR_ and DOTNET_ prefixes. |
Comment on lines
+87
to
+90
| if (result != 100) | ||
| { | ||
| return result; | ||
| } |
jkotas
reviewed
Jul 13, 2026
jkotas
reviewed
Jul 13, 2026
- Trim the AttemptLoadProfilerList comment to describe only the current code rather than the prior loop-condition behavior (per review). - Remove unreachable 'if (result != 100) return result;' checks in the multiple profiler test; ProfilerTestRunner.Run only returns 100 or throws, so chain the Run calls and return the last result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
hoyosjs
approved these changes
Jul 13, 2026
jkotas
reviewed
Jul 14, 2026
Comment on lines
+71
to
+80
| // A notification profiler list whose final entry has no trailing ';' must still | ||
| // load every entry. | ||
| Console.WriteLine($"Running the test with no trailing separator (CORECLR prefix)."); | ||
| ProfilerTestRunner.Run(profileePath: System.Reflection.Assembly.GetExecutingAssembly().Location, | ||
| testName: "MultiplyLoaded", | ||
| profilerClsid: MultipleProfilerGuid, | ||
| loadAsNotification: true, | ||
| notificationCopies: 2, | ||
| appendNotificationSeparator: false, | ||
| envVarProfilerPrefix: "CORECLR"); |
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.
AttemptLoadProfilerList used SString::Find as the loop condition, so any profiler list section not followed by ';' - the last or only entry - was never processed. This silently dropped single-entry lists and the final entry of multi-entry lists, a regression from the .NET 8 wcstok_s-based parser.
Restructure the loop so each section runs up to the next ';' or to the end of the list, and skip empty sections produced by consecutive or trailing separators. This also stops the previous code from reporting a spurious bad-path error for empty sections.
Add a regression test: ProfilerTestRunner gains an optional appendNotificationSeparator flag (default preserves existing behavior), and the multiple profiler test now also runs with no trailing separator for both the CORECLR and DOTNET env var prefixes.
Fixes #126197