Skip to content
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

Hook for debugging shared libraries in Visual Studio #121

Merged
merged 12 commits into from
May 8, 2024

Conversation

juansblanco
Copy link
Contributor

@juansblanco juansblanco commented Apr 16, 2024

Added hook that copies PDB files from the build folder to the package folder, so shared libraries are debuggable with Visual Studio

Docs: conan-io/docs#3694

This post-package hook solves it by copying the .pdb (symbol files) needed for debugging to the package folder so that Visual can find them.
It searches for the corresponding .pdb for every .dll and copies it next to it so that Visual can find them.
It searches for every .dll and copies it's corresponging .pdb next to it so that Visual can find it.

This features isn't enabled by default because .pdb files are heavy compared to the rest of the files in the package.

Related to: conan-io/conan#15797

Closes conan-io/conan#12258

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2024

CLA assistant check
All committers have signed the CLA.

extensions/hooks/copy_pdbs_to_package.py Outdated Show resolved Hide resolved
with open("out.json") as f:
dumpbin_path = json.load(f)[0]

# Find all dll paths in the package folder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to PDBs of static libraries? Debug builds of static libraries also produce PDBs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked at least with opencv and PDBs were not generated for static libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do a conan new cmake_lib + conan build . -s build_type=Debug and you will get a PDB

Copy link

@jcar87 jcar87 Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are leaving PDBs of static libraries explicitly out of scope at the moment - even when you can get the compiler to produce pdbs for static libraries, they are not loaded by the debugger at all - the debugger only loads .pdbs from DLLs and executables.

So we need a different approach for static libraries. First we need to confirm if the linker, when invoked to generate an executable or a DLL, is able to aggregate .pdbs from static libraries, and if this done automatically - the link.exe documentation explicitly says that you cannot tell it where to look for PDBs, so we may have important limitations

I think CMake projects have relatively predictable outcomes, but others don't specify where to save the PDB for a static library when compiled with /Zi and we end up with a vc<ver>.pdb where is the version of visual studio - we need a way , given a static library file, to derive where the pdb is, and it is not clear at the moment what the search mechanism is at the time of invoking the linker

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there are different aspects of this hook, if I understood correctly:

  • The first one is packaging the PDBs. Even if the debugger doesn't load PDBs from static libs automatically (it does if you point VS to the folders containing the PDBs), the PDBs are still necessary for the executables and shared libraries when they link the static library to obtain the symbols/debug information for its own PDB generation.
  • Then the second aspect is loading the PDBs for debugging

If we are building app -> static-lib, and the static-lib hook didn't package the static-lib.pdb, then app linking will still complain that static-lib.pdb was not found and wont' use its debug info?

Then, what I don't fully understand is why excluding the PDBs in the first case, my concern is that the debug information of exes and shared libraries will be incomplete.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said earlier, we were already working and leaving static libraries for the future, as that requires further investigation, apologies this was not clear in the PR description.

The PDBs are generated at multiple stages:

  • by the cl.exe compiler with /Zi. It is possible for multiple cl.exe compilations to write concurrently to the same file, newer versions of visual studio apparently handle this. The name can be arbitrary and not be exactly the library name, and if unspecified visual studio generates a default filename.
  • by link.exe when linking a DLL or an executable

The PDBs are ready by:

  • link.exe when linking a DLL or an executable - it looks for the PDBs of static libraries and object files (but if Im not mistaken, it will not look for the PDBs of shared libraries)
  • by the debugger - it will only look for PDBs for linked binaries (executables and shared libraries)

What we observed with @juansblanco is that a build directory may contain multiple instances of the same .pdb filename, but in different subfolders, with different sizes/contents. And because the name filename can completely arbitrary (libfoo.dll may have a unicorn.pdb file), we are using dumpbin /pdbpath to locate the exact PDB in the build folder, because we asume at the time of running the hook the build folder still exists. This way we can be more robust against arbitrary pdb filenames (in particular for cases like Autotools projects that do not pass a flag to specify the filename, which falls back to some default)

For static libraries, we know that the debugger won't even look for these, but link.exe technically might:
The documentation says this:

The linker looks for the object's PDB first in the absolute path written in the OBJ file, and then in the directory that contains the OBJ file. You can't specify an object's PDB file name or location to the linker.

That is, we cannot tell link.exe where to look for .PDBs, and of the two options it mentions:

  • "the absolute path written in the obj file" - this is the absolute path at the time the .lib and .pdb where generated - so if this is being consumed from a different conan package, especially one built in a different machine and then downloaded, then this path no longer exists
  • "in the directory that contains the obj file" - but we are linking a .lib, not a .obj - does link.exe look inside every .obj inside the .lib and to get the pdb name? could there be multiple names? does it work at all? dumpbin.exe /pdbpath: does not seem to work on static libraries, so we are still investigating

it's the last point that we are trying to clarify - especially as it involves consuming static libraries built on a different machine.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference for static libraries:

when link.exe is invoked to create a shared library or an executable, and we are linking symbols from a static library dependency - during the linker passes it will try to find symbols, then it will map which .obj file inside the static library the symbol is found, and eventually it will query that individual obj file for a .pdb filename. then link.exe will take care of aggregating the .pdb files from the static libraries (and any other .obj files) in the .pdb file for the resulting executable or DLL

This is described and summarised here:
https://zeux.io/2010/11/22/z7-everything-old-is-new-again/

The linker gets paths to all PDB files from object files (or from object files inside static libraries), reads debug information from them, generates a single PDB file, writes it to disk and stores the path to this file in the executable (exe or dll). Debugger reads the PDB path from the executable module and uses the debugging information from that file

Unfortunately the static .lib files do not necessarily have "a" .pdb associated with them - the .obj files inside the archive do. And we have all of the following options:

  • a single .pdb file with the same base name as the .lib file, right next to it. think if the project is a visual studio one, or a cmake-generated visual studio one, this will be the case. If the cmake generator is Ninja or Makefiles, I'm not so sure where the file is placed or what filename it is given.
  • a single .pdb file called vc<ver>.pdb if the compiler is passed /Zi but we don't tell it which file to write - this is the fallback behaviour. AFAIK this is certainly the case for libraries built with Autotools using cl.exe
  • a single .pdb with a different, arbitrary name than the .lib - I've seen some evidence (per error logs online, see here) that this is possible - precompiled headers could be an issue, etc.
  • multiple .pdb - a single library (.lib) with multiple .obj files where each .obj has a different associated .pdb - this is definitely the worst case scenario, but still entirely possible - although this I'd have a harder time reasoning why it would be done like this in the first place.

So unfortunately we cannot ask dumpbin to give us a pdbpath for a static library, and we cannot always rely on libfoo.lib having a lifboo.pdb with the same name, OR inside the same folder. This link provides more insights on reasons for this, for example if you have foo.exe and a foo.dll - they'd have to have .pdb files with different basenames.

When link.exe fails to locate the .pdb for static library dependencies, it issues a warning like this:

libcurl_a_debug.lib(rc2_cbc.obj) : warning LNK4099: PDB 'lib.pdb' was not found with 'libcurl_a_debug.lib(rc2_cbc.obj)' or at 'C:\dev\scaler\center\dlux\lib.pdb'; linking object as if no debug info

But the documentation of visual studio does provide a way to troubleshoot this: https://learn.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-warning-lnk4099?view=msvc-170

We would have to:

  • For every static library in the package, call lib.exe /list filename.lib to get a list of the .obj files inside it
  • For every .obj file inside the library:
    • call lib.exe /extract:object.obj filename.lib to extract the .obj file
    • call dumpbin.exe /section:.debug$T /rawdata filename.obj which will print the path to the .pdb that link.exe would look for (parsing this is tricky because the output looks like this):
RAW DATA #3
  00000000: 04 00 00 00 5A 00 15 15 FE 1B E8 1C A7 04 C6 4E  ....Z...þ.è.§.ÆN
  00000010: 94 FA 95 4F 95 F7 1D 15 01 00 00 00 43 3A 5C 55  .ú.O.÷......C:\U
  00000020: 73 65 72 73 5C 6C 75 69 73 63 5C 2E 63 6F 6E 61  sers\luisc\.cona
  00000030: 6E 32 5C 70 5C 62 5C 7A 6C 69 62 61 34 31 35 39  n2\p\b\zliba4159
  00000040: 65 36 62 65 38 36 39 66 5C 62 5C 62 75 69 6C 64  e6be869f\b\build
  00000050: 5C 44 65 62 75 67 5C 7A 6C 69 62 2E 70 64 62 00  \Debug\zlib.pdb.

which is not ideal - and then, that is the .pdb we need to copy right next to the .lib :D

There are a few simplifications we could make for static libraries:

  • assume a libz.lib in the package_folder has a libz.pdb in the build_folder with the exact same name - and copy that

  • based on the visual studio version, derive the vcxxx.pdb file as a fallback

  • copy all .pdb files recursively found in the build_package right next to the .lib files in the package_folder - maybe this can't hurt? but we've already seen that we may have the same filename repeated multiple times in the build_folder, so how would we disambiguate?

  • do the whole lib.exe /list + lib.exe /extract + dumpbin /section:... but only once, and assume all obj inside the lib have the same .pdb?

:D definitely one to discuss next time

extensions/hooks/copy_pdbs_to_package.py Outdated Show resolved Hide resolved
Copy link

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add a check at the beginning to check for os=Windows and compiler=msvc
  • Throw an exception if vswhere.exe is not found or if it exits with error, or consider doing nothing and printing a warning (making it a best-effort situation)
  • Use the stdout redirection instead of writing things to a file

tests/test_pdb_hook.py Outdated Show resolved Hide resolved
tests/test_pdb_hook.py Outdated Show resolved Hide resolved
tests/test_pdb_hook.py Outdated Show resolved Hide resolved
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach of the hook, we just need to finally come up with an idea to not enable this by default - Separate repo?

@juansblanco
Copy link
Contributor Author

I like the approach of the hook, we just need to finally come up with an idea to not enable this by default - Separate repo?

@RubenRBS We chose to have the hook name starting with _hook_... by default so users will need to rename it to use the hook.
This works because hooks only run if their name starts with hook_...
As we discussed we don't want this hook running by default as it greatly increases the size of packages.

Copy link

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hook LGTM!
Leaving @czoido to comment on the test :D

# Use dumpbin to get the pdb path from each dll
dumpbin_output = StringIO()
conanfile.run(rf'"{dumpbin_path}" /PDBPATH {dll_path}', stdout=dumpbin_output)
dumpbin = str(dumpbin_output.getvalue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, what paths are returned here? I just realized that for example most CCI recipes remove the pdbs in the package method, and if tyhis happens after the package, maybe the pdbs do not exist anymore for the interested recipes - I'm saying that maybe we shoukd have a way to avoid those pdb deletions in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an example of a recipe that does this so I can check how it works? As I understand it dumpbin gets the path of were the PDB is originally located, this is usually (always?) where the library is created which should be the build folder not the package one.

Copy link
Member

@AbrilRBS AbrilRBS Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libxml2 is a good example.
The line you're looking for is something that starts with
rm(self, "*.pdb", in the package() method - let me know if you need any help reproing it, but you might be right that this might point to the build folder so we're ok :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this and everything is working fine as expected!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine. Having a look at https://learn.microsoft.com/en-us/cpp/build/reference/pdbpath?view=msvc-170, it should return the path embedded in the binary, that should be the path in the build folder where the binary was originated. As this is a post_package hook, it should work even if the package() method deletes the pdbs because the hook is executed after the method 😄

@czoido czoido merged commit 3d7e764 into conan-io:main May 8, 2024
3 checks passed
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that I didn't realize earlier. Two very minor issues.

import os
import re
from io import StringIO
from conans.errors import ConanException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from conan.errors import ConanException will be more future-proof


#### [PDBs](extensions/hooks/_hook_copy_pdbs_to_package.py)

Post pacakge hook that copies PDBs to the package folder.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package - typo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Additional arguments for hooks that deal with packages and recipes
7 participants