-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSVC release configurations use /Z7 #9084
Comments
I think we should have the ability to override |
Whoops, looks like I accidentally unassigned @grdowns from this issue. @NancyLi1013 or @grdowns - can you fix my mistake? |
Without it, we won't have debugging information for libraries in release. |
@BillyONeal Thanks. |
What @KindDragon said. There's no reason to remove debugging information; we believe CMake's default setting which includes not even stripped symbols in release mode is an incorrect default. Notably, Windows turning on debug information never increases the size of the resulting program, only in the worst case some intermediate files. All our debugging bits end up going into a PDB, and you always want PDBs even in release builds so that you can interpret stack traces etc. later. |
Thanks for your detailed explanation to this issue. @BillyONeal |
According to the documentation for /Z7 /Zi, it looks like there would be no difference between /Z7 and /Zi in terms of debugging information generated, it's just about where this information is stored. If using /Z7 the debug info is stored directly in the executable / library, while /Zi would produce a separate pdb file. If I were to create nuget packages with just the binary libs and have my pdbs packed in another nuget package, I would not be able to do so with the /Z7 option, only with /Zi. So, why is this not a bug? |
This is technically true, but 'executable / binary' means '.obj' or '.lib', not something you ship to end user machines. When you build what you ship to customers (a DLL or EXE), the linker generates a PDB using the information from all the /Z7'd content. The big problem with /Zi for vcpkg purposes is that it embeds a name of a PDB into the static library or .obj which cannot be renamed, or the linker can't find it, which means we have no means of separating libraries from each other, and that it reduces available parallelism. /Zi made a lot more sense when the average dev box only had 1 or 2 cores (when that MSDN doc was written) and the potential perf improvement gained by being able to reuse type information in different translation units was a bigger savings on average than the loss of parallelism incurred by needing to synchronize compilers' access to the shared PDB. |
You're right about .libs not being shipped to end user machines. However, they get shipped to build agents and developers, and with the working from home and flaky internet connections, it makes sense to minimize the amount we need to download. This way we can get just the libs and if everything runs fine we never need to fetch the pdbs. If this were configurable we could have the choice that for release, for certain platforms, we turn off debug completely. |
Again though, vcpkg doesn't have a means in general to get the resulting PDB to have the right name, and it can't be renamed after creation. The default name is something like It is more than a desire for a policy recommendation here, it's a hard requirement for how vcpkg deploys libs to users. |
Billy, I don't fully understand the challenge you mentioned. |
Yes, but by and large people don't. Without patching every port would stomp on every other port. |
Description
Is there a debug flag being set incorrectly here?
vcpkg/scripts/toolchains/windows.cmake
Line 26 in 7ca7db5
vcpkg/scripts/toolchains/windows.cmake
Line 27 in 7ca7db5
The flag /Z7 appears to be (incorrectly imo) set for release configurations.
https://docs.microsoft.com/en-us/cpp/build/reference/z7-zi-zi-debug-information-format?view=vs-2019
Same applies:
vcpkg/scripts/cmake/vcpkg_configure_meson.cmake
Line 15 in 7ca7db5
vcpkg/scripts/cmake/vcpkg_configure_meson.cmake
Line 16 in 7ca7db5
vcpkg/scripts/cmake/vcpkg_configure_meson.cmake
Line 21 in 7ca7db5
vcpkg/scripts/cmake/vcpkg_configure_meson.cmake
Line 22 in 7ca7db5
Proposed solution
The /Z7 flag is removed from the variables highlighted above.
Additional context
I am interested to find out if there is a genuine reason for setting /Z7 in release configurations as my general understanding is that it is not required and would appear arbitrary.
Thanks!
The text was updated successfully, but these errors were encountered: