Start to use STL and stdio in superpmi #116422

huoyaoyuan · 2025-06-09T03:03:22Z

~~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.

huoyaoyuan · 2025-06-09T03:05:43Z

src/coreclr/tools/superpmi/superpmi-shim-counter/superpmi-shim-counter.cpp

+std::filesystem::path g_realJitPath{""}; // Destructable objects will be cleaned up and won't leak
+std::filesystem::path g_logPath{""};
+std::filesystem::path g_HomeDirectory{""};
+std::filesystem::path g_DefaultRealJitPath{""};

-void SetDefaultPaths()
+std::unique_ptr<MethodCallSummarizer> g_globalContext = nullptr;


According to https://learn.microsoft.com/windows/win32/dlls/dllmain and other documentations I can find, constructors/destructors for global objects with be respected by dll/so unloading. I haven't test it though.

Global destructors are a bug farm. The problem is that they run while the other threads are stopped at random spot (on Windows) and while other threads are running (on non-Windows). Every time we happen to introduce one by mistake, we end up deleting it later to fix intermittent hangs and crashes.

How does it compare to DllMain (DETACH)? The counter shim writes statistics on unload, which is simulated by PAL. Is there better approach?

DllMain (DETACH)

It has the same problems

Does is mean that things won't be worse if we replace existing DllMain with global objects? Currently the DllMain of JIT is doing shutdown.

Currently the DllMain of JIT is doing shutdown.

If I am reading the code correctly, DllMain of JIT is only called on Windows. It is never called on non-Windows.

Does is mean that things won't be worse if we replace existing DllMain with global objects?

I would expect it to be fine for superpmi. I am not sure about regular JIT.

If I am reading the code correctly, DllMain of JIT is only called on Windows. It is never called on non-Windows.

JIT is loaded by CLRLoadLibrary in LoadAndInitializeJIT, which is implemented in PAL with LoadLibraryExW and does DllMain simulation. It's never unloaded though, but if it is, the DllMain will also be called by PAL.

huoyaoyuan · 2025-06-09T03:51:41Z

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 · 2025-06-09T18:41:56Z

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 · 2025-06-10T08:01:12Z

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 · 2025-06-10T23:22:53Z

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 · 2025-06-11T03:15:04Z

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 · 2025-07-07T13:48:35Z

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 · 2025-07-07T13:52:50Z

@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 · 2025-07-07T20:34:42Z

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 · 2025-07-08T05:29:12Z

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.

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

src/coreclr/tools/superpmi/superpmi-shim-counter/methodcallsummarizer.h

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

AaronRobinsonMSFT · 2025-08-14T20:02:31Z

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

        if (jitLib == NULL)
        {
-            LogError("LoadRealJitLib - LoadLibrary failed to load '%ws' (0x%08x)", jitLibPath, ::GetLastError());
+            LogError("LoadRealJitLib - LoadLibrary failed to load '%s' (%d)", jitLibPath.c_str(), ::GetLastError());


Suggested change

LogError("LoadRealJitLib - LoadLibrary failed to load '%s' (%d)", jitLibPath.c_str(), ::GetLastError());

LogError("LoadRealJitLib - LoadLibrary failed to load '%s' (0x%08x)", jitLibPath.c_str(), ::GetLastError());

Win32 error codes are best expressed as hex.

Win32 error codes are best expressed as hex.

Nit: Win32 error code != HRESULTs. I believe Win32 error codes are typically displayed in decimal. For example, our own Win32Exception prints them as decimal - Console.WriteLine(new Win32Exception(3 /* ERROR_PATH_NOT_FOUND */)); gives System.ComponentModel.Win32Exception (3): The system cannot find the path specified..

This particular case was using hex format, while there are other places using decimal. I tend to keep it as-is for now.

Nit: Win32 error code != HRESULTs.

Agree. That is why I stated Win32 error code :)

I believe Win32 error codes are typically displayed in decimal.

Sadly, yes. That has been the most common experience with user facing scenarios and I continue to believe it isn't a helpful display format. The most common Win32 error codes (for example, 3, 5 ,etc) are sub 10, which display the same. The higher ones are less common and often also used with HRESULT and LSTATUS. Consistently displaying them as hex avoids the common signed issue (that is, using %d instead of %u) and allows for easier pattern recognition in logs.

In a better world, our error system would have something as simple as dlerror() as opposed to the painfully baroque FormatMessage().

it isn't a helpful display format.

I believe that it is the common established display format for Windows error code. For example, compare these search results - the first returns good answer for me, the second one returns confusing garbage.

https://www.bing.com/search?q=windows+error+code+3

https://www.bing.com/search?q=windows+error+code+0x00000003

The higher ones are less common and often also used with HRESULT and LSTATUS.

Right, these are different enums that are not interchangeable. I do not have a problem with using hex for HRESULT.

There are ~100 places that print GetLastError in superpmi. ~85% print it as decimal.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

AaronRobinsonMSFT · 2025-08-15T15:26:35Z

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

JulieLeeMSFT · 2025-09-08T16:25:52Z

@EgorBo, PTAL.

huoyaoyuan added 12 commits June 9, 2025 10:42

Use STL for MethodCallSummarizer

688e2b0

Use std::string for manipulating file name

6df1501

Use std::string for global strings

0ba0e96

Use unique_ptr for destruction

ff5058e

Use RAII for logger

01f4be7

Load JIT with std::string

0a95bc9

Enable C++ 17 for superpmi

84b95f3

Use std::filesystem for path manipulation

9d8e705

Use dlsym since dlopen has been used

137e9e3

Port changes to other shims

65cdfa5

Update g_dataFileName

67d2f11

Cleanup

ce5d486

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 bot added the community-contribution Indicates that the PR has been added by a community member label Jun 9, 2025

huoyaoyuan commented Jun 9, 2025

View reviewed changes

Don't replace . in extension

93224cf

huoyaoyuan added 3 commits June 10, 2025 16:51

Limit result file path

08d99e4

Add comment about destructor

b22e3a9

Fix sign/unsigned mismatch

407af70

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

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

huoyaoyuan added 3 commits August 13, 2025 01:17

Remove C++ 17 and fstream usage

9f1fcc8

Remove usages of std::filesystem::path

70152a8

Don't introduce fstream

4db4066

huoyaoyuan changed the title ~~Start to use C++ 17 STL in superpmi~~ Start to use STL and stdio in superpmi Aug 12, 2025

dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 12, 2025

Cleanup UNIX usage

58183b3

AaronRobinsonMSFT reviewed Aug 12, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Aug 12, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Aug 12, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shim-counter/methodcallsummarizer.h Outdated Show resolved Hide resolved

huoyaoyuan added 4 commits August 13, 2025 10:35

Mark method as private

b3ce44d

make_unique is not available in C++ 11

7e81613

Update LoadLibrary error checking

c169695

Handle long cmdline

f0f1857

jkotas reviewed Aug 13, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp Outdated Show resolved Hide resolved

Use getline instead

b1d2987

jkotas reviewed Aug 13, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp Show resolved Hide resolved

huoyaoyuan added 2 commits August 14, 2025 11:37

Merge branch 'main' into superpmi/stl1

3d10f49

Handle \0 delimiter

899f12d

jkotas reviewed Aug 14, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp Outdated Show resolved Hide resolved

Update src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

16f6ee1

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

AaronRobinsonMSFT reviewed Aug 14, 2025

View reviewed changes

Apply suggestions from code review

dd225a6

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

JulieLeeMSFT requested a review from EgorBo September 8, 2025 16:26

	LogError("LoadRealJitLib - LoadLibrary failed to load '%s' (%d)", jitLibPath.c_str(), ::GetLastError());
	LogError("LoadRealJitLib - LoadLibrary failed to load '%s' (0x%08x)", jitLibPath.c_str(), ::GetLastError());

Start to use STL and stdio in superpmi #116422

Are you sure you want to change the base?

Start to use STL and stdio in superpmi #116422

Uh oh!

Conversation

huoyaoyuan commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Jun 9, 2025

Uh oh!

janvorli commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huoyaoyuan commented Jun 10, 2025

Uh oh!

jkotas commented Jun 10, 2025

Uh oh!

huoyaoyuan commented Jun 11, 2025

Uh oh!

AaronRobinsonMSFT commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Jul 7, 2025

Uh oh!

JulieLeeMSFT commented Jul 7, 2025

Uh oh!

huoyaoyuan commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT commented Aug 15, 2025

Uh oh!

JulieLeeMSFT commented Sep 8, 2025

Uh oh!

Uh oh!

huoyaoyuan commented Jun 9, 2025 •

edited

Loading

janvorli commented Jun 9, 2025 •

edited

Loading

AaronRobinsonMSFT commented Jul 7, 2025 •

edited

Loading