[Opensg-users] OpenSG2: Accumulated changes... part 2: link flags

[Johannes Brunen]

Hi,

on request of Carsten I have split up my 'accumulated changes' into smaller chunks so that it easier to look at them.

This change does only affect the MSVC windows build. OpenSG is traditionally build with the compiler flag /Zi on windows even in the optimized cases. That results in generation of the .pdb database files that can be used for debugging purposes. However, I think that it is recommended to use the following linker flags in that case: /OPT:REF /OPT:ICF in the optimized cases.

From the docs: https://msdn.microsoft.com/en-us/library/bxwfs976.aspx

/OPT:REF eliminates functions and data that are never referenced;
/OPT:ICF to perform identical COMDAT folding. Redundant COMDATs can be removed from the linker output.

By default, /OPT:REF is enabled in non-debug builds.
When /OPT:REF is enabled either explicitly or by default, a limited form of /OPT:ICF is enabled that only folds identical functions.

If /DEBUG<https://msdn.microsoft.com/en-us/library/xe4t6fc1.aspx> is specified, the default for /OPT is NOREF, and all functions are preserved in the image. To override this default and optimize a debugging build, specify /OPT:REF. Because /OPT:REF implies /OPT:ICF, we recommend that you also specify /OPT:NOICF to preserve identical functions in debugging builds. This makes it easier to read stack traces and set breakpoints in functions that would otherwise be folded together. The /OPT:REF option disables incremental linking.

Best,
Johannes

[Carsten Neumann]

Hello Johannes,

Post by Johannes Brunen
This change does only affect the MSVC windows build. OpenSG is
traditionally build with the compiler flag /Zi on windows even in the
optimized cases. That results in generation of the .pdb database files
that can be used for debugging purposes. However, I think that it is
recommended to use the following linker flags in that case: /OPT:REF
/OPT:ICF in the optimized cases.

hmm, this simply exceeds my understanding of the MS tools and their
options, so I'm just going to take your word for it that this is an
improvement :)
Unless I hear some objections here I'm planning to apply this in the
near future. Thanks for the patch,

Cheers,
Carsten

[Marcus Lindblom Sonestedt]

Hi,

We need debugging symbols in release builds.

If OPT:REF / OPT:ICF doesn't conflict with /Zi and pdb-generation, I have
no objections.

Cheers,
/Marcus

Post by Carsten Neumann
Hello Johannes,

Post by Johannes Brunen
This change does only affect the MSVC windows build. OpenSG is
traditionally build with the compiler flag /Zi on windows even in the
optimized cases. That results in generation of the .pdb database files
that can be used for debugging purposes. However, I think that it is
recommended to use the following linker flags in that case: /OPT:REF
/OPT:ICF in the optimized cases.

hmm, this simply exceeds my understanding of the MS tools and their
options, so I'm just going to take your word for it that this is an
improvement :)
Unless I hear some objections here I'm planning to apply this in the
near future. Thanks for the patch,
Cheers,
Carsten
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Opensg-users mailing list
https://lists.sourceforge.net/lists/listinfo/opensg-users

[Johannes]

Hello Marcus and Carsten,

First of, we also do need debugging symbols in release builds.

Microsoft writes on
https://msdn.microsoft.com/en-us/library/windows/desktop/ee416349(v=vs.85).aspx

"You also need to take steps to maintain full compiler optimizations
while generating symbols."

- set Debug Information Format to Program Database (/Zi).
- set Generate Debug Info to Yes (/DEBUG).
- set References to Eliminate Unreferenced Data (/OPT:REF).
- set Enable COMDAT Folding to Remove Redundant COMDATs (/OPT:ICF).

That reference initiated my initial change of the code generation of all
of our libraries and executables since we have started to generate
memory dumps on crash in release code.

The flags are detailed on
https://msdn.microsoft.com/en-us/library/bxwfs976(v=vs.120).aspx

/OPT:REF eliminates functions and data that are never referenced;
/OPT:ICF to perform identical COMDAT folding. Redundant COMDATs can be
removed from the linker output.

Usage of /OPT:REF implicitly sets /OPT:ICF.
Usage of /Zi /DEBUG implicitly sets /OPT:NOREF

/OPT:ICF is controversial. Microsoft recommends to use OPT:NOICF to
preserve identical functions in debugging builds.

"If /DEBUG is specified, the default for /OPT is NOREF, and all
functions are preserved in the image. To override this default and
optimize a debugging build, specify /OPT:REF. Because /OPT:REF implies
/OPT:ICF, we recommend that you also specify /OPT:NOICF to preserve
identical functions in debugging builds. This makes it easier to read
stack traces and set breakpoints in functions that would otherwise be
folded together. The /OPT:REF option disables incremental linking."

Post by Marcus Lindblom Sonestedt
We need debugging symbols in release builds.
If OPT:REF / OPT:ICF doesn't conflict with /Zi and pdb-generation, I
have no objections.

I'm not quite sure on that. Probably the /OPT:REF /OPT:NOICF is the
better combination.

Finally, I think that best we should create some cmake options for that,
so that nobody is negatively affected. Then we can experiment a little
with these settings and see what is best.

What do you think,

Best,
Johannes

[Marcus Lindblom Sonestedt]

Hi Johannes,

Thanks for the explanation!

It sounds like /OPT:ICF is safe, even though the debugging experience might
be a bit degraded (but not totally).

Maybe check and see how much the binary sizes change. If it's only 5-10%,
it's not really worth it, but if we gain more than that by COMDAT folding
then it probably is a good thing.

I suggest defaulting to /OPT:ICF & /OPT:REF and adding a CMAKE option to
revert to the old behaviour (OSG_WINDOWS_LINK_OPTIMIZE=false, true by
default)

Incremental linking is nice to have when hacking on OpenSG, so I can see
that as a selling point for wanting it as a CMAKE flag.

Cheers,
/Marcus

Post by Johannes
Hello Marcus and Carsten,
First of, we also do need debugging symbols in release builds.
Microsoft writes on
https://msdn.microsoft.com/en-us/library/windows/desktop/
ee416349(v=vs.85).aspx
"You also need to take steps to maintain full compiler optimizations
while generating symbols."
- set Debug Information Format to Program Database (/Zi).
- set Generate Debug Info to Yes (/DEBUG).
- set References to Eliminate Unreferenced Data (/OPT:REF).
- set Enable COMDAT Folding to Remove Redundant COMDATs (/OPT:ICF).
That reference initiated my initial change of the code generation of all
of our libraries and executables since we have started to generate
memory dumps on crash in release code.
The flags are detailed on
https://msdn.microsoft.com/en-us/library/bxwfs976(v=vs.120).aspx
/OPT:REF eliminates functions and data that are never referenced;
/OPT:ICF to perform identical COMDAT folding. Redundant COMDATs can be
removed from the linker output.
Usage of /OPT:REF implicitly sets /OPT:ICF.
Usage of /Zi /DEBUG implicitly sets /OPT:NOREF
/OPT:ICF is controversial. Microsoft recommends to use OPT:NOICF to
preserve identical functions in debugging builds.
"If /DEBUG is specified, the default for /OPT is NOREF, and all
functions are preserved in the image. To override this default and
optimize a debugging build, specify /OPT:REF. Because /OPT:REF implies
/OPT:ICF, we recommend that you also specify /OPT:NOICF to preserve
identical functions in debugging builds. This makes it easier to read
stack traces and set breakpoints in functions that would otherwise be
folded together. The /OPT:REF option disables incremental linking."

Post by Marcus Lindblom Sonestedt
We need debugging symbols in release builds.
If OPT:REF / OPT:ICF doesn't conflict with /Zi and pdb-generation, I
have no objections.

I'm not quite sure on that. Probably the /OPT:REF /OPT:NOICF is the
better combination.
Finally, I think that best we should create some cmake options for that,
so that nobody is negatively affected. Then we can experiment a little
with these settings and see what is best.
What do you think,
Best,
Johannes
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Opensg-users mailing list
https://lists.sourceforge.net/lists/listinfo/opensg-users

[Johannes]

Hello Marcus,

I will implement your suggestion and provide a new diff for that. But
before I will take the time to build both versions so that I have
numbers that I can provide. Please stay tuned, it will take one day or
another.

Carsten, please wait before check in. I will give it another try.

Best,
Johannes

[Johannes]

Hi,

attached you can find the size numbers for the release dlls and pdb
files that I have extracted from the opensg master build with and
without the patch of OSGSetupCompiler.cmake. So we would be gaining
around 22.5% of size reduction for the dlls.

Best,
Johannes

[Johannes]

Hi,

finally I have finished the link flag undertaking :-)

Attached you can find the final diff file.

What I have done:

1. Added cmake variable OSG_WINDOWS_LINK_OPTIMIZE which defaults to OFF.
If the variable is ON the /OPT:REF /OPT:ICF are added to the
CMAKE_SHARED_LINKER_FLAGS_DEBUGOPT, CMAKE_SHARED_LINKER_FLAGS_RELEASE,
CMAKE_EXE_LINKER_FLAGS_DEBUGOPT, and CMAKE_EXE_LINKER_FLAGS_RELEASE
variables.

2. I removed the cmake code that tried to handle the INCREMENTAL linking
in the DEBUG case. I have done that for two reasons:

At first, I realized that the code is not working at all.
On my platform MSVC 2013 VC12 the CMAKE_SHARED_LINKER_FLAGS_DEBUG
variable contains the following content at the beginning:
"/debug /INCREMENTAL".

The string replacement code in OSGSetupCompiler.cmake tries to replace
string "INCREMENTAL:YES" with "INCREMENTAL:NO":

STRING(REPLACE "INCREMENTAL:YES" "INCREMENTAL:NO" replacementFlags
${CMAKE_SHARED_LINKER_FLAGS_DEBUG})

That does not work, since the search string is not contained in the
variable.

After that follows the code line:

SET(CMAKE_SHARED_LINKER_FLAGS_DEBUG "/INCREMENTAL:NO
${replacementFlags}" ).

That also won't work, since the "/INCREMENTAL" contained in the variable
replacementFlags is appended to the CMAKE_SHARED_LINKER_FLAGS_DEBUG
variable what finally leads to the following content of variable
CMAKE_SHARED_LINKER_FLAGS_DEBUG:

"/INCREMENTAL:NO /debug /INCREMENTAL msvcprtd.lib msvcrtd.lib".

Thus the incremental linking is still active.

Secondly, it is not desirable to do not incremental linking on the
debug variant which is used at development times and thus requires short
turn around times. I do not know for what reason that code was added to
the OSGCompiler.cmake file but I strongly recommend to relinquish that part.

I hope that this patch pleases all parties.

Best,
Johannes

[Marcus Lindblom Sonestedt]

Hi Johannes!

Thanks for your excellent work!

Sorry for not responding earlier but I've been traveling and it was lost in
the inbox. :(

The space savings are great. Hopefully it will enable some performance
benefit too.

Cheers,
/Marcus

P.S. Good catch on incremental linking too!

Post by Johannes
Hi,
finally I have finished the link flag undertaking :-)
Attached you can find the final diff file.
1. Added cmake variable OSG_WINDOWS_LINK_OPTIMIZE which defaults to OFF.
If the variable is ON the /OPT:REF /OPT:ICF are added to the
CMAKE_SHARED_LINKER_FLAGS_DEBUGOPT, CMAKE_SHARED_LINKER_FLAGS_RELEASE,
CMAKE_EXE_LINKER_FLAGS_DEBUGOPT, and CMAKE_EXE_LINKER_FLAGS_RELEASE
variables.
2. I removed the cmake code that tried to handle the INCREMENTAL linking
At first, I realized that the code is not working at all.
On my platform MSVC 2013 VC12 the CMAKE_SHARED_LINKER_FLAGS_DEBUG
"/debug /INCREMENTAL".
The string replacement code in OSGSetupCompiler.cmake tries to replace
STRING(REPLACE "INCREMENTAL:YES" "INCREMENTAL:NO" replacementFlags
${CMAKE_SHARED_LINKER_FLAGS_DEBUG})
That does not work, since the search string is not contained in the
variable.
SET(CMAKE_SHARED_LINKER_FLAGS_DEBUG "/INCREMENTAL:NO ${replacementFlags}"
).
That also won't work, since the "/INCREMENTAL" contained in the variable
replacementFlags is appended to the CMAKE_SHARED_LINKER_FLAGS_DEBUG
variable what finally leads to the following content of variable
"/INCREMENTAL:NO /debug /INCREMENTAL msvcprtd.lib msvcrtd.lib".
Thus the incremental linking is still active.
Secondly, it is not desirable to do not incremental linking on the debug
variant which is used at development times and thus requires short turn
around times. I do not know for what reason that code was added to the
OSGCompiler.cmake file but I strongly recommend to relinquish that part.
I hope that this patch pleases all parties.
Best,
Johannes
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Opensg-users mailing list
https://lists.sourceforge.net/lists/listinfo/opensg-users

[Carsten Neumann]

Hello Johannes,

Post by Johannes
finally I have finished the link flag undertaking :-)
Attached you can find the final diff file.

thank you for the patch. Everybody involved seemed happy with this, so
I've applied it now.

Cheers,
Carsten