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.

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

Set callconv in unicodedata callback and update doc #103852

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 22, 2024

toupper/lower in C++ only transforms the case of ASCII characters. It's different than what we expect in coreclr-pal and generally in C#. This change switches to minpal's toupper/lower which takes care of chars with opposite case in 0..0xFFFF range.

Repurposed: set callconv for x86 and update docs.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 22, 2024
Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

@am11 am11 force-pushed the feature/corehost/toupperlower-minipal branch 8 times, most recently from f118d9e to 0b054b2 Compare June 22, 2024 11:39
@huoyaoyuan
Copy link
Member

Is this something we want? The functions in corehost are used for parsing arguments, environment variables, path and runtimeconfig. Does any of these has normalization of non-ASCII text?

@am11
Copy link
Member Author

am11 commented Jun 22, 2024

I had this on my radar for a while #44466 (comment). Now that we have minipal, we can use shared implementation without much of a hassle.

@vitek-karas
Copy link
Member

@am11 could you please measure the impact of this change to the size of the apphost executable? We don't really care about size of dotnet executable or hostfxr,hostpolicy, but apphost is part of every app, so we want it as small as possible.

@am11
Copy link
Member Author

am11 commented Jun 22, 2024

It increases the size of apphost by 16.6 KB on osx-arm64 (before: 125328, after: 141936 in bytes).

If this is unacceptable. I can drop the corehost changes from the PR and keep the others (doc update and win32 build fixes of unicodedata).

@jkotas
Copy link
Member

jkotas commented Jun 23, 2024

Does this fix a bug in any observable behavior?

As far as I can tell, all places that use to_lower should work just fine with the C ASCII-only behavior. I do not think we want to take the binary size hit unless it fixes something material.

@am11
Copy link
Member Author

am11 commented Jun 23, 2024

@jkotas in the linked issue, it was found when runtime was cloned at non-ASCII path and host was trying to compare lowered hostfxr path with the lowered path returned on managed side in unit tests. Other than that, nethost uses it during the arg parsing (which we should anyway avoid). I’ll drop it and repurpose the PR.

@am11 am11 force-pushed the feature/corehost/toupperlower-minipal branch from b1fdd84 to f6e192a Compare June 23, 2024 07:02
@am11 am11 removed the area-Host label Jun 23, 2024
@am11 am11 changed the title Use minipal toupper/lower in corehost Set callconv in unicodedata callback and update doc Jun 23, 2024
src/native/minipal/utils.h Outdated Show resolved Hide resolved
src/native/minipal/utils.h Outdated Show resolved Hide resolved
src/native/minipal/utils.h Outdated Show resolved Hide resolved
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas jkotas merged commit a4c4f17 into dotnet:main Jun 24, 2024
157 of 160 checks passed
@am11 am11 deleted the feature/corehost/toupperlower-minipal branch June 24, 2024 14:20
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants