Fix TPA map hash calculation.

[viewizard]

The point of issue is "the Turkish-I Problem". After locale changed, towupper() provide another result for "i" and different hash are calculated in case if file name have "i" letter.

Fix #37910

CC @alpencolt @jkotas @HJLeee

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

[viewizard]

I see some tests failed with

Assert failure(PID 2340 [0x00000924], Thread: 3012 [0x0bc4]): CONTRACT VIOLATION by SString::SString at "F:\workspace\_work\1\s\src\coreclr\src\inc\sstring.inl" @ 231

THROWS called in a NOTHROW region.

                        CONTRACT in SString::SString at "F:\workspace\_work\1\s\src\coreclr\src\inc\sstring.inl" @ 231
VIOLATED-->  CONTRACT in SHash<class BINDER_SPACE::SimpleNameToFileNameMapTraits>::AddOrReplace at "F:\workspace\_work\1\s\src\coreclr\src\inc\shash.inl" @ 587
                        CONTRACT in SHash<class BINDER_SPACE::SimpleNameToFileNameMapTraits>::AddOrReplace at "F:\workspace\_work\1\s\src\coreclr\src\inc\shash.inl" @ 143
                        CONTRACT in CorHost2::CreateAppDomainWithManager at "F:\workspace\_work\1\s\src\coreclr\src\vm\corhost.cpp" @ 573

this PR should be fixed first.

[ghost]

Tagging subscribers to this area: @vitek-karas
Notify danmosemsft if you want to be subscribed.

[vitek-karas]

Could you please add a test for this - maybe just the Turkish i. Probably into this suite: https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime.Loader/tests

[viewizard]

I believe, I could create test, but I need more time, since I am not really familiar with this test suite.

[viewizard]

@vitek-karas I found, that the only way I could guaranteed test internal TPA map for consistent state is

#ifdef _DEBUG
            if (pTpaEntry == nullptr)
            {
                auto CheckAllEntriesBySimpleName = [&](SimpleNameToFileNameMapEntry &element)
                {
                    _ASSERTE(0 != wcscmp(element.m_wszSimpleName, simpleName.GetUnicode()));
                };
                tpaMap->ForEach(CheckAllEntriesBySimpleName);
            }
#endif //_DEBUG

code block after line:

runtime/src/coreclr/src/binder/assemblybinder.cpp

Line 1066 in 97e553f

const SimpleNameToFileNameMapEntry *pTpaEntry = tpaMap->LookupPtr(simpleName.GetUnicode());

With this changes in CoreCLR we could use simple test with setlocale call in https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime.Loader/tests
but the problem is - CI should test Libs with CoreCLR Debug build in order to make this work.
Is the any way I could provide Libs test for CoreCLR Debug build?

[vitek-karas]

Can't we write a repro based on the issue #37910?
Having internal consistency check in CLR is nice, but it's typically not how we test fixes. After all we do the fix to mitigate a problem observed on all builds (Debug or Release), so the test should validate that the fix works there as well.

[viewizard]

Can't we write a repro based on the issue #37910?

This could be tested by direct dotnet application start. Something like simple application:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp5.0</TargetFramework>
  </PropertyGroup>
</Project>

using System;
using System.Runtime.InteropServices;

namespace Test
{
    public class Program
    {
        private const string LibcLibrary = "libc";
        private const string TRLocale = "tr_TR.UTF-8";

        [DllImport(LibcLibrary, EntryPoint = "setlocale")]
        public static extern IntPtr setlocale(int category, [MarshalAs(UnmanagedType.LPStr)] string locale);

        static int Main(string[] args)
        {
            IntPtr res = setlocale(6 /*LC_ALL*/, TRLocale);
            if (TRLocale != Marshal.PtrToStringAnsi(res))
            {
                return 1;
            }
            Console.WriteLine(Marshal.PtrToStringAnsi(res)); // unhandled exception at this call
            
            return 0;
        }
    }
}

viewizard@totoro:~/Desktop/test$ dotnet build -c Release
Microsoft (R) Build Engine version 16.7.0-preview-20309-02+d6862eb21 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  Test -> /home/viewizard/Desktop/test/bin/Release/netcoreapp5.0/Test.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.45
viewizard@totoro:~/Desktop/test$ dotnet /home/viewizard/Desktop/test/bin/Release/netcoreapp5.0/Test.dll
Unhandled exception. System.IO.FileNotFoundException: Could not load file or assembly 'System.Threading, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.

File name: 'System.Threading, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
   at System.Console.<get_Out>g__EnsureInitialized|25_0()
   at System.Console.get_Out()
   at System.Console.WriteLine(String value)
   at Test.Program.Main(String[] args)
Aborted (core dumped)

No exception thrown if patch applied, or if TRLocale is en_US.UTF-8, ru_RU.UTF-8 or any other without Turkish-I.
But since this part of CoreCLR work with another logic during xunit test, AssemblyBinder::BindByTpaList() will be never called at Console.WriteLine() after setlocale with simple name already in TPA map but with different hash, so, no exception will be thrown.

[jkotas]

You are right that the xunit harness under libraries does not make it easy to test.

Writing this as a standalone test under coreclr should work better, e.g. you can start with something like this
jkotas@d8b59c9

[viewizard]

I added test. Please note, Linux must have tr_TR.UTF-8 locale installed/generated in system, or test will fail at setlocale call.

[jkotas]

Thank you!

[jkotas]

Linux must have tr_TR.UTF-8 locale installed/generated in system

Would it be better for the test to succeed when this locale is not installed in the system?

[viewizard]

I am not sure about this, since in this case test could never warn you about regression in code. With current implementation, you at least will see, that some tests are failed due to system environment.

[BruceForstall]

As predicted above, this test is failing on Linux, in all outerloop jobs.

Unfortunately, this test was added as a "Pri-1" test, but the outerloop / Pri-1 tests were not triggered as part of this PR. In the future, authors and code reviewers, please pay attention to that.

[BruceForstall]

Actually, the test only runs on Linux, so maybe it only passes on arm32/arm64 Linux runs, but no other?

[viewizard]

Actually, the test only runs on Linux, so maybe it only passes on arm32/arm64 Linux runs, but no other?

Was tested on amd64 too.
Error message:

...
Failed! tr_TR.UTF-8 locale was not found in system!
...

#38016 (comment)
I mentioned, that tr_TR.UTF-8 locale must be installed/generated in system.