IL: optimize attribute cluster reading (Fixed version)

[DedSec256]

Fixed version of #13821

This PR contains fix 1e0ebe5 for the loop counter that previously caused seek to hang in an unsorted table #14651.
Search in a sorted table is also slightly refactored to be more clear.

Without this fix, the hang reproduced at build time and in the IDE while analyzing file SplitApp.Android/MainActivity.fs from the issue above, when trying to read attributes for assembly System.Runtime.CompilerServices.Unsafe.dll (Metadata v4.0.30319).

cc @auduchinok

[auduchinok]

@DedSec256 Thanks!

[auduchinok]

Would it be possible to write a test that references System.Runtime.CompilerServices.Unsafe.DLL and can reproduce the issue without your fixes?

[vzarytovskii]

This will probably have to go past net8, just to be safe

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

@DedSec256 hey Alex, this LGTM, if possible please add a test described by Eugene, then we can probably pull this in.

[DedSec256]

@psfinaki, I tried to add a test, but it requires the specific version of System.Runtime.CompilerServices.Unsafe.dll (Metadata v4.0.30319) on which the problem is known to reproduce. So we need to push this version of the library, or come up with something else.

[vzarytovskii]

@psfinaki, I tried to add a test, but it requires the specific version of System.Runtime.CompilerServices.Unsafe.dll (Metadata v4.0.30319) on which the problem is known to reproduce. So we need to push this version of the library, or come up with something else.

Just have a separate manual test under tests/projects/ which demonstrates the issue + uses a just built compiler. We don't run them for now, though we probably can add them as we do with trimming tests.

[DedSec256]

Sorry, I tried to reproduce it yesterday and today, but for some reason I couldn't. The fix from 1e0ebe5

fsharp/src/Compiler/AbstractIL/ilread.fs

Line 1291 in 1e0ebe5

rid <- rid +1

seems obvious, maybe is it possible to merge this as is?

[vzarytovskii]

Just to be clear, the change compared to previous revision is an off-by-one fix?

[DedSec256]

Yes, the current version differs from the previous one by only one commit 1e0ebe5 that has two changes:

1. Fix for the loop counter that previously caused seek to hang in an unsorted table.
2. Search in a sorted table is also slightly refactored to be more clear.