Skip to content

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

Closed
dicej wants to merge 1 commit into
dotnet:mainfrom
dicej:compiler-cpp-portability
Closed

fix warnings in coreclr/jit/compiler.cpp for non-Windows targets#104805
dicej wants to merge 1 commit into
dotnet:mainfrom
dicej:compiler-cpp-portability

Conversation

@dicej

@dicejdicej commented Jul 12, 2024

Copy link
Copy Markdown
Contributor

This change was extracted from dotnet/runtimelab#2614, which includes various fixes to enable building on non-Windows systems. We're in the process of upstreaming the parts of that PR which are not specific to NativeAOT-LLVM, and this is the latest such change.

Note that I left a TODO comment in DisplayNowayAssertMap since I'm not sure how to print a LPCWSTR using fprintf on non-Windows systems, but it's clear that the existing code is not portable given the difference between wchar_t on Windows (where it's 16 bits) and other systems (where it's usually 32 bits).

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 12, 2024
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 12, 2024
@dicejdicej mentioned this pull request Jul 12, 2024
Closed
This was referenced Jul 12, 2024
Open
Closed
@dicej
dicejforce-pushed the compiler-cpp-portability branch from 4ebf660 to 760f3dbCompareJuly 12, 2024 22:59
SingleAccretion
SingleAccretion reviewed Jul 14, 2024
View reviewed changes

@SingleAccretionSingleAccretion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dotnet/jit-contrib

Comment threadsrc/coreclr/jit/compiler.cpp Outdated
Comment threadsrc/coreclr/jit/compiler.cpp Outdated
Comment threadsrc/coreclr/jit/compiler.cpp Outdated
Comment threadsrc/coreclr/jit/compiler.cpp Outdated
Comment threadsrc/coreclr/jit/compiler.cpp Outdated
@dicej
dicejforce-pushed the compiler-cpp-portability branch from 760f3db to 94ffdeeCompareJuly 15, 2024 22:45
@dicej
fix warnings in coreclr/jit/compiler.cpp for non-Windows targets
250a96f
This change was extracted from dotnet/runtimelab#2614,
which includes various fixes to enable building on non-Windows systems. We're
in the process of upstreaming the parts of that PR which are not specific to
NativeAOT-LLVM, and this is the latest such change.
Note that I left a TODO comment in `DisplayNowayAssertMap` since I'm not sure
how to print a `LPCWSTR` using `fprintf` on non-Windows systems, but it's clear
that the existing code is not portable given the difference between `wchar_t` on
Windows (where it's 16 bits) and other systems (where it's usually 32 bits).
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej
dicejforce-pushed the compiler-cpp-portability branch from 94ffdee to 250a96fCompareJuly 15, 2024 22:46
dicej added a commit to dicej/runtimelab that referenced this pull request Jul 17, 2024
@dicej
Linux cross compile build fixes
d97382f
This borrows from dotnet/runtime#104805
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej added a commit to dicej/runtimelab that referenced this pull request Jul 17, 2024
@dicej
Linux cross compile build fixes
91e4e26
This borrows from dotnet/runtime#104805
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicejdicej mentioned this pull request Jul 17, 2024
Merged
@JulieLeeMSFT

JulieLeeMSFT commented Aug 5, 2024

Copy link
Copy Markdown
Member

We will review it next week and merge as .NET 10.

@JulieLeeMSFTJulieLeeMSFT added this to the 10.0.0 milestone Aug 5, 2024
@JulieLeeMSFT

JulieLeeMSFT commented Sep 3, 2024

Copy link
Copy Markdown
Member

@EgorBo, please review the community PR.

EgorBo
EgorBo reviewed Sep 9, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/compiler.cpp
if (fout == nullptr)
{
#if !defined(HOST_WINDOWS)
// TODO: how do we print a `const char16_t*` portably?

@EgorBoEgorBoSep 9, 2024

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.

There are a lot more %ws in the jit codebase, why this one specifically is removed?

@EgorBoEgorBoSep 9, 2024

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.

e.g.

JITDUMP("Trying to unroll MemoryExtensions.Equals|SequenceEqual|StartsWith(op1, \"%ws\")...\n", str)

@dicejdicejSep 9, 2024

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.

This is the only one the compiler complained about when building NativeAOT-LLVM on non-Windows systems. I haven't dug into why that is, but my guess is that either other cases of %ws aren't included in the NativeAOT-LLVM build, or else they're being used with expressions which are of type wchar_t* rather than char16_t*; the former is portable when used with %ws, but the latter is not.

@SingleAccretionSingleAccretionSep 9, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reason this doesn't warn in other cases is because they are JITDUMPs, which compile down to logfs, which the compiler doesn't recognize as a printf-style function. I wonder if these other usages actually work properly on not-Windows.

@EgorBo

EgorBo commented Sep 9, 2024

Copy link
Copy Markdown
Member

how do we print a const char16_t* portably?

Perhaps, @AaronRobinsonMSFT@jkoritzinsky know?

@AaronRobinsonMSFT

AaronRobinsonMSFT commented Sep 9, 2024

Copy link
Copy Markdown
Member

how do we print a const char16_t* portably?

Perhaps, @AaronRobinsonMSFT@jkoritzinsky know?

We don't support that at present and it should be avoided. Convert to UTF-8 and log or display to prompt. I assume this is a non-product scenario, which means conversions should be performed or in the ideal case everything done in UTF-16 and then converted a single time. If this is a product scenario, we should discuss this.

@JulieLeeMSFT

JulieLeeMSFT commented Oct 21, 2024

Copy link
Copy Markdown
Member

Convert to UTF-8 and log or display to prompt.

@dicej, please feel free to reach out to us if you need help on how to do this conversion.

@JulieLeeMSFTJulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 21, 2024
@jakobbotschjakobbotsch mentioned this pull request Oct 31, 2024
Merged
@dicej

dicej commented Nov 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Convert to UTF-8 and log or display to prompt.

@dicej, please feel free to reach out to us if you need help on how to do this conversion.

Sorry for the late reply. I guess I do need some guidance on how to do this in a way that would be acceptable for this PR. Is there an existing UTF-16-to-UTF-8 converter in the .NET codebase I could call? I see there's a minipal_convert_utf16_to_utf8 function in src/native/minipal/utf8.c, for example.

I imagine that the user will only see sane output for non-ASCII characters if they're using a terminal and locale compatible with UTF-8, correct? I guess the "right" way to do this would be to check LANG and/or similar locale-related environment variables and target the encoding specified there; but maybe that's overboard in this situation.

@dotnet-policy-servicedotnet-policy-serviceBot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Nov 5, 2024
@jakobbotsch

jakobbotsch commented Nov 5, 2024

Copy link
Copy Markdown
Member

Note that #109418 will subsume this PR, so at this point you might just consider waiting for that one.

@dicej

dicej commented Nov 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Closing this per @jakobbotsch's comment above.

@dicejdicej closed this Nov 5, 2024
@jakobbotsch

jakobbotsch commented Nov 14, 2024

Copy link
Copy Markdown
Member

Actually, looking at this PR again, there are still a few non-UTF16 related changes here that #109418 did not subsume. Do you mind updating this PR to just resolve those?

@dicej

dicej commented Nov 14, 2024

Copy link
Copy Markdown
ContributorAuthor

GitHub wouldn't let me re-open this after rebasing, so I opened a new one: #109830

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 16, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBoEgorBoEgorBo left review comments
+1 more reviewer
@SingleAccretionSingleAccretionSingleAccretion left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dicejdicej

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@dicej@JulieLeeMSFT@EgorBo@AaronRobinsonMSFT@jakobbotsch@SingleAccretion