Skip to content

Conversation

@huoyaoyuan
Copy link
Member

@huoyaoyuan huoyaoyuan commented Jun 9, 2025

As discussed in #112419, there should not be blocker for adopting C++ 17 in development environment. SuperPMI is I/O intensive and can be simplified significantly with std::filesystem.

It's non-starter to introduce C++ 17. Instead, using std::string can also simplify string manipulation a lot.

The end goal is to eliminate PAL simulation of Windows I/O functions. This PR acts as a first step to adopt STL in the dll shim entries.

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 9, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 9, 2025
@huoyaoyuan
Copy link
Member Author

It seems that our linux glibc x64/x86/arm64 build environment aren't ready for C++ 17 STL. Is it because we are targeting the runtime version of lowest supported OS?

@janvorli
Copy link
Member

janvorli commented Jun 9, 2025

We use Ubuntu 22.04 18.04 as cross build target for arm64. That one doesn't have support for c++17. So unless we decided to raise the minimum supported OSes in .NET, we are blocked.

@huoyaoyuan
Copy link
Member Author

Is it OK to use some polyfill for superPMI only? We don't need strict compatibility or reliability here.

If this is too flaky, I can remove C++ 17 usage and convert to C++ 11 STL instad.

@jkotas
Copy link
Member

jkotas commented Jun 10, 2025

I can remove C++ 17 usage and convert to C++ 11 STL instad.

What would you have to give up on by converting to C++ 11?

@huoyaoyuan
Copy link
Member Author

What would you have to give up on by converting to C++ 11?

Everything in std::filesystem, currently just path that automatically converts to platform-native encoding. In superpmi it should be fine to use ANSI variants on Windows.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jul 7, 2025

What would you have to give up on by converting to C++ 11?

Everything in std::filesystem, currently just path that automatically converts to platform-native encoding. In superpmi it should be fine to use ANSI variants on Windows.

std::filesystem is a fine API, but we shouldn't really need it. It does help abstract away a lot, but it leveages C++ isms that people tend to balk at (for example, operator overloads). My preference would be to avoid std::filesystem in all cases and focus on collections and other language affordances that actually make the code simpler and explicit.

C++11 and C++14 are the biggest wins for the runtime repo from an API perspective.

@AaronRobinsonMSFT
Copy link
Member

@huoyaoyuan In fact, I called this API out specifically in the referenced issue #112419 (comment). We shouldn't be using this without a real benefit. I don't see that in this PR.

@JulieLeeMSFT JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 7, 2025
@JulieLeeMSFT
Copy link
Member

My preference would be to avoid std::filesystem in all cases

@huoyaoyuan, Given Aaron's comment to avoid std::filesystem, there isn't much point in pursuing this PR without it. What do you think?

@huoyaoyuan
Copy link
Member Author

The major benefit of std::filesystem in superpmi is actually encoding handling of file names. Given its non-shipping tool, we can really avoid completed support.

I'll convert this PR to std::string and fstream based. They are still beneficial.

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 8, 2025
@AaronRobinsonMSFT
Copy link
Member

/cc @dotnet/jit-contrib PTAL. I'm signed off.

@JulieLeeMSFT
Copy link
Member

@EgorBo, PTAL.

@JulieLeeMSFT JulieLeeMSFT requested a review from EgorBo September 8, 2025 16:26
Copy link
Member

@JulieLeeMSFT JulieLeeMSFT left a comment

Choose a reason for hiding this comment

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

LGTM.

@am11
Copy link
Member

am11 commented Oct 28, 2025

The end goal is to eliminate PAL simulation of Windows I/O functions. This PR acts as a first step to adopt STL in the dll shim entries.

src/native/libs handles the .NET libs' I/O needs in C99 pretty well. Most code under src/native/external is also ISO C. When there is a choice to avoid C++ runtime dependency, we tend to favor it.

@EgorBo EgorBo enabled auto-merge (squash) October 29, 2025 14:41
@huoyaoyuan
Copy link
Member Author

The build analysis failure looks like #121153.

@JulieLeeMSFT
Copy link
Member

/ba-g timeouts related to #121153.

@JulieLeeMSFT
Copy link
Member

/ba-g timeouts related to #121153.

@EgorBo
Copy link
Member

EgorBo commented Nov 3, 2025

/ba-g deadletter

@EgorBo EgorBo merged commit 5f84cb0 into dotnet:main Nov 3, 2025
92 of 97 checks passed
@huoyaoyuan huoyaoyuan deleted the superpmi/stl1 branch November 3, 2025 17:19
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI 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.

7 participants