Skip to content

CI Test - #78709

Closed
MihaZupan wants to merge 4 commits into
dotnet:mainfrom
MihaZupan:indexofanyvalues-http-arm64-checked-test
Closed

CI Test#78709
MihaZupan wants to merge 4 commits into
dotnet:mainfrom
MihaZupan:indexofanyvalues-http-arm64-checked-test

Conversation

@MihaZupan

Copy link
Copy Markdown
Member

@MihaZupanMihaZupan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it area-System.Net.Http labels Nov 22, 2022
@MihaZupanMihaZupan added this to the 8.0.0 milestone Nov 22, 2022
@ghostghost assigned MihaZupanNov 22, 2022
@MihaZupan

Copy link
Copy Markdown
MemberAuthor

/azp list

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

#78660

Author:MihaZupan
Assignees:-
Labels:

NO-MERGE, NO-REVIEW, area-System.Net.Http

Milestone:8.0.0

@azure-pipelines

This comment was marked as resolved.

@EgorBo

EgorBo commented Nov 22, 2022

Copy link
Copy Markdown
Member

@MihaZupan any guesses from the log of your test that has just failed?
I'm currently not able to test this on an actual linux-arm64, might be in a few hours.

@MihaZupan

Copy link
Copy Markdown
MemberAuthor

The "Correct" behavior is on the left, "arm64 checked" on the right: https://www.diffchecker.com/Wuwbdju9

utf8Length=14 utf8='70, 105, 108, 101, 195, 131, 78, 97, 109, 101, 46, 98, 97, 116' -IndexOfAnyExcept length=4 // Expected+IndexOfAnyExcept length=5 // Arm64 checked

The IndexOfAnyExcept in this case is

privatestaticreadonlyIndexOfAnyValues<byte>s_rfc5987AttrBytes=IndexOfAnyValues.Create("!#$&+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~"u8);utf8.IndexOfAnyExcept(s_rfc5987AttrBytes);

I've added a log to see if the type of s_rfc5987AttrBytes is the expected IndexOfAnyAsciiCharValues.

@MihaZupan

MihaZupan commented Nov 22, 2022

Copy link
Copy Markdown
MemberAuthor

Narrowing it down to

[Fact]publicvoidIndexOfAnyExceptTest(){ReadOnlySpan<byte>bytes=newbyte[]{70,105,108,101,195,131,78,97,109,101,46,98,97,116};intoffset=bytes.IndexOfAnyExcept(s_rfc5987AttrBytes);_output.WriteLine($"s_rfc5987AttrBytes is {s_rfc5987AttrBytes.GetType().FullName}");Assert.Equal(4,offset);}

@EgorBo

Copy link
Copy Markdown
Member

@MihaZupan from what I see so far - it reproduces on Release too (osx-arm64) and with JitMinOpts=1 meaning that it's very unlikely a jit optimization

@MihaZupan

MihaZupan commented Nov 22, 2022

Copy link
Copy Markdown
MemberAuthor

🤦‍♂️ Found it... it's a bug in IndexOfAnyAsciiSearcher on arm64 for ASCII byte inputs:


should be

if(AdvSimd.Arm64.IsSupported||TOptimizations.NeedleContainsZero)

like it is above for chars

if(AdvSimd.Arm64.IsSupported||TOptimizations.NeedleContainsZero)

I'm very surprised the tests missed this in #78093, looks like we need more ...

@EgorBo

EgorBo commented Nov 22, 2022

Copy link
Copy Markdown
Member

@MihaZupan any idea why tests in #78093 didn't fail? From my understanding, it should reliably fail on any arm64 device (since neon is always there)

@MihaZupan

MihaZupan commented Nov 22, 2022

Copy link
Copy Markdown
MemberAuthor

As long as it supports AdvSimd.Arm64, we should see it yeah.

I expected that at least the outerloop tests I added in that PR should exercise this case, but maybe I was wrong.
You essentially need the following:

  1. You must be using the byte overloads
  2. The needle passed to IndexOfAnyValues.Create must be all ASCII
  3. The input you are searching must contain a non-ascii character
  4. That non-ascii character must be present before any other matching character
  5. Run on a machine where AdvSimd.Arm64.IsSupported

Perhaps the test doesn't satisfy 4.? I'll have to double-check.

@EgorBo

Copy link
Copy Markdown
Member

Run on a machine where AdvSimd.Arm64.IsSupported

It should be a baseline for any arm64 hardware, unless explicitly disabled

@MihaZupan

MihaZupan commented Nov 22, 2022

Copy link
Copy Markdown
MemberAuthor

Actually, there is another condition:
6. The non-ASCII character that was a false-positive (or false-negative in case of Except) must be exactly 128 higher than a value present in the needle.

In this case we weren't matching 195, which happens to be 128 + 67, and 67 is the ASCII C character present in the needle.

Since we're testing random inputs, I bet this is a case we just didn't hit :/

We just got lucky that that one HTTP test happened to be using exactly such values.

@MihaZupan

Copy link
Copy Markdown
MemberAuthor

I'll get a PR up, shouldn't need this one anymore.

Thanks for looking into this as well Egor!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.HttpNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@MihaZupan@EgorBo