-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Relax some asserts in SpanHelpers.T.cs #1472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relax some asserts in SpanHelpers.T.cs #1472
Conversation
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs
Outdated
Show resolved
Hide resolved
It makes me worried that you are seeing the asserts, but we are not catching them in the CI. @dotnet/runtime-infrastructure Are there any tests runs that run debug/checked flavors of everything? |
|
Both the pre-checkin and the outerloop pipeline only run release bits. AFAIK the only runs that use checked product bits are GC stress and JIT stress runs. |
|
The dotnet/coreclr repo had coverage for running corefx tests on checked coreclr in the CI. This coverage was proven to be important and caught many bugs early. Is there a workitem to add this back? What it would take to add this back? |
|
I know we discussed that with @BruceForstall when I was working on the PR but looking now I cannot find an explicit item - Bruce, do you know whether there is one or whether I should create a new one? |
With the single pipeline effort that I'm doing this will be simplified. I might just include running corefx tests on checked coreclr (regular runs, not stress) when coreclr is touched. |
|
I have a work item to add back libraries testing on checked coreclr for coreclr checkins, but I've been waiting for the merged repo and live/live CI changes to stabilize before doing that... and maybe @safern's work will make it easy or do it for me. |
Yeah, let's sync up after I'm done, I have some ideas. |
|
@dotnet/runtime-infrastructure A lot of legs are failing with "RestApiException: The response contained an invalid status code 500 Internal Server Error". I am not able to figure out what the problem is - can't find any logs with details. Could you please take a look? |
That's happening across a bunch of PRs, including one I'm working on. Filed https://github.com/dotnet/core-eng/issues/8591 to track. |
|
So in my PR I'm adding some corefx test runs with a checked runtime and it did hit this issue: I will pull up this change and see if it fixes it. |
|
@halter73 @jkotas after pulling the changes into a PR only Here is the dump: |
|
Let's revert the change that added the asserts for now, so that you can get the changes that are fixing the CI coverage in: #1628 |
|
Should this be closed now? |
|
Opened #1767 to track the follow up. |
I saw the following assert failure in System.Memory.Tests
Because of the aggressive inlining, MemoryExtensions.LastIndexOf (a public extension method on Span) doesn't show up in the stack trace, but that's what was calling into the generic SpanHelpers.LastIndexOf method instead of a char-specialized version. I fixed MemoryExtensions.LastIndexOf to call a newly-added char-specialized SpanHelpers.LastIndexOf method.
I also noticed that here are no char-specialized SpanHelpers.LastIndexOfAny methods, so I relaxed the asserts in the generic SpanHelpers.LastIndexOfAny methods.
@jkotas This is a follow up to #1200. Is there an easy way to discover if there are any more assert failures before merging?