Cleanup native build warnings and unnecessary logos #91563
Conversation
The native build was producing a lot of warnings on Windows due to various compiler options overridings. It also prints logos for the resource compiler, assembler and c++ compiler when used for preprocessing. This change cleans all of these. It introduces custom properties for the compiler options that were previously just overriden on manually removed from the options list. Now the properties enable setting the options like /guard:cf, /guard:ehcont or various /EHxxx per target / directory via these properties, so no overload of options occurs.
|
Tagging subscribers to this area: @hoyosjs Issue DetailsThe native build was producing a lot of warnings on Windows due to various compiler options overridings. It also prints logos for the resource compiler, assembler and c++ compiler when used for preprocessing. This change cleans all of these. It introduces custom properties for the compiler options that were previously just overriden on manually removed from the options list. Now the properties enable setting the options like /guard:cf, /guard:ehcont or various /EHxxx per target / directory via these properties, so no overload of options occurs.
|
eng/native/configurecompiler.cmake
Outdated
| @@ -54,7 +54,19 @@ add_compile_definitions("$<$<CONFIG:CHECKED>:DEBUG;_DEBUG;_DBG;URTBLDENV_FRIENDL | |||
| add_compile_definitions("$<$<OR:$<CONFIG:RELEASE>,$<CONFIG:RELWITHDEBINFO>>:NDEBUG;URTBLDENV_FRIENDLY=Retail>") | |||
|
|
|||
| if (MSVC) | |||
| add_linker_flag(/guard:cf) | |||
|
|
|||
| define_property(TARGET PROPERTY CLR_CONTROL_FLOW_GUARD INHERITED BRIEF_DOCS "/guard:cf flag" FULL_DOCS "/guard:cf flag blablabla") | |||
There was a problem hiding this comment.
I never thought about doing this! This is quite clean. I'll probably use this technique more throughout the repo. Thanks for teaching me about it!
|
The failures are unrelated to the PR |
|
@janvorli This PR has significantly altered MSVC's codegen of clrjit.dll. We currently see the following in our throughput measurements for PRs that compare a clrjit.dll from before this PR to one after:
|
|
@jakobbotsch That is strange, I was verifying the diff between compiler options and found them to be the same, just reordered. I will double check the jit ones though. |
|
I've double checked the options for the JIT source files (diffing the generated compile_commands.json) for Windows and there is really no change. |
I'm quite sure -- I've narrowed it to this PR with a bisection. The above runs are all on Windows-x64 host but in some cases using crossjits. Indeed our native linux-x64 runs do not see these any TP differences.
Is the size of clrjit.dll the same before and after? When I check, clrjit.dll is smaller after this change. I checked a few functions and in some cases it looks like there is less inlining -- did we maybe drop the link-time optimization options? |
|
Hmm, I have actually not verified the linker options, so maybe I've missed something there. Let me check those too. |
|
And neither the linker options differ. |
|
Actually, let me try the release build. I was looking at debug build options. |
|
Ok, found it. The release build is missing the |
ghost
locked as resolved and limited conversation to collaborators
The native build was producing a lot of warnings on Windows due to various compiler options overridings. It also prints logos for the resource compiler, assembler and c++ compiler when used for preprocessing.
This change cleans all of these. It introduces custom properties for the compiler options that were previously just overriden or manually removed from the options list. Now the properties enable setting the options like /guard:cf, /guard:ehcont or various /EHxxx per target / directory via these properties, so no overload of options occurs.