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

Build PDB file for Windows #1678

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Build PDB file for Windows #1678

wants to merge 3 commits into from

Conversation

zyn0217
Copy link

@zyn0217 zyn0217 commented Jun 20, 2023

This could help with dumping human-readable backtrace symbols with line no.

See also #1641.

This could help with dumping human-readable backtrace symbols with line
no.

See also clangd#1641.
@zyn0217
Copy link
Author

zyn0217 commented Jun 20, 2023

Request for review :)

@sam-mccall @kadircet @hokein

CC @HighCommander4

Copy link
Member

@sam-mccall sam-mccall left a comment

Choose a reason for hiding this comment

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

Sorry about the delay here, I've been missing stuff coming through github :-(

This looks OK to me, I'd probably prefer to just ship the PDB by default unless it's big (say, >50% of clangd.exe) but will leave that up to you.

@@ -17,6 +17,9 @@ If you can, provide a minimal chunk of code that shows the problem (either inlin
Please attach the clangd stderr log if you can. (Usually available from the editor)
If possible, run with `--log=verbose` - note that the logs will include the contents of open files!
If this is a crash, try to put `llvm-symbolizer` on your PATH per the troubleshooting instructions.
(If you're using Windows, place the associated PDB file(debug symbols) in the same directory as
Copy link
Member

Choose a reason for hiding this comment

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

rather than requiring this extra step, it'd be nice just to include the pdb file in the right location in the windows clangd.zip

Do you think it is too large in practice?
I like having the binaries be reasonably compact

(Ideally I think we'd ship by default a tiny debuginfo that just had just enough to symbolize stacktraces - this is trivially small as -gmlt on other platforms shows - but don't want to ask you to dig into that if it's not really obvious)

Copy link
Author

@zyn0217 zyn0217 Sep 7, 2023

Choose a reason for hiding this comment

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

it'd be nice just to include the pdb file in the right location in the windows clangd.zip.

I don't have much development experience on Windows, so I'm not sure if this is a common practice. One example I could recall is Okular, a PDF reader ported for Windows from the KDE project, which delivers the binaries and PDBs separately. For a reference, its compressed binary archive is ~120 MiB (which is larger than ours) with the compressed PDB file at ~60 MiB. As such, I think this is probably fine.

One thing to keep in mind is, we may have to take account of the billing/quota on Github. The document says, we only have 500 MiB free storage and 2000 minutes each month: I'm concerned that the building PDB process could exhaust our resources (Assume the CI is not deployed on a self-hosted service.)

Do you think it is too large in practice?

To be clear, my forked CI produces ~110 MiB archive with single PDB file. (~398 MiB in raw size. This is a build of snapshot 20230620, and I presume it won't change too much at the order of magnitude.)

(Ideally I think we'd ship by default a tiny debuginfo that just had just enough to symbolize stacktraces - this is trivially small as -gmlt on other platforms shows - but don't want to ask you to dig into that if it's not really obvious)

Agree. Unfortunately, it seems MSVC doesn't provide any equivalent that bakes in debug symbols while keeping the binary as small as possible. As per the document, only /Z7 packs the binary with debug symbols, but they "can be substantially larger than files that have no debugging information." (I didn't test this flag, as I'm afraid the substantially increased size would surprise users, and it's probably unacceptable to install a large binary on a Windows PC. I think this is in contrast to Linux where people tend to deploy a single static binary as a service.)

Aside: Maybe some undocumented flags could achieve the goal, but I don't know...

BTW, I don't have the commit access to this repository, so I'd ask you to help me land this PR once I feel it's ready. Thanks in advance. :-)

@zyn0217
Copy link
Author

zyn0217 commented Oct 7, 2023

Sorry for my procrastination; I didn't get my hands on my Windows device for a while ;)

I did a rough test with a raw PDB file of around 760 MiB locally: (Compression method - Archive size - Running time)

  • bzip2: 102 MiB; 34 s
  • deflate: 138 MiB; 77 s
  • deflate64: 128 MiB; 90 s
  • lzma: 74 MiB; 40.7 s
  • ppmd: 92 MiB; 36 s

All of these methods are running with default arguments that 7zip suggests. Of the result, LZMA wins with a fair running time.

@sam-mccall WDYT? Would you mind merging this PR so that we could roll the clangd 17 release as well?

Github imposes a 7 GiB RAM restriction for free action plans, which
might be insufficient for parallel linking jobs to execute on.
@@ -152,6 +152,8 @@ jobs:
"-DCMAKE_CXX_COMPILER=cl"
"-DLLVM_ENABLE_ZLIB=OFF"
"-DLLVM_USE_CRT_RELEASE=MT"
"-DLLVM_ENABLE_PDB=ON"
"-DLLVM_PARALLEL_LINK_JOBS=1"
Copy link
Author

@zyn0217 zyn0217 Oct 7, 2023

Choose a reason for hiding this comment

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

Note I'm turning the parallel link jobs for Windows builder to 1 since there's only 7 GiB RAM available for Github's free action plan. Running these jobs together would somehow hit this bottleneck and results in OOM.

And here's the action link to my successfully built artifacts. (Windows only)

@zyn0217
Copy link
Author

zyn0217 commented Oct 19, 2023

Gently ping @sam-mccall

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.

None yet

2 participants