Skip to content

Use utf conversions from minipal - #89036

Merged
LakshanF merged 9 commits into
dotnet:mainfrom
LakshanF:UtfConversions
Jul 21, 2023
Merged

Use utf conversions from minipal#89036
LakshanF merged 9 commits into
dotnet:mainfrom
LakshanF:UtfConversions

Conversation

@LakshanF

Copy link
Copy Markdown
Contributor

Leverage the UTF conversion functionality needed for EventPipe from minipal.

@LakshanFLakshanF added this to the 8.0.0 milestone Jul 17, 2023
@LakshanFLakshanF self-assigned this Jul 17, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

Leverage the UTF conversion functionality needed for EventPipe from minipal.

Author:LakshanF
Assignees:LakshanF
Labels:

area-NativeAOT-coreclr

Milestone:8.0.0

Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/CMakeLists.txt Outdated
Comment threadsrc/native/minipal/utf8.c Outdated

@am11am11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment threadsrc/native/minipal/utf8.c Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
LakshanFand others added 2 commits July 18, 2023 03:55
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@lewing

Copy link
Copy Markdown
Member

The test failures here are known

Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
ep_char16_t* lpDestStr = NULL;

if (static_cast<int>(len) < 0)
len = strlen(str) + 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need the +1 here? Adding the null terminator is handled explicitly below?

@am11am11Jul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is an existing pattern:

if (len<0)
len= (glong)strlen(str) +1;
glongret= (glong)minipal_get_length_utf8_to_utf16 (str, len, flags);

Reason is, minipal get_length API needs the source length with null-terminator accounted for, so it returns the correct destination count. Handling it within minipal was requiring some additional conditions which we decided to leave out for the caller to handle.

@elinor-fungelinor-fungJul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the length in ep_rt_utf16_to_utf8_string need +1 too, then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is an existing pattern:

It has the same issue of allocating one extra byte than necessary, and it is further complicated by returning items_written.

We do not need to replicate the issue here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jkotas, not sure which issue you are referring to. The easiest way to understand what goes wrong without +1 is to remove it from coreclr version (it's on the callsite of PAL API) and run PAL tests. fb613a5 did the same thing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jkotas, not sure which issue you are referring to.

Allocating buffer that is one character longer than necessary.

For example, if ep_rt_utf8_to_utf16le_string is called with a string that is 1 ascii character + zero terminator, the current code allocates 3 * sizeof(CHAR16_T). It is one CHAR16_T more than what is actually needed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It seems like the pattern with the extra +1 that @am11 mentions is needed to preserve the current behavior. For example, this test fails without +1.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you saying that the event pipe code expects the strings returned by this method to have double zero terminators? It is very atypical contract. If it is really the case, these should be a long comment explaining it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Allocating buffer that is one character longer than necessary.

PAL, COM and reflection tests are very sensitive about "fill the exact slot or fail" cases. So if that was the case, they all will fail. IOW, that +1 effect gets cancelled out how the code in utf8.c works.

Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated

ep_char8_t *str_utf8 = reinterpret_cast<ep_char8_t *>(malloc ((len_utf16 + 1) * sizeof (ep_char8_t)));
if (!str_utf8)
ep_char8_t* lpDestStr = NULL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: rename to str_utf8 and move the declaration down to where it is first assigned.

Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
@LakshanF
LakshanF merged commit c0d7d2d into dotnet:mainJul 21, 2023
@LakshanF
LakshanF deleted the UtfConversions branch July 21, 2023 18:46
@ghostghost locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@LakshanF@lewing@am11@jkotas@elinor-fung