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>
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>
Change-Id: Ibecedd195224ea53ff36f376897a6ff3c4e773d2
Reviewed-on: https://chromium-review.googlesource.com/757085
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
This was previously proposed at
https://chromium-review.googlesource.com/c/crashpad/crashpad/+/339103/2/util/win/pe_image_reader_test.cc#84.
It didn’t land because the change was abandoned for other reasons, but
the fix was valid. nsi.dll is not VFT_APP or VFT_DLL, and if it’s
loaded, crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
fails.
Although I can’t reproduce nsi.dll being loaded spontaneously in local
testing or on trybots, it occurred in the monolithic crashpad_tests at
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/64492:
[ RUN ] PEImageReader.VSFixedFileInfo_AllModules
../../third_party/crashpad/crashpad/snapshot/win/pe_image_reader_test.cc(90): error: Value of: observed.dwFileType == VFT_APP || observed.dwFileType == VFT_DLL
Actual: false
Expected: true
Google Test trace:
../../third_party/crashpad/crashpad/snapshot/win/pe_image_reader_test.cc(164): C:\Windows\syswow64\NSI.dll
[ FAILED ] PEImageReader.VSFixedFileInfo_AllModules (11 ms)
I can also reproduce locally by calling LoadLibrary(L"nsi.dll").
Bug: chromium:779790, chromium:782011
Test: crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
Change-Id: I361c7d6521645913277a441ce38779aaa4a182c2
Reviewed-on: https://chromium-review.googlesource.com/757077
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Instead of individual per-directory test executables like
crashpad_util_test, all Crashpad tests in Chromium will be run from a
single crashpad_tests executable.
Test: crashpad_util_test Paths.Executable, ProcessInfo.Self; crashpad_snapshot_test PEImageReader.DebugDirectory
Bug: chromium:779790
Change-Id: If95272fd641734fbdb8e231fbcdc4e7ccb2cb822
Reviewed-on: https://chromium-review.googlesource.com/749303
Reviewed-by: Scott Graham <scottmg@chromium.org>
These are mostly -Wsign-compare warnings, with a -Wconstant-conversion
and a -Wunguarded-availability thrown in.
Bug: chromium:779790
Change-Id: Ic2103f3332ce57378db83eca7fa2569efec1a7b6
Reviewed-on: https://chromium-review.googlesource.com/746081
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
This uses “static” at function scope to avoid making local copies, even
in cases where the compiler can’t see that the local copy is
unnecessary. “constexpr” adds additional safety in that it prevents
global state from being initialized from any runtime dependencies, which
would be undesirable.
At namespace scope, “constexpr” is also used where appropriate.
For the most part, this was a mechanical transformation for things
matching '(^| )const [^=]*\['.
Similar transformations could be applied to non-arrays in some cases,
but there’s limited practical impact in most non-array cases relative to
arrays, there are far more use sites, and much more manual intervention
would be required.
Change-Id: I3513b739ee8b0be026f8285475cddc5f9cc81152
Reviewed-on: https://chromium-review.googlesource.com/583997
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
I opted to leave casts to types that were definitely the same size
alone. reinterpret_cast<uintptr_t>(pointer) and
reinterpret_cast<intptr_t>(pointer) should always be safe, for example.
Casts to other integral types have been replaced with
FromPointerCast<>(), which does zero-extension or sign-extension based
on the target type.
To make it possible to use FromPointerCast<>() with some use sites that
were already using checked_cast<>(), FromPointerCast<>() now uses
check_cast<>() when converting to a narrower type.
Test: crashpad_util_test FromPointerCast*, others
Change-Id: I4a71b4aa2d87f545c75524290a702f5f3138d675
Reviewed-on: https://chromium-review.googlesource.com/489701
Reviewed-by: Scott Graham <scottmg@chromium.org>
gtest used to require (expected, actual) ordering for arguments to
EXPECT_EQ and ASSERT_EQ, and in failed test assertions would identify
each side as “expected” or “actual.” Tests in Crashpad adhered to this
traditional ordering. After a gtest change in February 2016, it is now
agnostic with respect to the order of these arguments.
This change mechanically updates all uses of these macros to (actual,
expected) by reversing them. This provides consistency with our use of
the logging CHECK_EQ and DCHECK_EQ macros, and makes for better
readability by ordinary native speakers. The rough (but working!)
conversion tool is
https://chromium-review.googlesource.com/c/466727/1/rewrite_expectassert_eq.py,
and “git cl format” cleaned up its output.
EXPECT_NE and ASSERT_NE never had a preferred ordering. gtest never made
a judgment that one side or the other needed to provide an “unexpected”
value. Consequently, some code used (unexpected, actual) while other
code used (actual, unexpected). For consistency with the new EXPECT_EQ
and ASSERT_EQ usage, as well as consistency with CHECK_NE and DCHECK_NE,
this change also updates these use sites to (actual, unexpected) where
one side can be called “unexpected” as, for example, std::string::npos
can be. Unfortunately, this portion was a manual conversion.
References:
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#binary-comparison77d6b17338https://github.com/google/googletest/pull/713
Change-Id: I978fef7c94183b8b1ef63f12f5ab4d6693626be3
Reviewed-on: https://chromium-review.googlesource.com/466727
Reviewed-by: Scott Graham <scottmg@chromium.org>
Kasko needs a way to read crash keys from out of process. This API
reuses the functionality of PEImageAnnotationsReader.
Change-Id: I2f3bbc358212e6f50235183e9dbb4e5a2cf989cf
This is a reupload of https://codereview.chromium.org/1586433003/ but
for gerrit.
Change-Id: I2f3bbc358212e6f50235183e9dbb4e5a2cf989cf
Reviewed-on: https://chromium-review.googlesource.com/322550
Reviewed-by: Scott Graham <scottmg@chromium.org>
Tested-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Scott Graham <scottmg@google.com>
The BaseName() was added because system DLLs were being reported by
GetFileVersionInfo()/VerQueryValue() as having major file versions of
6.2 instead of 10.0 on Windows 10 when accessed by full path, but not
by BaseName(). The PEImageReader gets the correct version from the
in-memory images, 10.0.
This trick didn't work on Windows XP, where two copies of comctl32.dll
were found loaded into the process, one with a major file version of
5.82 and the other with 6.0. Giving GetFileVersionInfo() the BaseName()
would result in it returning information from one of these, which would
cause the version to not match when the PEImageReader was looking at the
other.
All of these GetFileVersionInfo() quirks make me glad that we're not
using it anymore (outside of the test).
Because of the version numbers involved (NT 6.2 = Windows 8, where
GetVersion()/GetVersionEx() start behaving differently for
non-manifested applications) and the fact that GetFileVersionInfo()
and VerQueryValue() seem to report 10.0 even with full paths on Windows
10 in applications manifested to run on that OS, the BaseName() thing is
restricted to Windows 8 and higher.
TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
BUG=crashpad:78
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1493933002 .
Don't call GetFileVersionInfo(), which calls LoadLibrary() to be able to
access the module's resources. Loading modules from the crashy process
into the handler process can cause trouble. The Crashpad handler
definitely doesn't want to run arbitrary modules' module initializer
code.
Since the VS_FIXEDFILEINFO needed is already in memory in the remote
process' address space, just access it from there.
BUG=crashpad:78
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1475023004 .
A few function implementations that were missing, various switches
for functions/functionality that didn't exist on XP, and far too long
figuring out what exactly was wrong with SYSTEM_PROCESS_INFORMATION
on x86 (the "alignment_for_x86" fields).
R=mark@chromium.org
BUG=crashpad:1, crashpad:50, chromium:531663
Review URL: https://codereview.chromium.org/1336823002 .
