Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup some string operating functions #96099

Merged
merged 16 commits into from
Jan 4, 2024
Merged

Cleanup some string operating functions #96099

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b733ba6
IsStringLongerThan -> strnlen
huoyaoyuan Aug 27, 2023
fd76ebe
StringCchCopy -> u16_strcpy_s
huoyaoyuan Aug 30, 2023
e8d844a
Delete strsafe.h
huoyaoyuan Aug 30, 2023
2794371
Remove FString usage in SString
huoyaoyuan Aug 30, 2023
23947ba
Remove fstring usage from corhlprpriv.h
huoyaoyuan Aug 30, 2023
fa3ff62
Remove fstring
huoyaoyuan Aug 30, 2023
90d51cc
Commit suggestion from review
huoyaoyuan Dec 17, 2023
c756156
Fix compiling
huoyaoyuan Dec 17, 2023
7d91247
Fix null terminator in corhlprpriv.h
huoyaoyuan Dec 25, 2023
0e11ecb
Link utf8.c for Unix minipal
huoyaoyuan Dec 26, 2023
128920f
Apply suggestions from code review
huoyaoyuan Jan 3, 2024
765b4e3
Only add link for utf8 on osx
huoyaoyuan Jan 3, 2024
51da9cd
Flip invariant in SString::ConvertToUTF8
huoyaoyuan Jan 3, 2024
1dd038d
Throw ERROR_NO_UNICODE_TRANSLATION
huoyaoyuan Jan 3, 2024
e9137da
Apply suggestions from code review
huoyaoyuan Jan 4, 2024
466e3d0
Update src/coreclr/utilcode/sstring.cpp
AaronRobinsonMSFT Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/debug/daccess/inspect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,8 +1049,8 @@ ClrDataValue::GetString(
{
*strLen = static_cast<ULONG32>(u16_strlen(msgStr) + 1);
}
status = StringCchCopy(str, bufLen, msgStr) == S_OK ?
S_OK : S_FALSE;

status = u16_strcpy_s(str, bufLen, msgStr) ? S_OK : S_FALSE;
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/stack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ ClrDataFrame::GetArgumentByIndex(
*nameLen = 5;
}

StringCchCopy(name, bufLen, W("this"));
u16_strcpy_s(name, bufLen, W("this"));
}
else
{
Expand Down
5 changes: 0 additions & 5 deletions src/coreclr/debug/daccess/stdafx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@
#include "dacimpl.h"


#define STRSAFE_NO_DEPRECATE
#include <strsafe.h>
#undef _ftcscat
#undef _ftcscpy

// from ntstatus.h
#define STATUS_STOWED_EXCEPTION ((NTSTATUS)0xC000027BL)

Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/debug/daccess/task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,8 +732,7 @@ ClrDataAppDomain::GetName(
}
else
{
status = StringCchCopy(name, bufLen, (PCWSTR)rawName) == S_OK ?
S_OK : S_FALSE;
status = u16_strcpy_s(name, bufLen, (PCWSTR)rawName) ? S_OK : S_FALSE;
if (nameLen)
{
size_t cchName = u16_strlen((PCWSTR)rawName) + 1;
Expand Down Expand Up @@ -4768,7 +4767,7 @@ ClrDataExceptionState::GetString(
message->GetStringLength(),
true);

status = StringCchCopy(str, bufLen, msgStr) == S_OK ? S_OK : S_FALSE;
status = u16_strcpy_s(str, bufLen, msgStr) ? S_OK : S_FALSE;
if (strLen != NULL)
{
size_t cchName = u16_strlen(msgStr) + 1;
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/ilasm/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "clrversion.h"
#include "shimload.h"

#include "strsafe.h"
#define ASSERTE_ALL_BUILDS(expr) _ASSERTE_ALL_BUILDS((expr))

WCHAR* EqualOrColon(_In_ __nullterminated WCHAR* szArg)
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/inc/clr/fs/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@

#include "clrtypes.h"

#include "strsafe.h"

#include "clr/str.h"

namespace clr
Expand Down
62 changes: 11 additions & 51 deletions src/coreclr/inc/corhlprpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#define __CORHLPRPRIV_H__

#include "corhlpr.h"
#include "fstring.h"
#include <minipal/utf8.h>

#if defined(_MSC_VER) && defined(HOST_X86)
#pragma optimize("y", on) // If routines don't get inlined, don't pay the EBP frame penalty
Expand Down Expand Up @@ -225,71 +225,31 @@ class CQuickMemoryBase
iSize = cbTotal;
}


// Convert UTF8 string to UNICODE string, optimized for speed
HRESULT ConvertUtf8_UnicodeNoThrow(const char * utf8str)
{
bool allAscii;
DWORD length;

HRESULT hr = FString::Utf8_Unicode_Length(utf8str, & allAscii, & length);

if (SUCCEEDED(hr))
{
LPWSTR buffer = (LPWSTR) AllocNoThrow((length + 1) * sizeof(WCHAR));

if (buffer == NULL)
{
hr = E_OUTOFMEMORY;
}
else
{
hr = FString::Utf8_Unicode(utf8str, allAscii, buffer, length);
}
}

return hr;
}

// Convert UTF8 string to UNICODE string, optimized for speed
void ConvertUtf8_Unicode(const char * utf8str)
{
bool allAscii;
DWORD length;
size_t sourceLen = strlen(utf8str);
size_t destLen = minipal_get_length_utf8_to_utf16(utf8str, sourceLen, 0);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be LPSTR (UTF8).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement is unchanged (with whitespace off).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length -> destLen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some platforms do not support char16_t,

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


if (SUCCEEDED(hr))
if (!minipal_convert_utf8_to_utf16(utf8str, sourceLen, buffer, destLen, 0))
{
LPWSTR buffer = (LPWSTR) AllocThrows((length + 1) * sizeof(WCHAR));

hr = FString::Utf8_Unicode(utf8str, allAscii, buffer, length);
}

if (FAILED(hr))
{
ThrowHR(hr);
ThrowHR(COR_E_INSUFFICIENTMEMORY);
}
}

// Convert UNICODE string to UTF8 string, optimized for speed
void ConvertUnicode_Utf8(const WCHAR * pString)
{
bool allAscii;
DWORD length;

HRESULT hr = FString::Unicode_Utf8_Length(pString, & allAscii, & length);
size_t sourceLen = u16_strlen(pString);
size_t destLen = minipal_get_length_utf16_to_utf8(pString, sourceLen, 0);

if (SUCCEEDED(hr))
{
LPSTR buffer = (LPSTR) AllocThrows((length + 1) * sizeof(char));

hr = FString::Unicode_Utf8(pString, allAscii, buffer, length);
}
LPSTR buffer = (LPSTR) AllocThrows((length + 1) * sizeof(char));

if (FAILED(hr))
if (!minipal_convert_utf16_to_utf8(pString, sourceLen, buffer, destLen, 0))
{
ThrowHR(hr);
ThrowHR(COR_E_INSUFFICIENTMEMORY);
}
}

Expand Down
44 changes: 0 additions & 44 deletions src/coreclr/inc/fstring.h
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions src/coreclr/inc/utilcode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2834,27 +2834,6 @@ template <class T> class CChainedHash
};


//*****************************************************************************
//
//********** String helper functions.
//
//*****************************************************************************

//*****************************************************************************
// Checks if string length exceeds the specified limit
//*****************************************************************************
inline BOOL IsStrLongerThan(_In_ _In_z_ char* pstr, unsigned N)
{
LIMITED_METHOD_CONTRACT;
unsigned i = 0;
if(pstr)
{
for(i=0; (i < N)&&(pstr[i]); i++);
}
return (i >= N);
}


//*****************************************************************************
// Class to parse a list of simple assembly names and then find a match
//*****************************************************************************
Expand Down
Loading