search

intel / intel-ipsec-mb

Intel(R) Multi-Buffer Crypto for IPSec
BSD 3-Clause "New" or "Revised" License
292 stars 88 forks source link

Always generate PDBs, even in release builds. #93

Closed BillyONeal closed 2 years ago

BillyONeal commented 2 years ago

Description / Motivation and Context

PDBs are necessary for postmortem debugging and for teams to get reports from Watson, so they should be built for released software too. This change always passes /DEBUG to the MSVC linker. That switch controls whether a PDB is generated (as in "generate debugging information") not a statement that the resulting files are the debug specific version of your library.

/DEBUG changes the default values of /INCREMENTAL (to on) and /OPT:REF and /OPT:ICF to off, so those are changed back to incremental off and linker optimizations on in debug builds.

(This missing PDB issue was discovered in fixing intel-ipsec's port in vcpkg to build on Windows)

Affected parts

How Has This Been Tested?

This patch will be being applied by Microsoft/vcpkg to the intel-ipsec port in order to make that port build successfully on Windows. (Previously there was no attempt to support Windows in that port)

(That's not landed yet but I tested and it now generates all the correct files:

image

)

Types of changes

Checklist:

tkanteck commented 2 years ago

Thanks Billy for the pull request. I wasn't aware that there is vcpkg of the library. I'd be very happy to learn more about it. Is this work related to SPDK maybe?

Should PDB files installed somewhere in the system, too? Should install makefile target be modified?

MSDN link notes from my reading :) feel free to skip

When you specify /DEBUG with no additional options, the linker defaults to /DEBUG:FULL

The /DEBUG:FULL option moves all private symbol information from individual compilation products (object files and libraries) into a single PDB, and can be the most time-consuming part of the link. However, the full PDB can be used to debug the executable when no other build products are available, such as when the executable is deployed.

/INCREMENTAL is implied when /DEBUG is specified.

/DEBUG changes the defaults for the /OPT option from REF to NOREF and from ICF to NOICF, so if you want the original defaults, you must explicitly specify /OPT:REF or /OPT:ICF. MSDN more link reading LINK first processes options specified in the LINK environment variable, followed by options in the order they are specified on the command line and in command files. If an option is repeated with different arguments, the last one processed takes precedence.

Looking into the pull request and final linker command lines:

BillyONeal commented 2 years ago

Thanks Billy for the pull request. I wasn't aware that there is vcpkg of the library. I'd be very happy to learn more about it. Is this work related to SPDK maybe?

It's been in vcpkg since about 3 years ago: https://github.com/microsoft/vcpkg/pull/5586

The missing PDBs was noticed as part of an attempt to update it to a current version: https://github.com/microsoft/vcpkg/pull/22269

Should PDB files installed somewhere in the system, too? Should install makefile target be modified?

There's no "default" location to install things on Windows so I'm not sure your install target should do anything differently. If you do choose to do that next to the binary is OK.

Will /DEBUG override previous /OPT:REF /OPT:ICF settings?

It shouldn't as far as I am aware but I didn't dig too closely to ensure COMDAT folding happened. I just read that MSDN page and added the options :)

tkanteck commented 2 years ago

Thanks. If this is OK I'll experiment a bit with the sequence of linker options and get back to you

tkanteck commented 2 years ago

Thanks! I'll update install target too. I experimented with these settings a bit. I admit that it was a bit time consuming. Depending on the order of options final PDB size differs. Luckily DLL remains the same every time :)

My suggestion is to make RELEASE build link command line look like this: link /RELEASE /DEBUG /OPT:REF /OPT:ICF /INCREMENTAL:NO ...

If you are OK with it I'll update your PR with this change and pursue with merging it.

BillyONeal commented 2 years ago

As long as PDBs come out I'm happy :D

tkanteck commented 2 years ago

Thanks a mil! merged in https://github.com/intel/intel-ipsec-mb/commit/19d3b3fca689fb87e819b0b27bc74d1f52fcd2c8