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.

Sign up for GitHub

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

Jump to bottom

Remove printf implementation #81243

Merged
merged 6 commits into from
Jan 28, 2023
Merged

Remove printf implementation #81243

merged 6 commits into from
Jan 28, 2023

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jan 26, 2023 •
edited
Loading

Replace implementation of file related printfs using the
platform implementation (PAL_fprintf and PAL_vfprintf).

Remove tests for PAL_printf, PAL_vprintf, PAL_fprintf and
PAL_vfprintf since they are all now supplied by the platform.

Left _vsnprintf_s tests to ensure the _TRUNCATE
logic is validated.

Size impact (bytes)

Name Before After Delta
libclrjit.so 3252992 3236608 -16384
libclrjit_universal_arm64_x64.so 2759520 2747232 -12288
libclrjit_universal_arm_x64.so 2431816 2419528 -12288
libclrjit_unix_x64_x64.so 2792264 2771784 -20480
libclrjit_win_x64_x64.so 2771784 2755400 -16384
libclrjit_win_x86_x64.so 2759456 2743072 -16384
libcoreclr.so 7013704 6985032 -28672
libmscordaccore.so 2478400 2453824 -24576

@AaronRobinsonMSFT
Remove PAL_printf and PAL_vprintf
5197be8 
Replace implementation of file related printfs using the
platform implementation (PAL_fprintf and PAL_vfprintf).

Remove tests for PAL_printf, PAL_vprintf, PAL_fprintf and
PAL_vfprintf since they are all now supplied by the platform.
@AaronRobinsonMSFT
Remove _vsnprintf declaration.
dd1cf8d
@AaronRobinsonMSFT
Remove underlying printf impl
f5aae90 
Left _vsnprintf_s tests to ensure the _TRUNCATE
logic is validated.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Jan 26, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review January 27, 2023 00:51
@AaronRobinsonMSFT
Copy link
Member Author

/cc @dotnet/interop-contrib @am11 @mangod9

@AaronRobinsonMSFT
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime-coreclr ilasm

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@jkoritzinsky
Copy link
Member

It looks like the _TRUNCATE behavior in _vsnprintf_s is the same behavior as the C99 vsnprintf. Would it be possible to move our usages of _vsnprintf_s to vsnprintf, either in this PR or a follow-up one?

janvorli
janvorli reviewed Jan 27, 2023
View reviewed changes
src/coreclr/pal/inc/pal.h Outdated Show resolved Hide resolved
@AaronRobinsonMSFT
Copy link
Member Author

Would it be possible to move our usages of _vsnprintf_s to vsnprintf, either in this PR or a follow-up one?

@jkoritzinsky It should be possible, but note that it does have semantic differences. Particularly the return value. It is also worth noting MSVC complains if not using the _s (i.e., secure) APIs. I'm not sure this one applies though. I think a follow-up is warranted to understand use. This PR isn't going to change them because it will introduce broader changes that needed. It is a good follow-up though.

@jkotas
Copy link
Member

jkotas commented Jan 27, 2023

It is also worth noting MSVC complains if not using the _s (i.e., secure) APIs

It is not just MSVC. The static analysis tools that are part of Microsoft release gates are going to complain about it too. Switching back to non-_s APIs means filling paperwork at recurring intervals that explains why it is ok. Not worth it.

@jkoritzinsky
Copy link
Member

Are the _s variants available on non-Windows platforms with the same signatures? It looks like the standard version of vsnprintf_s has a different signature without the second count parameter (and the _s variants are only available in C11). Or are we always going to have a little bit of the PAL for the “Secure CRT” to satisfy the security tooling?

@AaronRobinsonMSFT
Copy link
Member Author

Or are we always going to have a little bit of the PAL for the “Secure CRT” to satisfy the security tooling?

We are likely to always have a bit of this. MSVC doesn't officially support C11 either so the tooling there is going to have rough edges. As the PAL evolves this may change but for right now we need a shim.

@am11
Copy link
Member

am11 commented Jan 27, 2023

MSVC doesn't officially support C11 either

According to CRT team With the advent of two new compiler switches, /std:c11 and /std:c17, we are officially supporting the latest ISO C language standards. which is all true for the VS studio versions we are supporting. ;)

@AaronRobinsonMSFT
Copy link
Member Author

MSVC doesn't officially support C11 either

According to CRT team With the advent of two new compiler switches, /std:c11 and /std:c17, we are officially supporting the latest ISO C language standards. which is all true for the VS studio versions we are supporting. ;)

That is the official statement, but it isn't complete support and that was admitted in the announcement - https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/#whats-not. I'm not saying there isn't some support but it isn't as consistent with other compilers and playing the piecemeal game is tiresome. This also is difficult because of mono and potential code sharing. Yes, we don't share this code, but introducing C11 as a dependency and then realizing that some part can't be used by mono is a bad idea. For now, we should focus on C99 and work towards C11 as more support is enabled. This PR has no need for that at present.

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime-coreclr ilasm

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@build-analysis build-analysis bot mentioned this pull request Jan 28, 2023
Closed
@AaronRobinsonMSFT
Copy link
Member Author

@janvorli and @am11 The failure here is known so the CI is "green". Any other feedback?

am11
am11 approved these changes Jan 28, 2023
View reviewed changes
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

LGTM, nice cleanup! 👍

src/coreclr/tools/superpmi/CMakeLists.txt Show resolved Hide resolved
janvorli
janvorli approved these changes Jan 28, 2023
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit cd3f357 into dotnet:main Jan 28, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the remove_printf branch January 28, 2023 16:44
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Jan 28, 2023
Open
@EgorBo EgorBo mentioned this pull request Jan 31, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Feb 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 approved these changes

@janvorli janvorli janvorli approved these changes

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels
area-PAL-coreclr
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@AaronRobinsonMSFT @jkoritzinsky @jkotas @am11 @janvorli