Cleanup some string operating functions #96099

huoyaoyuan · 2023-12-16T15:30:38Z

Replace with standard or shared ones.

Please confirm their behavior and performance characteristics are desired.

src/coreclr/inc/corhlprpriv.h

am11 · 2023-12-16T16:56:19Z

src/coreclr/inc/corhlprpriv.h


-        HRESULT hr = FString::Utf8_Unicode_Length(utf8str, & allAscii, & length);
+        LPWSTR buffer = (LPWSTR) AllocThrows((length + 1) * sizeof(WCHAR));


This should be LPSTR (UTF8).

This statement is unchanged (with whitespace off).

length -> destLen

Suggested change

LPWSTR buffer = (LPWSTR) AllocThrows((length + 1) * sizeof(WCHAR));

CHAR16_T* buffer = (CHAR16_T*) AllocThrows((length + 1) * sizeof(CHAR16_T));

dn-u16.h uses WCHAR while minipal/utf-8 uses CHAR16_T. They are not treated as compatible although both typedef-d from wchar_t. Is any reason that compiler supported char16_t can't be used?

Some platforms do not support char16_t, which is why we went with the least supported type to handle coreclr and mono simultaneously.

runtime/src/native/minipal/utf8.h

Lines 21 to 25 in 17594ec

#ifdef TARGET_WINDOWS

typedef wchar_t CHAR16_T;

#else

typedef unsigned short CHAR16_T;

#endif

During this refactoring, it's better to directly allocate the target type; CHAR16_T for UTF16 or char for UTF8 to avoid further confusion.

Some platforms do not support char16_t,

I was asking for the exact platforms and GCC/Clang versions we need to target.

macOS doesn't support char16_t so you can cross the whole IsApplePlatform series off the list. Our least supported versions support C11 and hence char16_t on Linux, so only OS headers are relevant here.

macOS doesn't support char16_t

Asked a friend to confirm that char16_t is supported with latest build tool on macOS. The mentions of macOS doesn't support char16_t are quite old. On cppreference, char16_t is marked as supported by Apple clang. There's also documentation about CChar16 in Swift doc.

Can you confirm again about this?

I think in C mode, we had some problems. I don't remember the details offhand. This is the quick way to find out what doesn't work in the CI:

#ifdef TARGET_WINDOWS typedef wchar_t CHAR16_T; #else - typedef unsigned short CHAR16_T; + typedef char16_t CHAR16_T; #endif

(note that we'd still need CHAR16_T for Windows, so perhaps it's better to keep this investigation separate from this PR)

am11 · 2023-12-16T17:04:05Z

Thanks for starting this work. Ideally we should remove the intermediate wrappers for string conversion and directly use minipal/utf8.h in coreclr/mono/corehost, including windows targets; but it will take a while to get there. (I started the grand change in a branch, but realized that it's best to do in batches to ease up the review like you are doing here)

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

src/coreclr/inc/corhlprpriv.h

huoyaoyuan · 2023-12-26T02:40:13Z

Now tests are passing. I need some assist about link failure on macos.

am11 · 2023-12-26T14:01:44Z

I need some assist about link failure on macos.

Try main...am11:runtime:patch_gh-96099.

huoyaoyuan · 2023-12-26T16:33:30Z

Hmm it is linked in Unix pal, but not minipal. Will it cause duplicated symbols on Linux?

src/coreclr/minipal/Unix/CMakeLists.txt

src/coreclr/debug/daccess/task.cpp

src/coreclr/debug/daccess/inspect.cpp

src/coreclr/utilcode/sstring.cpp

src/coreclr/inc/corhlprpriv.h

src/coreclr/minipal/Unix/CMakeLists.txt

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

src/coreclr/inc/corhlprpriv.h

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

huoyaoyuan · 2024-01-04T06:36:03Z

Since the buffer will always be sufficient, the resultant error code could only be NO_UNICODE_TRANSLATION.
I assume using errno is desired since it's in latest minipal implementation.

AaronRobinsonMSFT

Thanks!

@am11 anything else?

am11

LGTM

src/coreclr/utilcode/sstring.cpp

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

This reverts commit bc81c55.

* Revert "Forward minipal_get_length_utf16_to_utf8 and minipal_convert_utf16_to_utf8 from DAC (#97055)" This reverts commit 1263107. * Revert "Cleanup some string operating functions (#96099)" This reverts commit bc81c55.

* Revert "Forward minipal_get_length_utf16_to_utf8 and minipal_convert_utf16_to_utf8 from DAC (dotnet#97055)" This reverts commit 1263107. * Revert "Cleanup some string operating functions (dotnet#96099)" This reverts commit bc81c55.

huoyaoyuan added 6 commits December 16, 2023 19:39

IsStringLongerThan -> strnlen

b733ba6

StringCchCopy -> u16_strcpy_s

fd76ebe

Delete strsafe.h

e8d844a

Remove FString usage in SString

2794371

Remove fstring usage from corhlprpriv.h

23947ba

Remove fstring

fa3ff62

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 16, 2023

dotnet-issue-labeler bot added the area-VM-coreclr label Dec 16, 2023

build-analysis bot mentioned this pull request Dec 16, 2023

[8.0] Failing to build native components #94823

Closed

am11 reviewed Dec 16, 2023

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

am11 reviewed Dec 16, 2023

View reviewed changes

Commit suggestion from review

90d51cc

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

am11 reviewed Dec 17, 2023

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

am11 reviewed Dec 17, 2023

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

am11 reviewed Dec 17, 2023

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

huoyaoyuan force-pushed the str-op branch from 6a4f535 to 2df0e53 Compare December 17, 2023 11:14

Fix compiling

c756156

huoyaoyuan force-pushed the str-op branch from 2df0e53 to c756156 Compare December 19, 2023 03:02

runfoapp bot mentioned this pull request Dec 19, 2023

Methodical_others test JIT/Methodical/Coverage/copy_prop_byref_to_native_int crashing #69832

Open

Fix null terminator in corhlprpriv.h

7d91247

This was referenced Dec 25, 2023

Tracking issue for CI build timeouts #76454

Closed

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

Link utf8.c for Unix minipal

0e11ecb

am11 reviewed Dec 26, 2023

View reviewed changes

src/coreclr/minipal/Unix/CMakeLists.txt Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Dec 26, 2023

Test failure WindowsAlternateDataStreamOverwrite #83659

Open

AaronRobinsonMSFT reviewed Jan 2, 2024

View reviewed changes

huoyaoyuan and others added 4 commits January 3, 2024 10:53

Apply suggestions from code review

128920f

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Only add link for utf8 on osx

765b4e3

Flip invariant in SString::ConvertToUTF8

51da9cd

Throw ERROR_NO_UNICODE_TRANSLATION

1dd038d

am11 reviewed Jan 3, 2024

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

am11 reviewed Jan 3, 2024

View reviewed changes

src/coreclr/inc/corhlprpriv.h Outdated Show resolved Hide resolved

Apply suggestions from code review

e9137da

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

AaronRobinsonMSFT approved these changes Jan 4, 2024

View reviewed changes

am11 reviewed Jan 4, 2024

View reviewed changes

src/coreclr/utilcode/sstring.cpp Outdated Show resolved Hide resolved

Update src/coreclr/utilcode/sstring.cpp

466e3d0

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

AaronRobinsonMSFT merged commit bc81c55 into dotnet:main Jan 4, 2024
111 checks passed

huoyaoyuan deleted the str-op branch January 5, 2024 02:12

tommcdon mentioned this pull request Jan 15, 2024

[NETSDKE2E][Regression] With VSCode on Linux, debugging a .NET 9 project is failed with Unknown Error 0x80131c3c Unable to attach to CoreCLR. #96747

Closed

janvorli mentioned this pull request Jan 19, 2024

Fix mscordac exports #97210

Closed

AaronRobinsonMSFT mentioned this pull request Jan 21, 2024

System.Diagnostics.Debugger.Break() causes System.Runtime.InteropServices.COMException #97261

Closed

tommcdon added a commit to tommcdon/runtime that referenced this pull request Jan 21, 2024

Revert "Cleanup some string operating functions (dotnet#96099)"

237e0bb

This reverts commit bc81c55.

tommcdon mentioned this pull request Jan 21, 2024

Revert pr#96099 #97264

Merged

github-actions bot locked and limited conversation to collaborators Feb 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup some string operating functions #96099

Cleanup some string operating functions #96099

huoyaoyuan commented Dec 16, 2023

am11 Dec 16, 2023

huoyaoyuan Dec 17, 2023

am11 Dec 17, 2023

am11 Dec 17, 2023

huoyaoyuan Dec 17, 2023

am11 Dec 17, 2023

huoyaoyuan Dec 17, 2023

am11 Dec 17, 2023

huoyaoyuan Dec 18, 2023

am11 Dec 18, 2023

am11 commented Dec 16, 2023

huoyaoyuan commented Dec 26, 2023

am11 commented Dec 26, 2023

huoyaoyuan commented Dec 26, 2023

huoyaoyuan commented Jan 4, 2024

AaronRobinsonMSFT left a comment

am11 left a comment


		HRESULT hr = FString::Utf8_Unicode_Length(utf8str, & allAscii, & length);
		LPWSTR buffer = (LPWSTR) AllocThrows((length + 1) * sizeof(WCHAR));

	LPWSTR buffer = (LPWSTR) AllocThrows((length + 1) * sizeof(WCHAR));
	CHAR16_T* buffer = (CHAR16_T) AllocThrows((length + 1) sizeof(CHAR16_T));

	#ifdef TARGET_WINDOWS
	typedef wchar_t CHAR16_T;
	#else
	typedef unsigned short CHAR16_T;
	#endif

Cleanup some string operating functions #96099

Cleanup some string operating functions #96099

Conversation

huoyaoyuan commented Dec 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 commented Dec 16, 2023

huoyaoyuan commented Dec 26, 2023

am11 commented Dec 26, 2023

huoyaoyuan commented Dec 26, 2023

huoyaoyuan commented Jan 4, 2024

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

am11 left a comment

Choose a reason for hiding this comment