JIT: Switch config values to UTF8 #109418

jakobbotsch · 2024-10-31T15:24:55Z

Switch JitConfig strings from UTF16 to UTF8
Switch relevant JIT-EE and host methods to UTF8
Remove getJitTimeLogFilename JIT-EE call, which seemingly existed to avoid having a config parser in the JIT in release builds, but that ship has sailed

* Switch JitConfig strings from UTF16 to UTF8 * Switch relevant JIT-EE and host methods to UTF8

dotnet-policy-service · 2024-10-31T15:26:02Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-10-31T15:50:17Z

src/coreclr/vm/jithost.cpp

 {
    WRAPPER_NO_CONTRACT;

+    SString str(SString::Utf8, name);


Suggested change

SString str(SString::Utf8, name);

StackSString str;

SString(SString::Utf8Literal, name).ConvertToUnicode(str);

This is more efficient way to do the conversions (avoids heap allocations and unnecessary copies).

jakobbotsch · 2024-11-01T08:35:14Z

Hmm, this seems to break the ability to specify file paths with unicode characters on Windows. Probably can't switch to fopen.

jakobbotsch · 2024-11-01T10:15:05Z

Hmm, this seems to break the ability to specify file paths with unicode characters on Windows. Probably can't switch to fopen.

Actually it works fine with UTF8 enabled, this was operator error.

jakobbotsch · 2024-11-01T10:15:27Z

PTAL @dotnet/jit-contrib @AaronRobinsonMSFT

jkotas · 2024-11-01T15:02:52Z

Actually it works fine with UTF8 enabled, this was operator error.

Do you mean that you have to have your Windows configured to CP_ACP == CP_UTF8?.

jakobbotsch · 2024-11-01T16:08:39Z

Actually it works fine with UTF8 enabled, this was operator error.

Do you mean that you have to have your Windows configured to CP_ACP == CP_UTF8?.

I think so, but I haven't confirmed. I can try to confirm whether this is the case.
Do you think we should continue using _wfopen for Windows to avoid this regression? I thought it might be ok since most of the JIT env vars involving file paths are pretty obscure / power user scenarios.

jkotas · 2024-11-01T20:47:27Z

I thought it might be ok since most of the JIT env vars involving file paths are pretty obscure / power user scenarios.

I think it is ok for debug-only configs. If it is used for documented env vars in release builds, it is a compliance issue. All features have to handle globalization properly, even the obscure ones.

src/coreclr/jit/codegenlinear.cpp

src/coreclr/jit/importervectorization.cpp

src/coreclr/jit/jitconfig.cpp

jakobbotsch · 2024-11-04T11:01:03Z

I think it is ok for debug-only configs. If it is used for documented env vars in release builds, it is a compliance issue. All features have to handle globalization properly, even the obscure ones.

I asked @EgorBo to test this since he has UTF8 disabled, but it appears that this fails much earlier on the VM side when you try to use non-ANSI paths:

Assert failure(PID 864836 [0x000d3244], Thread: 864760 [0xd31f8]): SString::_stricmp(valueNoCache, temp.GetUTF8()) == 0

CORECLR! `anonymous namespace'::EnvGetString + 0x3B8 (0x00007ff8`747f40d8)
CORECLR! `anonymous namespace'::GetConfigString + 0xDB (0x00007ff8`747f46eb)
CORECLR! CLRConfig::GetConfigValue + 0xD1 (0x00007ff8`747f4951)
CORECLR! CLRConfig::GetConfigValue + 0xCF (0x00007ff8`747f4eef)
CORECLR! JitHost::getStringConfigValue + 0x8F (0x00007ff8`741ed90f)
CLRJIT! JitConfigValues::initialize + 0x145C (0x00007ff9`206e70fc)
CLRJIT! jitStartup + 0x7E (0x00007ff9`205d9d6e)
CORECLR! LoadAndInitializeJIT + 0xEE7 (0x00007ff8`7496b267)
CORECLR! EEJitManager::LoadJIT + 0x1C0 (0x00007ff8`7496b650)
CORECLR! UnsafeJitFunction + 0x20D (0x00007ff8`741fe3ed)
    File: C:\prj\runtime-main\src\coreclr\utilcode\clrconfig.cpp:211
    Image: C:\prj\runtime-main\artifacts\bin\coreclr\windows.x64.Checked\CoreRun.exe
ERROR:
Assert failure(PID 864836 [0x000d3244], Thread: 864760 [0xd31f8]): SString::_stricmp(valueNoCache, temp.GetUTF8()) == 0

CORECLR! `anonymous namespace'::EnvGetString + 0x3B8 (0x00007ff8`747f40d8)
CORECLR! `anonymous namespace'::GetConfigString + 0xDB (0x00007ff8`747f46eb)
CORECLR! CLRConfig::GetConfigValue + 0xD1 (0x00007ff8`747f4951)
CORECLR! CLRConfig::GetConfigValue + 0xCF (0x00007ff8`747f4eef)
CORECLR! JitHost::getStringConfigValue + 0x8F (0x00007ff8`741ed90f)
CLRJIT! JitConfigValues::initialize + 0x145C (0x00007ff9`206e70fc)
CLRJIT! jitStartup + 0x7E (0x00007ff9`205d9d6e)
CORECLR! LoadAndInitializeJIT + 0xEE7 (0x00007ff8`7496b267)
CORECLR! EEJitManager::LoadJIT + 0x1C0 (0x00007ff8`7496b650)
CORECLR! UnsafeJitFunction + 0x20D (0x00007ff8`741fe3ed)
    File: C:\prj\runtime-main\src\coreclr\utilcode\clrconfig.cpp:211
    Image: C:\prj\runtime-main\artifacts\bin\coreclr\windows.x64.Checked\CoreRun.exe

(fails on main too)

Edit: This appears to just be a buggy assert, in release builds main seems to work ok, and indeed this PR regresses it.

jkotas · 2024-11-04T11:27:24Z

Assert failure(PID 864836 [0x000d3244], Thread: 864760 [0xd31f8]): SString::_stricmp(valueNoCache, temp.GetUTF8()) == 0
Edit: This appears to just be a buggy assert, in release builds main seems to work ok, and indeed this PR regresses it

This came from https://github.com/dotnet/runtime/pull/59513/files#diff-ca28f8b9cc75904774104e92e308462d908149b3e95288e37acd5b719bf6e0e9R211 cc @AaronRobinsonMSFT

jkotas · 2024-11-04T11:30:38Z

src/coreclr/jit/utils.cpp

+FILE* fopen_utf8(const char* path, const char* mode)
+{
+#ifdef HOST_WINDOWS
+    Utf16String<256> pathWide(path);


Paths on Windows can be longer than 256 characters

There is a dynamic allocation inside Utf16String for the longer case

MAX_PATH would be more appropriate here.

I don't really agree, this constant is just an optimization to avoid dynamic allocation in the common cases, it is not intended to have any relation to MAX_PATH.

I don't really agree, this constant is just an optimization to avoid dynamic allocation in the common cases, it is not intended to have any relation to MAX_PATH.

The fact that 256 was choosen on Windows means it is precisely for a path and the common case. That is why the MAX_PATH is what is it, 260, because it was the most common path on Windows and indicates as such. If this was 1024 because that was common, then sure, but someone chose 256 and that aligns with MAX_PATH. The runtime is littered with MAX_PATH precisely for the reason being made.

I chose 256 because that is a 512 byte stack allocation, which was the largest size stack allocation I wanted to go with for the buffer.

Okay. Given this is defined to only be on Windows and there is a macro with a predefined size for the "common" path on the platform, using MAX_PATH in a variable called pathWide seems like the most appropriate to me. If you think an explicit 256 is clearer then feel free to go with it.

I prefer the 256 simply because the value I picked here only coincidentally is close to MAX_PATH and wasn't intended to have any semantic relevance to its value (it seems like an outdated define nowadays given that the max is much higher).

jakobbotsch · 2024-11-04T13:33:58Z

With help from @EgorBo we've verified that unicode paths now work again, even without UTF8 code page. @AaronRobinsonMSFT @jkotas can you please take another look?

src/coreclr/jit/utils.cpp

jakobbotsch · 2024-11-13T15:05:48Z

Ping @dotnet/jit-contrib for a review on the JIT changes

EgorBo · 2024-11-13T15:37:34Z

src/coreclr/jit/importervectorization.cpp

+            }
+            printf("Trying to unroll MemoryExtensions.Equals|SequenceEqual|StartsWith(op1, \"%s\")...\n", utf8Str);
+        }
+#endif


It looks like too much effort to just dump a string in JitDump, I wouldn't bother. We should either remove it or move into some utility, I think it decreases code readability in this file

Good point - I added a Compiler::convertUtf16ToUtf8ForPrinting and switched this to use it.

EgorBo

LGTM

amanasifkhalid

JIT changes LGTM too, thanks!

src/coreclr/jit/utils.cpp

amanasifkhalid · 2024-11-13T15:40:41Z

src/coreclr/jit/utils.cpp

@@ -1883,29 +1881,15 @@ AssemblyNamesList2::AssemblyNamesList2(const WCHAR* list, HostAllocator alloc)

                AssemblyName* newName = new (m_alloc) AssemblyName();

-                // Null out the current character so we can do zero-terminated string work; we'll restore it later.
-                *listWalk = W('\0');
+                size_t nameLen          = listWalk - nameStart;


nit: This logic looks safe to me, though I'm a bit surprised that none of the native compilers are complaining about implicit cast from ptrdiff_t to size_t, since the former is signed.

Switched it to ptrdiff_t

JIT: Switch config values to UTF8

4bfdbf0

* Switch JitConfig strings from UTF16 to UTF8 * Switch relevant JIT-EE and host methods to UTF8

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 31, 2024

dotnet-policy-service bot assigned jakobbotsch Oct 31, 2024

Add null check

22ae214

jkotas reviewed Oct 31, 2024

View reviewed changes

jakobbotsch added 2 commits October 31, 2024 17:42

Update some %ws, fix build on Linux

36e8329

Feedback

6594461

build-analysis bot mentioned this pull request Oct 31, 2024

Test failure BasicEventSourceTests.TestsUserErrors.Test_BadEventSource_MismatchedIds_WithEtwListener #96968

Open

Remove unnecessary const_cast

eb2e030

jakobbotsch marked this pull request as ready for review November 1, 2024 08:23

jakobbotsch requested a review from MichalStrehovsky as a code owner November 1, 2024 08:23

jakobbotsch requested review from AaronRobinsonMSFT and a team November 1, 2024 10:15

AaronRobinsonMSFT reviewed Nov 1, 2024

View reviewed changes

src/coreclr/jit/codegenlinear.cpp Show resolved Hide resolved

src/coreclr/jit/importervectorization.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Nov 2, 2024

View reviewed changes

src/coreclr/jit/jitconfig.cpp Outdated Show resolved Hide resolved

jakobbotsch added 3 commits November 4, 2024 12:11

Fix freeing of MethodSet::m_list

74a7688

Fix build

c9b9f82

Add fopen_utf8 to fopen an UTF8 path

e293a90

jkotas reviewed Nov 4, 2024

View reviewed changes

jakobbotsch added 2 commits November 4, 2024 12:36

Fix char type

074d969

Fix char type try 2

d2cc12a

Remove unnecessary header

e78f7e6

build-analysis bot mentioned this pull request Nov 4, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

AaronRobinsonMSFT reviewed Nov 4, 2024

View reviewed changes

src/coreclr/jit/utils.cpp Show resolved Hide resolved

Rename m_buffer -> m_bufferUnsafe

140ed65

jakobbotsch mentioned this pull request Nov 5, 2024

fix warnings in coreclr/jit/compiler.cpp for non-Windows targets #104805

Closed

jakobbotsch added 3 commits November 5, 2024 11:11

Fix build

ea58330

Merge branch 'main' of github.com:dotnet/runtime into jit-config-utf8

8fbe6a2

Rerun ThunkGenerator

8c8565b

AaronRobinsonMSFT approved these changes Nov 13, 2024

View reviewed changes

EgorBo reviewed Nov 13, 2024

View reviewed changes

EgorBo approved these changes Nov 13, 2024

View reviewed changes

amanasifkhalid approved these changes Nov 13, 2024

View reviewed changes

jakobbotsch added 2 commits November 14, 2024 11:35

Address feedback

ec4e8e7

Run jit-format

0e22b18

jakobbotsch merged commit c653208 into dotnet:main Nov 14, 2024
92 of 94 checks passed

jakobbotsch deleted the jit-config-utf8 branch November 14, 2024 13:00

build-analysis bot mentioned this pull request Nov 14, 2024

JIT: libraries jitstress Assertion failed 'doesVNMatch' during 'Assertion prop' #109745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Switch config values to UTF8 #109418

JIT: Switch config values to UTF8 #109418

jakobbotsch commented Oct 31, 2024 •

edited

Loading

dotnet-policy-service bot commented Oct 31, 2024

jkotas Oct 31, 2024

jakobbotsch commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jkotas commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jkotas commented Nov 1, 2024

jakobbotsch commented Nov 4, 2024 •

edited

Loading

jkotas commented Nov 4, 2024

jkotas Nov 4, 2024

jakobbotsch Nov 4, 2024

AaronRobinsonMSFT Nov 4, 2024

jakobbotsch Nov 4, 2024

AaronRobinsonMSFT Nov 4, 2024

jakobbotsch Nov 4, 2024

AaronRobinsonMSFT Nov 4, 2024

jakobbotsch Nov 5, 2024

jakobbotsch commented Nov 4, 2024

jakobbotsch commented Nov 13, 2024

EgorBo Nov 13, 2024 •

edited

Loading

jakobbotsch Nov 14, 2024

EgorBo left a comment

amanasifkhalid left a comment

amanasifkhalid Nov 13, 2024

jakobbotsch Nov 14, 2024

	SString str(SString::Utf8, name);
	StackSString str;
	SString(SString::Utf8Literal, name).ConvertToUnicode(str);

JIT: Switch config values to UTF8 #109418

JIT: Switch config values to UTF8 #109418

Conversation

jakobbotsch commented Oct 31, 2024 • edited Loading

dotnet-policy-service bot commented Oct 31, 2024

Choose a reason for hiding this comment

jakobbotsch commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jkotas commented Nov 1, 2024

jakobbotsch commented Nov 1, 2024

jkotas commented Nov 1, 2024

jakobbotsch commented Nov 4, 2024 • edited Loading

jkotas commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Nov 4, 2024

jakobbotsch commented Nov 13, 2024

EgorBo Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo left a comment

Choose a reason for hiding this comment

amanasifkhalid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Oct 31, 2024 •

edited

Loading

jakobbotsch commented Nov 4, 2024 •

edited

Loading

EgorBo Nov 13, 2024 •

edited

Loading