Remove printf implementation - #81243

Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged

Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom
AaronRobinsonMSFT:remove_printf

Conversation

@AaronRobinsonMSFT

@AaronRobinsonMSFTAaronRobinsonMSFT commented Jan 26, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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)

NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT added the area-PAL-coreclr only for closed issues label Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT 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

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

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

@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

/azp list

@azure-pipelines

azure-pipelinesBot commented Jan 27, 2023

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr ilasm

@azure-pipelines

azure-pipelinesBot commented Jan 27, 2023

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

@jkoritzinsky

jkoritzinsky commented Jan 27, 2023

Copy link
Copy Markdown
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?

am11
am11 reviewed Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt Outdated
janvorli
janvorli reviewed Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h Outdated
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

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

jkotas commented Jan 27, 2023

Copy link
Copy Markdown
Member

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

jkoritzinsky commented Jan 27, 2023

Copy link
Copy Markdown
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

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

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

am11 commented Jan 27, 2023

Copy link
Copy Markdown
Member

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

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

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

AaronRobinsonMSFT commented Jan 27, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr ilasm

@azure-pipelines

azure-pipelinesBot commented Jan 27, 2023

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

@build-analysisbuild-analysisBot mentioned this pull request Jan 28, 2023
Closed
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jan 28, 2023

Copy link
Copy Markdown
MemberAuthor

@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

@am11am11 left a comment

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.

LGTM, nice cleanup! 👍

Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli approved these changes Jan 28, 2023
View reviewed changes

@janvorlijanvorli left a comment

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.

LGTM, thank you!

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

Reviewers

@janvorlijanvorlijanvorli approved these changes
+1 more reviewer
@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Remove printf implementation - #81243
Merged
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Jan 28, 2023
Merged
Remove printf implementation#81243
AaronRobinsonMSFT merged 6 commits into
dotnet:mainfrom 
AaronRobinsonMSFT:remove_printf
Conversation
@AaronRobinsonMSFT
@AaronRobinsonMSFTAaronRobinsonMSFT
commented
Jan 26, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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)
NameBeforeAfterDelta
libclrjit.so32529923236608-16384
libclrjit_universal_arm64_x64.so27595202747232-12288
libclrjit_universal_arm_x64.so24318162419528-12288
libclrjit_unix_x64_x64.so27922642771784-20480
libclrjit_win_x64_x64.so27717842755400-16384
libclrjit_win_x86_x64.so27594562743072-16384
libcoreclr.so70137046985032-28672
libmscordaccore.so24784002453824-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.
@AaronRobinsonMSFTAaronRobinsonMSFT
added
the 
area-PAL-coreclr
only for closed issues
label
Jan 26, 2023
@AaronRobinsonMSFTAaronRobinsonMSFT
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/cc @dotnet/interop-contrib @am11@mangod9
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp list
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@jkoritzinsky
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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?
am11
am11
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/tests/palsuite/CMakeLists.txt
Outdated

janvorli
janvorli
reviewed
Jan 27, 2023
View reviewed changes
Comment threadsrc/coreclr/pal/inc/pal.h
Outdated

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
jkotas
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
jkoritzinsky
commented
Jan 27, 2023

Copy link


Copy Markdown

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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
am11
commented
Jan 27, 2023

Copy link


Copy Markdown

Member
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
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
AaronRobinsonMSFT
commented
Jan 27, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr ilasm
@azure-pipelines
azure-pipelinesBot
commented
Jan 27, 2023

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 28, 2023

Closed

@AaronRobinsonMSFT
AaronRobinsonMSFT
commented
Jan 28, 2023

Copy link


Copy Markdown

MemberAuthor
@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
@am11am11
left a comment


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.

LGTM, nice cleanup! 👍
Comment threadsrc/coreclr/tools/superpmi/CMakeLists.txt
janvorli
janvorli
approved these changes
Jan 28, 2023
View reviewed changes
@janvorlijanvorli
left a comment


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.

LGTM, thank you!
@AaronRobinsonMSFT
AaronRobinsonMSFT
merged commit cd3f357
into
dotnet:mainJan 28, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT
deleted the

remove_printf

branch
January 28, 2023 16:44
@AaronRobinsonMSFTAaronRobinsonMSFT
mentioned this pull request
Jan 28, 2023

Open

@EgorBoEgorBo
mentioned this pull request
Jan 31, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Feb 28, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@janvorlijanvorlijanvorli approved these changes

+1 more reviewer

@am11am11am11 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-PAL-coreclronly for closed issues

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AaronRobinsonMSFT@jkoritzinsky@jkotas@am11@janvorli