New issue
Advanced search Search tips

Issue 682500 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 700381
issue 82385



Sign in to add a comment

upload clang-cl pdb symbols to…somewhere?

Project Member Reported by thakis@chromium.org, Jan 19 2017

Issue description

It'd be cool for various reasons if we archived clang-cl symbols (symbolized stacks on compiler crashes, profiling, etc).

We tried this once but it broke the bots and then we immediately gave up. Let's try again, and harder this time. (last time's revert which helped: https://chromium.googlesource.com/chromium/src/+/949ce560edea778357f6df5441b68d66144048f8 -- sadly I didn't note the failure output).

Scott: Where should we upload the debug info to?
 
https://chromium-browser-symsrv.commondatastorage.googleapis.com is the endpoint for retrieval. I'm not sure who can set up the auth to write to that though.

Comment 2 by h...@chromium.org, Jan 23 2017

IIRC, the clang invocations were failing without any error message when we tried this. (https://codereview.chromium.org/2213743002#msg9)

We'll also need to be mindful that there seem to be differences in optimization level between CMake's Release and RelWithDebInfo builds (see the "RelWithDebInfo vs Release optimization level?" llvm-dev thread)

Comment 3 by thakis@chromium.org, Jan 24 2017

Oh, thanks for pointing that out! So I guess we want to manually add /Zi to CMAKE_C(XX)_FLAGS instead of using RelWithDebInfo then. Maybe that helps with the errors too.
It's super annoying that cmake doesn't default to making RelWithDebInfo just as efficient as Release, but that's a rant for another day.

You may need to add /DEBUG /OPT:REF /OPT:ICF to the linker flags as well - they are probably missing. The /OPT:REF and /OPT:ICF are important because otherwise turning on /DEBUG disables those options and the binaries get slightly more bloated.

Comment 5 by r...@chromium.org, Jan 24 2017

We should make sure ASan is built with Z7, since we distribute it as a static library. I tried really hard to do that in compiler-rt/CMakeLists.txt, but I'm not confident that it really worked:
  # Use /Z7 instead of /Zi for the asan runtime. This avoids the LNK4099
  # warning from the MS linker complaining that it can't find the 'vc140.pdb'
  # file used by our object library compilations.
  list(APPEND SANITIZER_COMMON_CFLAGS /Z7)
  llvm_replace_compiler_option(CMAKE_CXX_FLAGS "/Z[i7I]" "/Z7")
  llvm_replace_compiler_option(CMAKE_CXX_FLAGS_DEBUG "/Z[i7I]" "/Z7")
  llvm_replace_compiler_option(CMAKE_CXX_FLAGS_RELWITHDEBINFO "/Z[i7I]" "/Z7")


Comment 6 by thakis@chromium.org, Feb 21 2017

https://codereview.chromium.org/2709613004/ seems to work locally at least
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8adfa3476b2aaba341cf01f3b6a8365dcf250c7

commit d8adfa3476b2aaba341cf01f3b6a8365dcf250c7
Author: thakis <thakis@chromium.org>
Date: Thu Mar 09 18:55:06 2017

upload clang pdbs to chrome's symbol server

BUG= 682500 
NOTRY=true

Review-Url: https://codereview.chromium.org/2709613004
Cr-Commit-Position: refs/heads/master@{#455816}

[modify] https://crrev.com/d8adfa3476b2aaba341cf01f3b6a8365dcf250c7/tools/clang/scripts/package.py
[modify] https://crrev.com/d8adfa3476b2aaba341cf01f3b6a8365dcf250c7/tools/clang/scripts/update.py

Comment 8 Deleted

I built a binary with that change that's at http://commondatastorage.googleapis.com/chromium-browser-clang-staging/Win/clang-296321-1.tgz . If I load that into windbg and set the symbol search path, it looks like windbg does download the pdb from our symbol server, and stacks look symbolized.

Here's an example:

0:000> kP
 # Child-SP          RetAddr           Call Site
00 00000000`00a5e618 00000001`3f1110e3 clang!llvm::InstructionSelector::InstructionSelector(void) [E:\b\build\slave\win_upload_clang\build\src\third_party\llvm\lib\CodeGen\GlobalISel\InstructionSelector.cpp @ 24]
01 (Inline Function) --------`-------- clang!llvm::PrettyStackTraceProgram::PrettyStackTraceProgram+0x5 [E:\b\build\slave\win_upload_clang\build\src\third_party\llvm\include\llvm\Support\PrettyStackTrace.h @ 75]
02 00000000`00a5e620 00000001`4245cc85 clang!main(
			int argc_ = 0n1, 
			char ** argv_ = 0x00000000`00ae31b0)+0x73 [E:\b\build\slave\win_upload_clang\build\src\third_party\llvm\tools\clang\tools\driver\driver.cpp @ 309]
03 (Inline Function) --------`-------- clang!invoke_main+0x22 [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 64]
04 00000000`00a5fb70 00000000`773259cd clang!__scrt_common_main_seh(void)+0x11d [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 253]
05 00000000`00a5fbb0 00000000`7745a561 kernel32!BaseThreadInitThunk+0xd
06 00000000`00a5fbe0 00000000`00000000 ntdll!RtlUserThreadStart+0x1d


Is this useful? If so, I can land a roll with this binary.

Source files don't show up, but from what I understand that's expected. (Is that something we should try to set up as well, or do folks use that less?)

Sorry about the clueless questions, I don't know what people expect of their symbol server features.
I guess I'll just push that binary, it's likely an improvement over no symbols at all. Let me know if you want More Stuff though.
I grabbed clang-cl.exe from the .tgz file and used dumpbin to extract the build time stamp, image size, GUID, age, pdb name, etc. Then I used UIforETW's RetrieveSymbols tool to grab the PDB and the binary:

> retrievesymbols {EA4564EA-6D47-405A-B6EE-56969E08EE0F}, 1 clang.pdb
_NT_SYMBOL_PATH=SRV*C:\symbols*https://msdl.microsoft.com/download/symbols;SRV*C:\symbols*https://chromium-browser-symsrv.commondatastorage.googleapis.com
Looking for PDB file EA4564EA6D47405AB6EE56969E08EE0F 1 clang.pdb.
Found file - placed it in C:\symbols\clang.pdb\EA4564EA6D47405AB6EE56969E08EE0F1\clang.pdb.

> retrievesymbols 58C1BB9A 438C000 clang-cl.exe
_NT_SYMBOL_PATH=SRV*C:\symbols*https://msdl.microsoft.com/download/symbols;SRV*C:\symbols*https://chromium-browser-symsrv.commondatastorage.googleapis.com
Looking for PE file clang-cl.exe 58c1bb9a 438c000.
Found file - placed it in C:\symbols\clang-cl.exe\58C1BB9A438c000\clang-cl.exe.

This mirrors the process that a debugger or profiler would use so this tells me that everything has been set up correctly for clang-cl.exe. Even loading crash dumps will work (that's what uploading the image file is for).

Ship it!

BTW, I can never remember the exact details so I consulted my blog. I really only write for myself:
https://randomascii.wordpress.com/2013/03/09/symbols-the-microsoft-way/

Will future builds automatically upload the symbols and image files?

Cc: sebmarchand@chromium.org
Another thing you could check is to crash the compiler and try to load the .dmp in windbg (that would check that you can download the binary you uploaded too), but that's probably less important since we'll generally be using an in-tree binary.

I'm not very familiar with source indexing, Seb could probably point you to the scripts that do that for Chrome and answer questions. It might be a bit useful since the code isn't otherwise in-tree for most people.
Yes, all future builds will have this automatically.
Regarding the other question about source files: that requires an extra step of adding source indexing information (basically a mapping from build-machine-source-paths to version-control-paths and retrieval instructions). This is a "nice to have" that only helps when debugging a binary from the build machine. This is a much less mainstream case for clang so I would ignore that for now. It's cool, but probably not important. I added source indexing to UIforETW's builds but I doubt anyone has ever used it.

We can open a separate bug if we decide to go that route.

The RetrieveSymbols checks that I did verify that downloading the binary works, so loading a .dmp with windbg shouldn't be needed (and, as Scott says, that is a less common case).
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae4a9e6d5186753f2563af342463c9d1fccf91f6

commit ae4a9e6d5186753f2563af342463c9d1fccf91f6
Author: thakis <thakis@chromium.org>
Date: Fri Mar 10 03:12:18 2017

Roll clang 296320:296321.

This rolls by just one revision, which happens to be a no-op change.
What's new (other than picking up recent plugin changes) is that
the packager now uploaded PDBs for Windows to chromium-browser-symsrv.

BUG= 682500 

Review-Url: https://codereview.chromium.org/2738253002
Cr-Commit-Position: refs/heads/master@{#455977}

[modify] https://crrev.com/ae4a9e6d5186753f2563af342463c9d1fccf91f6/tools/clang/scripts/update.py

Blocking: 700381
Status: Fixed (was: Assigned)
Filed bug 700381 for possible source server follow-ups.
Status: Started (was: Fixed)
Hm, I played with this a bit today. After syncing, I loaded third_party\llvm-build\Release+Asserts\bin\clang-cl.exe into windbg. Symbols loaded fine, `bp clang!main` worked, kP worked. So far, so good.

Then I used UIforETW to record a trace while building blink_tests. But hen I opened that in Windows Performance Analyzer and did Trace->Load Symbols, clang-cl symbols didn't appear. The UI says "clang-cl.exe!?", so maybe it's confused that the symbols really are in clang! not clang-cl! -- or I'm holding ETW incorrectly.

Can someone familiar with these tools give this a try (sync past the clang roll in comment 15)?
I'll do some ETW tests. It's quite possible that you're just hitting an ETW bug - they pass incorrect paths to symbol servers and Chrome's symbol server can't handle these wrong paths. UIforETW has some hacks for this built in so that Chrome's symbols get downloaded, but the hack needs a list of PDBs that are affected. I blogged about this last year:

https://randomascii.wordpress.com/2016/11/07/wpa-symbol-loading-is-much-faster-but-broken-for-chrome/

The bug should be fixed in WPA soon. You could work around the bug by uploading the clang-cl symbols twice (once with an extra slash character in the path) but that's just gross.

The bug only affects the *downloading* of symbols so if you debug clang-cl and then profile it then it *should* work, as long as both tools use the same symbol path. So, either you have different symbol paths or else you are hitting *another* WPA bug where it actually ignores _NT_SYMBOL_PATH.

Sigh...

Download symbols on Windows has been a solved problem for about twenty years so I'm not sure why it's such a mess right now. Fix should be shipping soon. In the meantime I'll verify the symbols and document here how to make it work in the interim.

I just tested and the symbols show up fine in WPA, once you work around its bugs.

If using UIforETW you can modify etwpackage\bin\StripChromeSymbols.py. The necessary modification is to add "clang" to the list of PDBs in line 156, so it looks like this:

    for dllName in ["clang", "chrome.exe", "chrome.dll", "blink_web.dll", "content.dll", "chrome_elf.dll", "chrome_watcher.dll", "libEGL.dll", "libGLESv2.dll"]:

Then you can invoke this script manually by selecting the trace in UIforETW, right-clicking, and then clicking the Scripts-> Strip Chrome symbols menu.

If Settings-> Chrome developer is checked then this script will be run automatically, but if you have a previously recorded trace then you'll need to run it manually. Note that this only needs to be done once per clang version.

The script downloads and then tries to strip the symbols. When I ran it the script failed to strip the symbols, but that doesn't matter (that is working around *another* old WPA symbol bug that is mostly fixed now). You can even disable the stripping part of the process by setting strip_and_translate in the script to False.

If the symbols still don't show up then it's probably something wrong in Trace-> Configure Symbol Paths, but I think this will do the job. You can also manually download the symbols with etwpackage\bin\RetrieveSymbols.exe (that's what the script does, ultimately) but then you have to muck about with GUIDs.

The Creators Edition version of WPA should fix all this madness. If I do another release of UIforETW before then I could add clang to the script, but I'm really just looking forward to retiring it entirely.

Status: Fixed (was: Started)
Cool, thanks.
And filed issue 703383 for the compiler-rt thing mentioned in comment 5.

Sign in to add a comment