From dd4859965f90d873fd5927453ae156af4fbc48a6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 9 Nov 2016 11:29:59 -0500 Subject: [PATCH 1/9] Update compat version of winnt.h to 10.0.14393.0 SDK (Windows 10 1607) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This defines PROCESSOR_ARCHITECTURE_ARM64 and PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64, usable in MINIDUMP_SYSTEM_INFO::ProcessorArchitecture. This also defines four new PF_* flags usable in CPU_INFORMATION::OtherCpuInfo::ProcessorFeatures. Definitions are provided in compat/non_win, and #ifdef-guarded definitions in compat/win for compatibility with Windows SDKs older than Chrome’s minimum requirement. PROCESSOR_ARCHITECTURE_ARM64 means the same thing that Breakpad used the value 0x8003 for. At this point, Crashpad aims to use the officially-defined constant. In the minidump_extensions.h MinidumpCPUArchitecture enum, 0x8003 remains present and documented as deprecated to discourage reuse of that constant for another purpose. BUG= Change-Id: Ic4b5fb9de31c5f00f3698f112633ece2a036b889 Reviewed-on: https://chromium-review.googlesource.com/409098 Reviewed-by: Scott Graham --- compat/non_win/winnt.h | 6 ++++++ compat/win/winnt.h | 10 ++++++++-- minidump/minidump_extensions.h | 17 +++++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/compat/non_win/winnt.h b/compat/non_win/winnt.h index 82af0e6d..5e46f535 100644 --- a/compat/non_win/winnt.h +++ b/compat/non_win/winnt.h @@ -65,6 +65,8 @@ #define PROCESSOR_ARCHITECTURE_AMD64 9 #define PROCESSOR_ARCHITECTURE_IA32_ON_WIN64 10 #define PROCESSOR_ARCHITECTURE_NEUTRAL 11 +#define PROCESSOR_ARCHITECTURE_ARM64 12 +#define PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64 13 #define PROCESSOR_ARCHITECTURE_UNKNOWN 0xffff //! \} @@ -104,6 +106,10 @@ #define PF_ARM_EXTERNAL_CACHE_AVAILABLE 26 #define PF_ARM_FMAC_INSTRUCTIONS_AVAILABLE 27 #define PF_RDRAND_INSTRUCTION_AVAILABLE 28 +#define PF_ARM_V8_INSTRUCTIONS_AVAILABLE 29 +#define PF_ARM_V8_CRYPTO_INSTRUCTIONS_AVAILABLE 30 +#define PF_ARM_V8_CRC32_INSTRUCTIONS_AVAILABLE 31 +#define PF_RDTSCP_INSTRUCTION_AVAILABLE 32 //! \} //! \anchor IMAGE_DEBUG_MISC_x diff --git a/compat/win/winnt.h b/compat/win/winnt.h index 2c5ac662..674b70e9 100644 --- a/compat/win/winnt.h +++ b/compat/win/winnt.h @@ -15,6 +15,9 @@ #ifndef CRASHPAD_COMPAT_WIN_WINNT_H_ #define CRASHPAD_COMPAT_WIN_WINNT_H_ +// include_next +#include <../um/winnt.h> + // https://msdn.microsoft.com/en-us/library/windows/desktop/aa373184.aspx: // "Note that this structure definition was accidentally omitted from WinNT.h." struct PROCESSOR_POWER_INFORMATION { @@ -26,7 +29,10 @@ struct PROCESSOR_POWER_INFORMATION { ULONG CurrentIdleState; }; -// include_next -#include <../um/winnt.h> +// 10.0.14393.0 SDK + +#ifndef PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64 +#define PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64 13 +#endif #endif // CRASHPAD_COMPAT_WIN_WINNT_H_ diff --git a/minidump/minidump_extensions.h b/minidump/minidump_extensions.h index 00ac6e88..245ef8b5 100644 --- a/minidump/minidump_extensions.h +++ b/minidump/minidump_extensions.h @@ -162,6 +162,16 @@ enum MinidumpCPUArchitecture : uint16_t { kMinidumpCPUArchitectureX86Win64 = PROCESSOR_ARCHITECTURE_IA32_ON_WIN64, kMinidumpCPUArchitectureNeutral = PROCESSOR_ARCHITECTURE_NEUTRAL, + + //! \brief 64-bit ARM. + //! + //! These systems identify their CPUs generically as “arm64” or “aarch64”, or + //! with more specific names such as “armv8”. + //! + //! \sa #kMinidumpCPUArchitectureARM64Breakpad + kMinidumpCPUArchitectureARM64 = PROCESSOR_ARCHITECTURE_ARM64, + + kMinidumpCPUArchitectureARM32Win64 = PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64, kMinidumpCPUArchitectureSPARC = 0x8001, //! \brief 64-bit PowerPC. @@ -170,11 +180,10 @@ enum MinidumpCPUArchitecture : uint16_t { //! specific names such as “ppc970”. kMinidumpCPUArchitecturePPC64 = 0x8002, - //! \brief 64-bit ARM. + //! \brief Used by Breakpad for 64-bit ARM. //! - //! These systems identify their CPUs generically as “arm64” or “aarch64”, or - //! with more specific names such as “armv8”. - kMinidumpCPUArchitectureARM64 = 0x8003, + //! \deprecated Use #kMinidumpCPUArchitectureARM64 instead. + kMinidumpCPUArchitectureARM64Breakpad = 0x8003, //! \brief Unknown CPU architecture. kMinidumpCPUArchitectureUnknown = PROCESSOR_ARCHITECTURE_UNKNOWN, From 5c44f1d14f1c6dfce2c2106cf6837e2dd44d61f7 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 9 Nov 2016 13:15:17 -0500 Subject: [PATCH 2/9] Indicate rdtscp availability in MINIDUMP_SYSTEM_INFO of x86_64 minidumps This exposes a bit for PF_RDTSCP_INSTRUCTION_AVAILABLE in CPU_INFORMATION::OtherCpuInfo::ProcessorFeatures. This bit was introduced in Windows 10. Change-Id: I464c308f8325d14c0839f609ea4260737a58f7a5 Reviewed-on: https://chromium-review.googlesource.com/409138 Reviewed-by: Scott Graham --- minidump/minidump_system_info_writer.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/minidump/minidump_system_info_writer.cc b/minidump/minidump_system_info_writer.cc index 066363cb..5b7ca8f4 100644 --- a/minidump/minidump_system_info_writer.cc +++ b/minidump/minidump_system_info_writer.cc @@ -64,10 +64,12 @@ uint64_t AMD64FeaturesFromSystemSnapshot( MAP_FEATURE(cpuid_features, F_RDRAND, PF_RDRAND_INSTRUCTION_AVAILABLE); #define FX_XD 20 +#define FX_RDTSCP 27 #define FX_3DNOW 31 uint64_t extended_features = system_snapshot->CPUX86ExtendedFeatures(); + MAP_FEATURE(extended_features, FX_RDTSCP, PF_RDTSCP_INSTRUCTION_AVAILABLE); MAP_FEATURE(extended_features, FX_3DNOW, PF_3DNOW_INSTRUCTIONS_AVAILABLE); #define F7_FSGSBASE 0 @@ -76,8 +78,8 @@ uint64_t AMD64FeaturesFromSystemSnapshot( MAP_FEATURE(leaf7_features, F7_FSGSBASE, PF_RDWRFSGSBASE_AVAILABLE); - // This feature bit should be set if NX (XD, DEP) is enabled, not just if - // it’s available on the CPU as indicated by the XF_XD bit. + // This feature bit should be set if NX (XD, DEP) is enabled, not just if it’s + // available on the CPU as indicated by the FX_XD bit. if (system_snapshot->NXEnabled()) { minidump_features |= ADD_FEATURE(PF_NX_ENABLED); } @@ -86,10 +88,10 @@ uint64_t AMD64FeaturesFromSystemSnapshot( minidump_features |= ADD_FEATURE(PF_SSE_DAZ_MODE_AVAILABLE); } - // PF_SECOND_LEVEL_ADDRESS_TRANSLATION can’t be determined without - // consulting model-specific registers, a privileged operation. The exact - // use of PF_VIRT_FIRMWARE_ENABLED is unknown. PF_FASTFAIL_AVAILABLE is - // irrelevant outside of Windows. + // PF_SECOND_LEVEL_ADDRESS_TRANSLATION can’t be determined without consulting + // model-specific registers, a privileged operation. The exact use of + // PF_VIRT_FIRMWARE_ENABLED is unknown. PF_FASTFAIL_AVAILABLE is irrelevant + // outside of Windows. #undef MAP_FEATURE #undef ADD_FEATURE From 3abde199a7129246010107541cd0efc6645c72b4 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 9 Nov 2016 15:55:55 -0500 Subject: [PATCH 3/9] minidump: fix tests to expect new rdtscp bit after 5c44f1d14f1c TEST=crashpad_minidump_test MinidumpSystemInfoWriter.InitializeFromSnapshot_AMD64 Change-Id: I2fdd2061626a9f906eab025eeb8191d680196109 Reviewed-on: https://chromium-review.googlesource.com/409612 Reviewed-by: Scott Graham --- minidump/minidump_system_info_writer_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/minidump/minidump_system_info_writer_test.cc b/minidump/minidump_system_info_writer_test.cc index 9976250a..2a869c6b 100644 --- a/minidump/minidump_system_info_writer_test.cc +++ b/minidump/minidump_system_info_writer_test.cc @@ -405,7 +405,8 @@ TEST(MinidumpSystemInfoWriter, InitializeFromSnapshot_AMD64) { (1 << PF_COMPARE_EXCHANGE128) | (1 << PF_XSAVE_ENABLED) | (1 << PF_RDWRFSGSBASE_AVAILABLE) | - (1 << PF_RDRAND_INSTRUCTION_AVAILABLE); + (1 << PF_RDRAND_INSTRUCTION_AVAILABLE) | + (UINT64_C(1) << PF_RDTSCP_INSTRUCTION_AVAILABLE); expect_system_info.Cpu.OtherCpuInfo.ProcessorFeatures[1] = 0; const char kOSVersionBuild[] = "13F34"; From 741c9cc51ea3bfed5fb4b3ce9ce3b498c4e76188 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 9 Nov 2016 16:52:40 -0500 Subject: [PATCH 4/9] mac: Deal with bootstrap_look_up() race encountered on 10.12.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bootstrap_look_up() “successfully” returns MACH_PORT_DEAD about half of the time on 10.12.1 16B2657 (xnu-3789.21.4). Replace that with MACH_PORT_NULL in the BootstrapLookUp() wrapper that all callers are already routed through. BUG=crashpad:139 TEST=crashpad_util_test MachExtensions.BootstrapCheckInAndLookUp Change-Id: I9a39b709add5ca7e64bb5b970ed6ba3fdfd1d47a Reviewed-on: https://chromium-review.googlesource.com/409671 Reviewed-by: Robert Sesek --- util/mach/mach_extensions.cc | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/util/mach/mach_extensions.cc b/util/mach/mach_extensions.cc index 64552f1c..eb4e330e 100644 --- a/util/mach/mach_extensions.cc +++ b/util/mach/mach_extensions.cc @@ -157,7 +157,26 @@ base::mac::ScopedMachReceiveRight BootstrapCheckIn( base::mac::ScopedMachSendRight BootstrapLookUp( const std::string& service_name) { - return BootstrapCheckInOrLookUp(service_name); + base::mac::ScopedMachSendRight send( + BootstrapCheckInOrLookUp(service_name)); + + // It’s possible to race the bootstrap server when the receive right + // corresponding to the looked-up send right is destroyed immediately before + // the bootstrap_look_up() call. If the bootstrap server believes that + // |service_name| is still registered before processing the port-destroyed + // notification sent to it by the kernel, it will respond to a + // bootstrap_look_up() request with a send right that has become a dead name, + // which will be returned to the bootstrap_look_up() caller, translated into + // the caller’s IPC port name space, as the special MACH_PORT_DEAD port name. + // Check for that and return MACH_PORT_NULL in its place, as though the + // bootstrap server had fully processed the port-destroyed notification before + // responding to bootstrap_look_up(). + if (send.get() == MACH_PORT_DEAD) { + LOG(ERROR) << "bootstrap_look_up " << service_name << ": service is dead"; + send.reset(); + } + + return send; } base::mac::ScopedMachSendRight SystemCrashReporterHandler() { From 1382618fbe009d489c811944eab33e545915b36f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 9 Nov 2016 20:42:00 -0500 Subject: [PATCH 5/9] Provide backup #defines for things introduced in SDK 10.0.10240.0 This follows discussion on https://chromium-review.googlesource.com/409098/. Change-Id: Ic82b64a14bb89cfdf1c88b1482cc2c2c9c0c2589 Reviewed-on: https://chromium-review.googlesource.com/409604 Reviewed-by: Scott Graham --- compat/win/winnt.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/compat/win/winnt.h b/compat/win/winnt.h index 674b70e9..6a5e0164 100644 --- a/compat/win/winnt.h +++ b/compat/win/winnt.h @@ -29,6 +29,28 @@ struct PROCESSOR_POWER_INFORMATION { ULONG CurrentIdleState; }; +// 10.0.10240.0 SDK + +#ifndef PROCESSOR_ARCHITECTURE_ARM64 +#define PROCESSOR_ARCHITECTURE_ARM64 12 +#endif + +#ifndef PF_ARM_V8_INSTRUCTIONS_AVAILABLE +#define PF_ARM_V8_INSTRUCTIONS_AVAILABLE 29 +#endif + +#ifndef PF_ARM_V8_CRYPTO_INSTRUCTIONS_AVAILABLE +#define PF_ARM_V8_CRYPTO_INSTRUCTIONS_AVAILABLE 30 +#endif + +#ifndef PF_ARM_V8_CRC32_INSTRUCTIONS_AVAILABLE +#define PF_ARM_V8_CRC32_INSTRUCTIONS_AVAILABLE 31 +#endif + +#ifndef PF_RDTSCP_INSTRUCTION_AVAILABLE +#define PF_RDTSCP_INSTRUCTION_AVAILABLE 32 +#endif + // 10.0.14393.0 SDK #ifndef PROCESSOR_ARCHITECTURE_ARM32_ON_WIN64 From dd85381a32ddc4b1acee269d2c86927317e82f5d Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 11 Nov 2016 12:35:16 -0500 Subject: [PATCH 6/9] GCC fix: Disable -Wmultichar warning throughout Crashpad -Wmultichar is enabled by default with GCC (but not clang). It is impossible to disable this warning with #pragma GCC diagnostic ignored. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431 While compiling, for example, minidump_file_writer.cc: In file included from ../../minidump/minidump_extensions.h:25:0, from ../../minidump/minidump_file_writer.h:27, from ../../minidump/minidump_file_writer.cc:15: ../../util/misc/pdb_structures.h:45:38: error: multi-character character constan t [-Werror=multichar] static const uint32_t kSignature = '01BN'; ^~~~~~ ../../util/misc/pdb_structures.h:106:38: error: multi-character character consta nt [-Werror=multichar] static const uint32_t kSignature = 'SDSR'; ^~~~~~ ../../minidump/minidump_file_writer.cc:190:23: error: multi-character character constant [-Werror=multichar] header_.Signature = MINIDUMP_SIGNATURE; ^~~~~~~~~~~~~~~~~~ doc/developing.md is also updated to provide GCC build instructions for Android. Tested with: - GCC 4.9 from NDK r13 targeting arm with SDK 16 - GCC 4.9 from NDK r13 targeting arm64 with SDK 21 - GCC 6.2 targeting x86_64 BUG=crashpad:30 Change-Id: I9e7993761f5461281c9f4d8b4c56e8407e2c5b47 Reviewed-on: https://chromium-review.googlesource.com/409776 Reviewed-by: Robert Sesek --- build/crashpad.gypi | 11 +++++++++++ doc/developing.md | 7 ++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/build/crashpad.gypi b/build/crashpad.gypi index 027c7b68..166e768f 100644 --- a/build/crashpad.gypi +++ b/build/crashpad.gypi @@ -25,5 +25,16 @@ 4201, # nonstandard extension used : nameless struct/union. 4324, # structure was padded due to __declspec(align()). ], + 'conditions': [ + ['OS=="linux" or OS=="android"', { + 'conditions': [ + ['clang==0', { + 'cflags': [ + '-Wno-multichar', + ], + }], + ], + }], + ], }, } diff --git a/doc/developing.md b/doc/developing.md index 229e7b00..9f881678 100644 --- a/doc/developing.md +++ b/doc/developing.md @@ -154,7 +154,12 @@ $ CC_target=~/android-ndk-r13_arm64_api21/bin/clang \ --generator-output=out_android_arm64_api21 -f ninja-android ``` -Target “triplets” to use for `ar`, `nm`, and `readelf` are: +It is also possible to use GCC instead of Clang by making the appropriate +substitutions: `aarch64-linux-android-gcc` for `CC_target`; +`aarch64-linux-android-g++` for `CXX_target`; and `-Dclang=0` as an argument to +`gyp_crashpad.py`. + +Target “triplets” to use for `ar`, `nm`, `readelf`, `gcc`, and `g++` are: | Architecture | Target “triplet” | |:-------------|:------------------------| From 5b14b419925ff2149c2db57a79b80d23136b1d2a Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 10 Nov 2016 13:23:05 -0500 Subject: [PATCH 7/9] =?UTF-8?q?GCC=20fix:=20Don=E2=80=99t=20name=20a=20met?= =?UTF-8?q?hod=20exactly=20the=20same=20as=20its=20return=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While compiling, for example, minidump_simple_string_dictionary_writer.cc: In file included from ../../minidump/minidump_module_crashpad_info_writer.cc:21:0: ../../minidump/minidump_simple_string_dictionary_writer.h:55:45: error: declaration of ‘const crashpad::MinidumpSimpleStringDictionaryEntry* crashpad::MinidumpSimpleStringDictionaryEntryWriter::MinidumpSimpleStringDictionaryEntry() const’ [-fpermissive] MinidumpSimpleStringDictionaryEntry() const; ^~~~~ In file included from ../../minidump/minidump_module_crashpad_info_writer.h:25:0, from ../../minidump/minidump_module_crashpad_info_writer.cc:15: ../../minidump/minidump_extensions.h:255:26: error: changes meaning of ‘MinidumpSimpleStringDictionaryEntry’ from ‘struct crashpad::MinidumpSimpleStringDictionaryEntry’ [-fpermissive] struct ALIGNAS(4) PACKED MinidumpSimpleStringDictionaryEntry { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Tested with: - GCC 4.9 from NDK r13 targeting arm with SDK 16 - GCC 4.9 from NDK r13 targeting arm64 with SDK 21 - GCC 6.2 targeting x86_64 BUG=crashpad:30 Change-Id: I1e5e6a21a24f19eef7602e4123459ce15f3b089e Reviewed-on: https://chromium-review.googlesource.com/409624 Reviewed-by: Robert Sesek --- minidump/minidump_simple_string_dictionary_writer.cc | 6 +++--- minidump/minidump_simple_string_dictionary_writer.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/minidump/minidump_simple_string_dictionary_writer.cc b/minidump/minidump_simple_string_dictionary_writer.cc index 7a40cd4b..b4dd722e 100644 --- a/minidump/minidump_simple_string_dictionary_writer.cc +++ b/minidump/minidump_simple_string_dictionary_writer.cc @@ -34,8 +34,8 @@ MinidumpSimpleStringDictionaryEntryWriter:: } const MinidumpSimpleStringDictionaryEntry* -MinidumpSimpleStringDictionaryEntryWriter::MinidumpSimpleStringDictionaryEntry() - const { +MinidumpSimpleStringDictionaryEntryWriter:: + GetMinidumpSimpleStringDictionaryEntry() const { DCHECK_EQ(state(), kStateWritable); return &entry_; @@ -179,7 +179,7 @@ bool MinidumpSimpleStringDictionaryWriter::WriteObject( std::vector iovecs(1, iov); for (const auto& key_entry : entries_) { - iov.iov_base = key_entry.second->MinidumpSimpleStringDictionaryEntry(); + iov.iov_base = key_entry.second->GetMinidumpSimpleStringDictionaryEntry(); iov.iov_len = sizeof(MinidumpSimpleStringDictionaryEntry); iovecs.push_back(iov); } diff --git a/minidump/minidump_simple_string_dictionary_writer.h b/minidump/minidump_simple_string_dictionary_writer.h index 4e23717f..f2662bcc 100644 --- a/minidump/minidump_simple_string_dictionary_writer.h +++ b/minidump/minidump_simple_string_dictionary_writer.h @@ -52,7 +52,7 @@ class MinidumpSimpleStringDictionaryEntryWriter final //! //! \note Valid in #kStateWritable. const MinidumpSimpleStringDictionaryEntry* - MinidumpSimpleStringDictionaryEntry() const; + GetMinidumpSimpleStringDictionaryEntry() const; //! \brief Sets the strings to be written as the entry object’s key and value. //! From 57b2210ed75d5271aadab3ea1f4d4ec11e70da18 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 10 Nov 2016 15:11:20 -0500 Subject: [PATCH 8/9] GCC fix: Make UUID POD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This eliminates all constructors, but nearly all points of use were using the default constructor to initialize a UUID member variable as in uuid_(). This syntax will still produce a zeroed-out UUID. While compiling, for example, minidump_rva_list_writer.cc: In file included from ../../minidump/minidump_rva_list_writer.h:25:0, from ../../minidump/minidump_rva_list_writer.cc:15: ../../minidump/minidump_extensions.h:412:8: error: ignoring packed attribute because of unpacked non-POD field ‘crashpad::UUID crashpad::MinidumpCrashpadInfo::report_id’ [-Werror] UUID report_id; ^~~~~~~~~ ../../minidump/minidump_extensions.h:424:8: error: ignoring packed attribute because of unpacked non-POD field ‘crashpad::UUID crashpad::MinidumpCrashpadInfo::client_id’ [-Werror] UUID client_id; ^~~~~~~~~ Tested with: - GCC 4.9 from NDK r13 targeting arm with SDK 16 - GCC 4.9 from NDK r13 targeting arm64 with SDK 21 - GCC 6.2 targeting x86_64 BUG=crashpad:30 Change-Id: Iec6b1557441b69d75246f2f75c59c4158fb7ca29 Reviewed-on: https://chromium-review.googlesource.com/409641 Reviewed-by: Scott Graham --- snapshot/win/process_snapshot_win_test.cc | 3 ++- util/misc/uuid.cc | 22 +++++++--------------- util/misc/uuid.h | 21 ++++----------------- util/misc/uuid_test.cc | 15 +++++++++++---- util/win/process_info_test.cc | 3 ++- 5 files changed, 26 insertions(+), 38 deletions(-) diff --git a/snapshot/win/process_snapshot_win_test.cc b/snapshot/win/process_snapshot_win_test.cc index 76b5905b..a3e31a4e 100644 --- a/snapshot/win/process_snapshot_win_test.cc +++ b/snapshot/win/process_snapshot_win_test.cc @@ -31,7 +31,8 @@ namespace test { namespace { void TestImageReaderChild(const base::string16& directory_modification) { - UUID done_uuid(UUID::InitializeWithNewTag{}); + UUID done_uuid; + done_uuid.InitializeWithNew(); ScopedKernelHANDLE done( CreateEvent(nullptr, true, false, done_uuid.ToString16().c_str())); ASSERT_TRUE(done.get()); diff --git a/util/misc/uuid.cc b/util/misc/uuid.cc index e4b4b31a..4f1fbcbe 100644 --- a/util/misc/uuid.cc +++ b/util/misc/uuid.cc @@ -42,23 +42,15 @@ namespace crashpad { static_assert(sizeof(UUID) == 16, "UUID must be 16 bytes"); #if CXX_LIBRARY_VERSION >= 2011 -static_assert(std::is_standard_layout::value, - "UUID must be standard layout"); +static_assert(std::is_pod::value, "UUID must be POD"); #endif -UUID::UUID() : data_1(0), data_2(0), data_3(0), data_4(), data_5() { -} - -UUID::UUID(InitializeWithNewTag) { - CHECK(InitializeWithNew()); -} - -UUID::UUID(const uint8_t* bytes) { - InitializeFromBytes(bytes); -} - bool UUID::operator==(const UUID& that) const { - return memcmp(this, &that, sizeof(UUID)) == 0; + return memcmp(this, &that, sizeof(*this)) == 0; +} + +void UUID::InitializeToZero() { + memset(this, 0, sizeof(*this)); } void UUID::InitializeFromBytes(const uint8_t* bytes) { @@ -132,7 +124,7 @@ void UUID::InitializeFromSystemUUID(const ::UUID* system_uuid) { "unexpected system uuid size"); static_assert(offsetof(::UUID, Data1) == offsetof(UUID, data_1), "unexpected system uuid layout"); - memcpy(this, system_uuid, sizeof(::UUID)); + memcpy(this, system_uuid, sizeof(*this)); } #endif // OS_WIN diff --git a/util/misc/uuid.h b/util/misc/uuid.h index f25aa652..4e5884e2 100644 --- a/util/misc/uuid.h +++ b/util/misc/uuid.h @@ -36,27 +36,14 @@ namespace crashpad { //! //! A %UUID is a unique 128-bit number specified by RFC 4122. //! -//! This is a standard-layout structure. +//! This is a POD structure. struct UUID { - //! \brief Initializes the %UUID to zero. - UUID(); - - //! \brief Tag to pass to constructor to indicate it should initialize with - //! generated data. - struct InitializeWithNewTag {}; - - //! \brief Initializes the %UUID using a standard system facility to generate - //! the value. - //! - //! CHECKs on failure with a message logged. - explicit UUID(InitializeWithNewTag); - - //! \copydoc InitializeFromBytes() - explicit UUID(const uint8_t* bytes); - bool operator==(const UUID& that) const; bool operator!=(const UUID& that) const { return !operator==(that); } + //! \brief Initializes the %UUID to zero. + void InitializeToZero(); + //! \brief Initializes the %UUID from a sequence of bytes. //! //! \a bytes is taken as a %UUID laid out in big-endian format in memory. On diff --git a/util/misc/uuid_test.cc b/util/misc/uuid_test.cc index 673cedc8..f064c653 100644 --- a/util/misc/uuid_test.cc +++ b/util/misc/uuid_test.cc @@ -31,6 +31,7 @@ namespace { TEST(UUID, UUID) { UUID uuid_zero; + uuid_zero.InitializeToZero(); EXPECT_EQ(0u, uuid_zero.data_1); EXPECT_EQ(0u, uuid_zero.data_2); EXPECT_EQ(0u, uuid_zero.data_3); @@ -60,7 +61,8 @@ TEST(UUID, UUID) { 0x0d, 0x0e, 0x0f}; - UUID uuid(kBytes); + UUID uuid; + uuid.InitializeFromBytes(kBytes); EXPECT_EQ(0x00010203u, uuid.data_1); EXPECT_EQ(0x0405u, uuid.data_2); EXPECT_EQ(0x0607u, uuid.data_3); @@ -78,7 +80,8 @@ TEST(UUID, UUID) { EXPECT_FALSE(uuid == uuid_zero); EXPECT_NE(uuid, uuid_zero); - UUID uuid_2(kBytes); + UUID uuid_2; + uuid_2.InitializeFromBytes(kBytes); EXPECT_EQ(uuid, uuid_2); EXPECT_FALSE(uuid != uuid_2); @@ -155,7 +158,8 @@ TEST(UUID, UUID) { EXPECT_EQ(0x45u, uuid.data_5[5]); EXPECT_EQ("45454545-4545-4545-4545-454545454545", uuid.ToString()); - UUID initialized_generated(UUID::InitializeWithNewTag{}); + UUID initialized_generated; + initialized_generated.InitializeWithNew(); EXPECT_NE(initialized_generated, uuid_zero); } @@ -182,7 +186,9 @@ TEST(UUID, FromString) { {"6d247a34-53d5-40ec-a90d-d8dea9e94cc01", false} }; - const std::string empty_uuid = UUID().ToString(); + UUID uuid_zero; + uuid_zero.InitializeToZero(); + const std::string empty_uuid = uuid_zero.ToString(); for (size_t index = 0; index < arraysize(kCases); ++index) { const TestCase& test_case = kCases[index]; @@ -190,6 +196,7 @@ TEST(UUID, FromString) { "index %" PRIuS ": %s", index, test_case.uuid_string)); UUID uuid; + uuid.InitializeToZero(); EXPECT_EQ(test_case.success, uuid.InitializeFromString(test_case.uuid_string)); if (test_case.success) { diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index be2cfed0..e377a6ee 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -132,7 +132,8 @@ TEST(ProcessInfo, Self) { void TestOtherProcess(const base::string16& directory_modification) { ProcessInfo process_info; - UUID done_uuid(UUID::InitializeWithNewTag{}); + UUID done_uuid; + done_uuid.InitializeWithNew(); ScopedKernelHANDLE done( CreateEvent(nullptr, true, false, done_uuid.ToString16().c_str())); From f83530bf9a0bae0eb4c01192056576284646757f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 10 Nov 2016 14:39:51 -0500 Subject: [PATCH 9/9] =?UTF-8?q?GCC=20fix:=20Don=E2=80=99t=20use=20arraysiz?= =?UTF-8?q?e()=20on=20packed=20structs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While compiling, for example, minidump_exception_writer.cc: In file included from ../../minidump/minidump_exception_writer.h:26:0, from ../../minidump/minidump_exception_writer.cc:15: ../../minidump/minidump_exception_writer.cc: In member function ‘void crashpad::MinidumpExceptionWriter::SetExceptionInformation(const std::vector&)’: ../../minidump/minidump_exception_writer.cc:67:44: error: cannot bind packed field ‘((crashpad::MinidumpExceptionWriter*)this)->crashpad::MinidumpExceptionWriter::exception_.MINIDUMP_EXCEPTION_STREAM::ExceptionRecord.MINIDUMP_EXCEPTION::ExceptionInformation’ to ‘long unsigned int (&)[15]’ arraysize(exception_.ExceptionRecord.ExceptionInformation); ~~~~~~~~~~~~~~~~~~~~~~~~~~~^ ../../third_party/mini_chromium/mini_chromium/base/macros.h:41:50: note: in definition of macro ‘arraysize’ #define arraysize(array) (sizeof(ArraySizeHelper(array))) Tested with: - GCC 4.9 from NDK r13 targeting arm with SDK 16 - GCC 4.9 from NDK r13 targeting arm64 with SDK 21 - GCC 6.2 targeting x86_64 BUG=crashpad:30 Change-Id: I63963b277a309b4715148215f51902c33ba13b5a Reviewed-on: https://chromium-review.googlesource.com/409694 Reviewed-by: Scott Graham --- minidump/minidump_exception_writer.cc | 3 +- minidump/minidump_misc_info_writer.cc | 11 ++-- minidump/minidump_system_info_writer.cc | 8 ++- util/misc/arraysize_unsafe.h | 27 +++++++++ util/misc/arraysize_unsafe_test.cc | 73 +++++++++++++++++++++++++ util/util.gyp | 1 + util/util_test.gyp | 1 + 7 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 util/misc/arraysize_unsafe.h create mode 100644 util/misc/arraysize_unsafe_test.cc diff --git a/minidump/minidump_exception_writer.cc b/minidump/minidump_exception_writer.cc index 5ec8d374..d2466a6b 100644 --- a/minidump/minidump_exception_writer.cc +++ b/minidump/minidump_exception_writer.cc @@ -21,6 +21,7 @@ #include "minidump/minidump_context_writer.h" #include "snapshot/exception_snapshot.h" #include "util/file/file_writer.h" +#include "util/misc/arraysize_unsafe.h" namespace crashpad { @@ -64,7 +65,7 @@ void MinidumpExceptionWriter::SetExceptionInformation( const size_t parameters = exception_information.size(); const size_t kMaxParameters = - arraysize(exception_.ExceptionRecord.ExceptionInformation); + ARRAYSIZE_UNSAFE(exception_.ExceptionRecord.ExceptionInformation); CHECK_LE(parameters, kMaxParameters); exception_.ExceptionRecord.NumberParameters = diff --git a/minidump/minidump_misc_info_writer.cc b/minidump/minidump_misc_info_writer.cc index 5f88c50c..ff4fdda8 100644 --- a/minidump/minidump_misc_info_writer.cc +++ b/minidump/minidump_misc_info_writer.cc @@ -26,6 +26,7 @@ #include "snapshot/process_snapshot.h" #include "snapshot/system_snapshot.h" #include "util/file/file_writer.h" +#include "util/misc/arraysize_unsafe.h" #include "util/numeric/in_range_cast.h" #include "util/numeric/safe_assignment.h" @@ -295,7 +296,7 @@ void MinidumpMiscInfoWriter::SetTimeZone(uint32_t time_zone_id, internal::MinidumpWriterUtil::AssignUTF8ToUTF16( misc_info_.TimeZone.StandardName, - arraysize(misc_info_.TimeZone.StandardName), + ARRAYSIZE_UNSAFE(misc_info_.TimeZone.StandardName), standard_name); misc_info_.TimeZone.StandardDate = standard_date; @@ -303,7 +304,7 @@ void MinidumpMiscInfoWriter::SetTimeZone(uint32_t time_zone_id, internal::MinidumpWriterUtil::AssignUTF8ToUTF16( misc_info_.TimeZone.DaylightName, - arraysize(misc_info_.TimeZone.DaylightName), + ARRAYSIZE_UNSAFE(misc_info_.TimeZone.DaylightName), daylight_name); misc_info_.TimeZone.DaylightDate = daylight_date; @@ -320,10 +321,12 @@ void MinidumpMiscInfoWriter::SetBuildString( misc_info_.Flags1 |= MINIDUMP_MISC4_BUILDSTRING; internal::MinidumpWriterUtil::AssignUTF8ToUTF16( - misc_info_.BuildString, arraysize(misc_info_.BuildString), build_string); + misc_info_.BuildString, + ARRAYSIZE_UNSAFE(misc_info_.BuildString), + build_string); internal::MinidumpWriterUtil::AssignUTF8ToUTF16( misc_info_.DbgBldStr, - arraysize(misc_info_.DbgBldStr), + ARRAYSIZE_UNSAFE(misc_info_.DbgBldStr), debug_build_string); } diff --git a/minidump/minidump_system_info_writer.cc b/minidump/minidump_system_info_writer.cc index 5b7ca8f4..9c665f4f 100644 --- a/minidump/minidump_system_info_writer.cc +++ b/minidump/minidump_system_info_writer.cc @@ -20,6 +20,7 @@ #include "minidump/minidump_string_writer.h" #include "snapshot/system_snapshot.h" #include "util/file/file_writer.h" +#include "util/misc/arraysize_unsafe.h" #include "util/misc/implicit_cast.h" namespace crashpad { @@ -196,7 +197,7 @@ void MinidumpSystemInfoWriter::SetCPUX86Vendor(uint32_t ebx, system_info_.ProcessorArchitecture == kMinidumpCPUArchitectureX86Win64); - static_assert(arraysize(system_info_.Cpu.X86CpuInfo.VendorId) == 3, + static_assert(ARRAYSIZE_UNSAFE(system_info_.Cpu.X86CpuInfo.VendorId) == 3, "VendorId must have 3 elements"); system_info_.Cpu.X86CpuInfo.VendorId[0] = ebx; @@ -254,8 +255,9 @@ void MinidumpSystemInfoWriter::SetCPUOtherFeatures(uint64_t features_0, system_info_.ProcessorArchitecture != kMinidumpCPUArchitectureX86Win64); - static_assert(arraysize(system_info_.Cpu.OtherCpuInfo.ProcessorFeatures) == 2, - "ProcessorFeatures must have 2 elements"); + static_assert( + ARRAYSIZE_UNSAFE(system_info_.Cpu.OtherCpuInfo.ProcessorFeatures) == 2, + "ProcessorFeatures must have 2 elements"); system_info_.Cpu.OtherCpuInfo.ProcessorFeatures[0] = features_0; system_info_.Cpu.OtherCpuInfo.ProcessorFeatures[1] = features_1; diff --git a/util/misc/arraysize_unsafe.h b/util/misc/arraysize_unsafe.h new file mode 100644 index 00000000..e53c70b8 --- /dev/null +++ b/util/misc/arraysize_unsafe.h @@ -0,0 +1,27 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_MISC_ARRAYSIZE_UNSAFE_H_ +#define CRASHPAD_UTIL_MISC_ARRAYSIZE_UNSAFE_H_ + +//! \file + +//! \brief Not the safest way of computing an array’s size… +//! +//! `#%include "base/macros.h"` and use its `arraysize()` instead. This macro +//! should only be used in rare situations where `arraysize()` does not +//! function. +#define ARRAYSIZE_UNSAFE(array) (sizeof(array) / sizeof(array[0])) + +#endif // CRASHPAD_UTIL_MISC_ARRAYSIZE_UNSAFE_H_ diff --git a/util/misc/arraysize_unsafe_test.cc b/util/misc/arraysize_unsafe_test.cc new file mode 100644 index 00000000..1c638e02 --- /dev/null +++ b/util/misc/arraysize_unsafe_test.cc @@ -0,0 +1,73 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/misc/arraysize_unsafe.h" + +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(ArraySizeUnsafe, ArraySizeUnsafe) { + char c0[0]; + static_assert(ARRAYSIZE_UNSAFE(c0) == 0, "c0"); + + char c1[1]; + static_assert(ARRAYSIZE_UNSAFE(c1) == 1, "c1"); + + char c2[2]; + static_assert(ARRAYSIZE_UNSAFE(c2) == 2, "c2"); + + char c4[4]; + static_assert(ARRAYSIZE_UNSAFE(c4) == 4, "c4"); + + int i0[0]; + static_assert(ARRAYSIZE_UNSAFE(i0) == 0, "i0"); + + int i1[1]; + static_assert(ARRAYSIZE_UNSAFE(i1) == 1, "i1"); + + int i2[2]; + static_assert(ARRAYSIZE_UNSAFE(i2) == 2, "i2"); + + int i4[4]; + static_assert(ARRAYSIZE_UNSAFE(i4) == 4, "i4"); + + long l8[8]; + static_assert(ARRAYSIZE_UNSAFE(l8) == 8, "l8"); + + int l9[9]; + static_assert(ARRAYSIZE_UNSAFE(l9) == 9, "l9"); + + struct S { + char c; + int i; + long l; + bool b; + }; + + S s0[0]; + static_assert(ARRAYSIZE_UNSAFE(s0) == 0, "s0"); + + S s1[1]; + static_assert(ARRAYSIZE_UNSAFE(s1) == 1, "s1"); + + S s10[10]; + static_assert(ARRAYSIZE_UNSAFE(s10) == 10, "s10"); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index 8269bd42..5768788a 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -84,6 +84,7 @@ 'mach/task_for_pid.h', 'mach/task_memory.cc', 'mach/task_memory.h', + 'misc/arraysize_unsafe.h', 'misc/clock.h', 'misc/clock_mac.cc', 'misc/clock_posix.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 9efe162a..b5989de9 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -55,6 +55,7 @@ 'mach/scoped_task_suspend_test.cc', 'mach/symbolic_constants_mach_test.cc', 'mach/task_memory_test.cc', + 'misc/arraysize_unsafe_test.cc', 'misc/clock_test.cc', 'misc/initialization_state_dcheck_test.cc', 'misc/initialization_state_test.cc',