-
Notifications
You must be signed in to change notification settings - Fork 510
Generate version script automatically unix #7929
Conversation
For reference, could you please share some numbers about the impact of this change on the final binary sizes? |
<CustomLinkerArg Include="-Wl,--version-script=$(ExportsFile)" /> | ||
<CustomLinkerArg Include="-Wl,--discard-all" /> | ||
<CustomLinkerArg Include="-Wl,--gc-sections" /> | ||
<CustomLinkerArg Include="-Wunused-command-line-argument" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need -Wunused-command-line-argument
? A comment may be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I thought to long term use all these commands with --exported-symbols. The new version is incompatible with that.
@@ -59,7 +59,8 @@ See the LICENSE file in the project root for more information. | |||
<NativeBinaryExt Condition="'$(NativeCodeGen)' == 'wasm'">.html</NativeBinaryExt> | |||
|
|||
<ExportsFileExt Condition="'$(OutputType)' != 'Exe' and '$(TargetOS)' == 'Windows_NT' and '$(NativeLib)' == 'Shared'">.def</ExportsFileExt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The conditions on ExportsFileExt
can be simplified:
<ExportsFileExt Condition="'$(TargetOS)' == 'Windows_NT'">.def</ExportsFileExt>
<ExportsFileExt Condition="'$(TargetOS)' == 'OSX'">.exports</ExportsFileExt>
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExportsFileExt should only be generated when final binary is a shared library, shouldn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the exports file should be generated only when final binary is shared library.
But it is simpler to set ExportsFileExt
property all the time, even when the exports file is not actually generated. It does not hurt for it to be set even when the value is not actually used.
@@ -59,7 +59,8 @@ See the LICENSE file in the project root for more information. | |||
<NativeBinaryExt Condition="'$(NativeCodeGen)' == 'wasm'">.html</NativeBinaryExt> | |||
|
|||
<ExportsFileExt Condition="'$(OutputType)' != 'Exe' and '$(TargetOS)' == 'Windows_NT' and '$(NativeLib)' == 'Shared'">.def</ExportsFileExt> | |||
<ExportsFileExt Condition="'$(OutputType)' != 'Exe' and '$(TargetOS)' != 'Windows_NT' and '$(NativeLib)' == 'Shared'">.exports</ExportsFileExt> | |||
<ExportsFileExt Condition="'$(OutputType)' != 'Exe' and '$(TargetOS)' == 'OSX' and '$(NativeLib)' == 'Shared'">.exports</ExportsFileExt> | |||
<ExportsFileExt Condition="'$(OutputType)' != 'Exe' and '$(TargetOS)' != 'Windows_NT' and '$(TargetOS)' != 'OSX' and '$(NativeLib)' == 'Shared'">.map</ExportsFileExt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I am not sure how standard is the .map
suffix for version scripts. I have done a quick search https://github.com/search?q=--version-script&type=Code and it is all over the place.
It may be ok to use .exports
for them to keep things simpler. It is what the CoreCLR build does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map is used at https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html
I think that there is no problem with use .exports, really it's just a flat file.
22.396.480 Full.so (Untouched) |
Nice! Does the |
Yes, I'm agree with you. The effect it's usesfull with RyuJITCodeGen because debug information is smaller than it's CppCodeGen version.
It should be clarified that when using strip the resulting file size is the same in all cases by test. |
* Simplification of ExportsFileExt condition. * Same extension for version-script (Linux) and Exported-Symbols-List (OSX). * Separation of version-script fixes and size file reduction changes. * Remove useless -Wunused-command-line-argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thank you!
As we discussed in #s, these changes will allow at shared libraries compilation on unix-like platforms to automatically generate a version-script and remove all local symbols and unused code.
As is said on #7340 (comment) the .so file contains all debug information so we need still remove all this information from file with strip in order to reduce significatly the size of .so file.