crashpad_tests won’t work with symbol_level = 0 and use_lld = true |
|||||
Issue descriptionhttps://chromium-review.googlesource.com/c/chromium/src/+/757188#message-0bcc4c745ce769380f49d6de1a2ca696abb16514: Reid: > One of the tests is failing on one of our LLD bots: > https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FCrWinClangLLD_tester%2F39%2F%2B%2Frecipes%2Fsteps%2Fcrashpad_tests%2F0%2Flogs%2FPEImageReader.DebugDirectory%2F0 > > These are the gn args it uses: > > clang_use_chrome_plugins = false > is_chrome_branded = true > is_clang = true > is_component_build = false > is_debug = false > is_official_build = true > llvm_force_head_revision = true > symbol_level = 0 > target_cpu = "x86" > use_lld = true > > I think the important bit here might be symbol_level=0, not use_lld=true. Does the test > get disabled when PDB generation is disabled (symbol_level=0)? (capturing the test failures) > [ RUN ] PEImageReader.DebugDirectory > ../../third_party/crashpad/crashpad/snapshot/win/pe_image_reader_test.cc(56): error: Value of: pe_image_reader.DebugDirectoryInformation(&uuid, &age, &pdbname) > Actual: false > Expected: true > ../../third_party/crashpad/crashpad/snapshot/win/pe_image_reader_test.cc(61): error: Expected: (pdbname.find(self_name)) != (std::string::npos), actual: 4294967295 vs 4294967295 Me: > It doesn’t. > > It’s pretty important to ensure that Crashpad is able to read debug directories from > modules, so I’m not keen on disabling the test. Instead, I’d prefer to make the test > load and a DLL and attempt to read its debug directory, if we could arrange to always > build that DLL with debug info even for a symbol_level = 0 build. > > That way, we’d still get the benefits of symbol_level = 0 for all of the code itself > and the main test executable, and we’d only produce a small .pdb for one module just to > be able to run this test. > > Another side of this, though, is that the ability to generate debug info (including > proper .pdb files) is a pretty important part of the linker, so I find it a little > weird that your chromium-win-clang-lld tester turns off .pdb generation. Is that an > oversight?
,
Nov 9 2017
Reid, I looked at this and couldn’t reproduce with symbol_level = 0 alone. Evidently Microsoft’s link.exe always writes the CodeView record (or maybe that’s only for how we’re invoking it?) lld-link.exe doesn’t. Can you advise on whether this distinction is a behavior difference that lld-link.exe wants to maintain relative to link.exe?
,
Nov 9 2017
,
Nov 9 2017
I’ve verified that link.exe 14.11.25547.0 from MSVS 2017 (15.4.1) always includes a debug directory with a PDB CodeView link even if /DEBUG is absent, where lld-link.exe 6.0.0 from the current Chrome trunk only includes one if /DEBUG is present.
> type test.cc
int main(int argc, char* argv[]) { return 0; }
> cl /nologo test.cc /c /Fotest.obj
test.cc
> link /nologo test.obj /out:test.exe
> dumpbin /headers test.exe
[…]
> lld-link test.obj /out:test.exe
> dumpbin /headers test.exe
[…]
The link-produced version shows a nonzero RVA and size for Debug Directory under OPTIONAL HEADER VALUES, and shows a debug directory with a valid CodeView PDB link (type=cv and Format: RSDS). The lld-link-produced version without /DEBUG shows zero for the Debug Directory’s RVA and size.
,
Nov 9 2017
What I observe when I dump the debug directory that link makes for all of its executables is a single debug directory entry of type IMAGE_DEBUG_TYPE_POGO. I don't see any entries of type IMAGE_DEBUG_TYPE_CODEVIEW.
$ cat t.c
int main() {
}
$ cl -c t.c && link t.obj && llvm-readobj -coff-debug-directory t.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.11.25508.2 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
t.c
Microsoft (R) Incremental Linker Version 14.11.25508.2
Copyright (C) Microsoft Corporation. All rights reserved.
File: t.exe
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
DebugDirectory [
DebugEntry {
Characteristics: 0x0
TimeDateStamp: 2017-11-09 20:56:42 (0x5A04C10A)
MajorVersion: 0x0
MinorVersion: 0x0
Type: POGO (0xD)
SizeOfData: 0x280
AddressOfRawData: 0x14288
PointerToRawData: 0x12888
RawData (
...
)
}
]
LLD doesn't have this POGO debug directory entry, so I think it's pretty reasonable for LLD to not have a debug directory at all.
,
Nov 9 2017
POGO, those are for PGO, right? Surprised that you see those without something like /GL. Now I can’t reproduce what I said in comment 4 either. link.exe absent /DEBUG produces a debug directory with a coffgrp-type entry, which I think are for incremental linking. Anyway, I’ve got the Crashpad-side fix in https://chromium-review.googlesource.com/c/crashpad/crashpad/+/761320 and https://chromium-review.googlesource.com/c/crashpad/crashpad/+/761501, and a Chrome-side fix staged and ready to go with a Crashpad update in Chrome.
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/e2b9ab3ed20227422936a0446068fd443bfc4b19 commit e2b9ab3ed20227422936a0446068fd443bfc4b19 Author: Mark Mentovai <mark@chromium.org> Date: Thu Nov 09 23:17:27 2017 win: Tests shouldn’t freak out when CodeView PDB links are absent crashpad_snapshot_test PEImageReader.DebugDirectory was hanging when crashpad_snapshot_test_image_reader.exe did not have a CodeView PDB link. This occurred when linked by Lexan ld-link.exe without /DEBUG. Bug: chromium:782781 Change-Id: I8fbc4d8decf6ac5e19f7ffeb230fd15d7c40fd51 Reviewed-on: https://chromium-review.googlesource.com/761320 Reviewed-by: Leonard Mosescu <mosescu@chromium.org> [modify] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/snapshot/win/pe_image_reader_test.cc [modify] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/snapshot/win/process_snapshot_win_test.cc [modify] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/util/util.gyp [add] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/util/win/scoped_set_event.cc [add] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/util/win/scoped_set_event.h [modify] https://crrev.com/e2b9ab3ed20227422936a0446068fd443bfc4b19/util/win/session_end_watcher.cc
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/0e3c38a4ca521a7db430ba2fce96536b79c1ae3d commit 0e3c38a4ca521a7db430ba2fce96536b79c1ae3d Author: Mark Mentovai <mark@chromium.org> Date: Thu Nov 09 23:18:51 2017 win: Make ProcessSnapshotTest.CrashpadInfoChild use a loaded module When this test examines a module that doesn’t have a CodeView PDB link, it will fail. Such a link may be missing when linking with Lexan ld-link.exe without /DEBUG. The test had been examining the executable as its module. Since it’s easier to provide a single small module linked with /DEBUG than it is to require that the test executable always be linked with /DEBUG, the test is revised to always load a module and operate on it. The module used is the existing crashpad_snapshot_test_image_reader_module.dll. It was chosen because it’s also used by PEImageReader.DebugDirectory, which also requires a CodeView PDB link. It’s the build system’s responsibility to ensure that crashpad_snapshot_test_image_reader_module.dll is linked appropriately. Crashpad’s own GYP-based build always links with /DEBUG. Chrome’s GN-based Crashpad build will require additional attention at symbol_level = 0. Bug: chromium:782781 Change-Id: I0dda8cd13278b82842263e76bcc46362bd3998df Reviewed-on: https://chromium-review.googlesource.com/761501 Reviewed-by: Leonard Mosescu <mosescu@chromium.org> [modify] https://crrev.com/0e3c38a4ca521a7db430ba2fce96536b79c1ae3d/snapshot/win/pe_image_reader_test.cc [modify] https://crrev.com/0e3c38a4ca521a7db430ba2fce96536b79c1ae3d/test/scoped_module_handle.h
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24e4db15c4bc9840b6830ffe10d75dababbd2343 commit 24e4db15c4bc9840b6830ffe10d75dababbd2343 Author: Mark Mentovai <mark@chromium.org> Date: Fri Nov 10 06:20:20 2017 Update Crashpad to b8f61fdc8b1ee3930afe9373e9a83f24a1c369f3 6f6f8a144d89 Add FileModificationTime 13e17bf90f1e Fix FileModificationTime for Android e2b9ab3ed202 win: Tests shouldn’t freak out when CodeView PDB links are absent 0e3c38a4ca52 win: Make ProcessSnapshotTest.CrashpadInfoChild use a loaded module b8f61fdc8b1e Add TimevalToTimespec and use it to get CurrentTime on macOS Bug: 782781 Change-Id: Id8943d8e245cf3cbdb97ad52bcd664fa9dddab2a Reviewed-on: https://chromium-review.googlesource.com/761878 Commit-Queue: Mark Mentovai <mark@chromium.org> Reviewed-by: Reid Kleckner <rnk@chromium.org> Cr-Commit-Position: refs/heads/master@{#515471} [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/build/secondary/third_party/crashpad/crashpad/snapshot/BUILD.gn [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/build/secondary/third_party/crashpad/crashpad/util/BUILD.gn [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/README.chromium [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/snapshot/win/pe_image_reader_test.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/snapshot/win/process_snapshot_win_test.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/test/filesystem.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/test/filesystem.h [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/test/scoped_module_handle.h [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/file/filesystem.h [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/file/filesystem_posix.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/file/filesystem_test.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/file/filesystem_win.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/misc/time.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/misc/time.h [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/misc/time_test.cc [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/util.gyp [add] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/win/scoped_set_event.cc [add] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/win/scoped_set_event.h [modify] https://crrev.com/24e4db15c4bc9840b6830ffe10d75dababbd2343/third_party/crashpad/crashpad/util/win/session_end_watcher.cc
,
Nov 10 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mark@chromium.org
, Nov 8 2017