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

Remove PAL_Random and move palrt APIs into the minipal #108999

Merged
merged 30 commits into from
Oct 31, 2024

Conversation

jkoritzinsky
Copy link
Member

Change usages of PAL_Random to instead use the minipal's random bytes APIs.

Move the remaining palrt APIs to the minipal to make them more easily usable outside of CoreCLR's PAL (and also to shrink the CoreCLR PAL). This work was spurred by #107961, where I realized that the set of Win32-like APIs that DNMD needs is equivalent to the set of Win32 API shims provided by palrt. To simplify things, I just decided to move the palrt implementations (with implementations that defer to the abstracted APIs on Windows) to the minipal and remove the palrt component.

Mostly extracted from #107961 when refactoring the new minipal APIs.

{
#endif // __cplusplus

typedef struct minipal_guid__
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
typedef struct minipal_guid__
typedef struct

Copy link
Member Author

Choose a reason for hiding this comment

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

We get a slightly better experience in debuggers by giving a name to the struct instead of just naming the typedef (without the name on the struct, you just get "anonymous struct" which is a little annoying).

src/native/minipal/memory.c Outdated Show resolved Hide resolved
src/coreclr/dlls/mscorpe/pewriter.cpp Outdated Show resolved Hide resolved
src/coreclr/dlls/mscorpe/pewriter.cpp Outdated Show resolved Hide resolved
src/coreclr/md/compiler/regmeta.cpp Outdated Show resolved Hide resolved
src/native/minipal/memory.c Outdated Show resolved Hide resolved
src/native/minipal/guid.c Outdated Show resolved Hide resolved
src/native/minipal/memory.c Outdated Show resolved Hide resolved
#endif // __cplusplus

#ifndef HOST_WINDOWS
inline void* CoTaskMemAlloc(size_t cb)
Copy link
Member

Choose a reason for hiding this comment

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

cc @lambdageek, who originally suggested adding a minipal_ prefix to all minipal APIs for clarity on their source. Up to now, all minipal APIs have consistently followed this naming convention.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be called minicom or something to make it clear that it is not minipal proper?

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Oct 29, 2024

Choose a reason for hiding this comment

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

I gave a prefix to mine in DNCP.

Declaration

Implementation:
Windows
Non-Windows

Copy link
Member Author

Choose a reason for hiding this comment

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

I can move this into the minipal/com directory where I initially had it to make that more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll do that and move this back to that subfolder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the subfolder work is in the DNMD PR itself, so that would be more complicated than necessary here. I'll just prefix it with minicom_ and go back to having an implementation on each platform.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that the include file should be called minicom. I did not mean to prefix the COM defs with it. We would not be able to apply the prefix for whatever is used in MIDL generated files that will lead to inconsistencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I'll move it into a subdirectory instead and set up that like I was planning on doing with DNMD.

@jkoritzinsky
Copy link
Member Author

/ba-g failure is deadletter

@jkoritzinsky jkoritzinsky merged commit 1b27ee9 into dotnet:main Oct 31, 2024
159 of 166 checks passed
@jkoritzinsky jkoritzinsky deleted the more-minipal branch October 31, 2024 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants