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

Merge breakpad upstream #2

Merged
merged 196 commits into from
May 23, 2023
Merged

Conversation

MarijnS95
Copy link

Probably more good reasons than just this, but my Arch install upgraded to GCC 13 yesterday and that triggers:

  cargo:warning=In file included from breakpad/src/client/linux/handler/minidump_descriptor.cc:32:
  cargo:warning=breakpad/src/client/linux/handler/minidump_descriptor.h:115:3: error: ‘uintptr_tdoes not name a type
  cargo:warning=  115 |   uintptr_t address_within_principal_mapping() const {
  cargo:warning=      |   ^~~~~~~~~
  cargo:warning=breakpad/src/client/linux/handler/minidump_descriptor.h:40:1: note: ‘uintptr_tis defined in header<cstdint>’; did you forget to#include <cstdint>’?
  cargo:warning=   39 | #include "common/using_std_string.h"
  cargo:warning=  +++ |+#include <cstdint>
  cargo:warning=   40 |

Upstream already has a solution at getsentry@7ea7ded, so we just merge in main completely.

I'm not sure if we should also go through the sentry fork, or do a clean rebase. Alternatively we can also pick just the patch that fixes the build?

pkasting and others added 30 commits July 29, 2021 15:56
Bug: chromium:1066980
Change-Id: Ie95754402ce30bbd4bfcfc0c0150f07d2e3008f6
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3055796
Reviewed-by: Nelson Billing <nbilling@google.com>
The size of symbol file for chrome binary increased from 577 MB to
1205 MB. There are 7,453,748 INLINE records and 1,268,493 INLINE_ORIGIN
records.

Bug: 1190878
Change-Id: I802ec1b4574c14f74ff80d0f69daf3c81085778a
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/2915828
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
The header is not present in earlier versions of split dwarf.

Change-Id: I8fde233268230cea157b2b3276f3cf05190962f2
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3083253
Reviewed-by: Sterling Augustine <saugustine@google.com>
Change-Id: I35d7a5e50537bd6f20bcb5a91d386ffee9325b18
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3098093
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
This is a follow-up to 3c70e01 to make -d work.

Bug: chromium:1190878,chromium:1238693
Change-Id: Ie0c6c663c98491462fca1aa992503037f19cefa9
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3103526
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Building fails for some people because configure requires c++11 but make_unique is a c++14 feature.

Change-Id: I23ce689fc92e9e90a95e7643ff29602f6b32ccbb
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3107784
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Dwarf generated by Clang -g1 will not have DW_AT_inline attribute for some DW_TAG_subprograms even if they are inlined. This warning recently increased a lot (~ 3 million) due to DW_TAG_inlined_subroutine also complains about unknown abstract origin. It caused infra failure in building bots.

Bug: 1241579
Change-Id: I9b5135925b71aa915760c140bcf73fc603bb77d3
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3111782
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Use range-based for-loops where appropriate.

Change-Id: I2fffd270d434c90850e8151ee40e5adf0736ce55
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3120666
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
This allows INLINE_ORIGIN records appears in after FUNC records.

Change-Id: I69b8b5948ed91453e15c7f4c3888dfbe38e7bc5c
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3132381
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Break statements immediately following returns are unreachable.

Bug: chromium:1246232
Change-Id: I0892a66617f7b27b5e317a7d9741f5fcd19249f2
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3140192
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Temporarily works around an issue on Mac where the system version of
NXGetLocalArchInfo is returning x86 information on x86_64 devices,
which results in dump_syms failing on said devices. Instead, the
Breakpad implementation of NXGetLocalArchInfo, which is meant for
dump_syms_mac on Linux, will be used until the system version is fixed.

Bug: 1242776
Change-Id: Id398338e580eb9c67c61f9f01670d2e7dbe86bea
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3143524
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
The app will check if process_architecture is ARM64_OLD which is 0x8003
but newman is a new arch which is ARM64 (0x12)
We can fix the issue by checking both values

Test: "/google/src/cloud/zyanwu/latest/google3/blaze-bin/chrome/dongle/platform/tools/minidump --crash_report_id=49ed111b84c0736e --crash_server=crash --build_number=265669 --build_branch=1.56 --product=newman-user --eureka_root=/usr/local/google/home/zyanwu/eureka --symbol_cache_dir=/usr/local/google/home/zyanwu/android/debug/symbols --debug" can work and it can convert the minidump to core dump then load gdb.
Bug: 199144156
Change-Id: I1590a5b617e55ae8347aad426ba5b636ff6dcdfb
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3146740
Reviewed-by: Sterling Augustine <saugustine@google.com>
Reviewed-by: Nelson Billing <nbilling@google.com>
This change makes sure dump_syms process DW_TAG_inlined_subroutine only when -d flag is given, which save memory and time when -d is not given. Before this, it always processes DW_TAG_inlined_subroutine and -d determines whether or not to emit INLINE records.

Bug: chromium:1250351, chromium:1246974
Change-Id: I54725ba1e513cafe17268ca389ff8acc9c11b25e
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3166674
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Change-Id: I83a2d026f1cef1771d28b420d76de17f0cf296ec
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3166678
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
It moves InlineOriginMap to module.h. Let Module keeps the global InlineOriginMap to easily get all referenced InlineOrigin when emitting. And release allocated memory inside its destructor.

Verified that the symbol file with inline records for chrome is the same before and after this change.

Change-Id: I7541aa05d3d2df0b9d52d670cab58241baecf20d
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3171638
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Change-Id: I3904d52e946158439899f4c5aaa92d1d15160745
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3183519
Reviewed-by: Ivan Penkov <ivanpe@chromium.org>
- Added StringView which is used as a reference to a string, but
doesn't own the string.
- Removed the old string pool in DwarfCUToModule::FilePrivate, since
it's doing string copy.
- Added a string pool in Module to store functions/inline origins'
names (mangled and demangled).
- The peak memory usage drops from 20.6 GB to 12.5 GB when disabling
inline records and drops from 36 GB to 20.3 GB when enabling inline records.

Bug: chromium:1246974, chromium:1250351
Change-Id: Ie7e9740ea10c1930a0fc58c6becaae2d718b83b8
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3189410
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
The context arguments are of type DWORD_PTR which is actually a
integer type, not a pointer, so using NULL here causes a type
missmatch warning:

  error: passing NULL to non-pointer argument 8 [...]
Change-Id: Ia52f51fd0cd33af3b139f0427dec6c59c2455d0a
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3168663
Reviewed-by: Primiano Tucci <primiano@chromium.org>
After ff5892c added the new StringView,
building fails with GCC 6 due to it apparently failing to properly find
the type for nullptr_t resulting in the following error:

In file included from ../src/common/module.h:49:0,
                 from ../src/common/dwarf_cfi_to_module.h:49,
                 from ../src/common/linux/dump_symbols.cc:59:
../src/common/string_view.h:55:27: error: field 'nullptr_t' has incomplete type 'google_breakpad::StringView'
   StringView(nullptr_t) = delete;
                           ^~~~~~
../src/common/string_view.h:42:7: note: definition of 'class google_breakpad::StringView' is not complete until the closing brace
 class StringView {
       ^~~~~~~~~~

This can be fixed by adding the std:: namespace to nullptr_t.

Change-Id: I00a090d307ebe21d1143eac4a605ff319ce27048
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3201997
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
The probot app we were using has been shutdown, so switch over to
the new GH actions flow.

Change-Id: Ifa8c2835e1ac1a4df53a5c4f0aa851fbacbd4096
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3217681
Reviewed-by: Mark Mentovai <mark@chromium.org>
With Travis shutdown, convert our flows over to GH actions.

Change-Id: Ia4d358dbbf3d8a73c347f4b9e4cd4637ce44e594
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3216116
Reviewed-by: Mark Mentovai <mark@chromium.org>
Keeps us in sync with Chromium a bit better.

Change-Id: I4cb80f28fc3aa2e3d0cd8637dd2a5b1ff4ae633d
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3223799
Reviewed-by: Mark Mentovai <mark@chromium.org>
Change-Id: I4c6a6fb353cacb09710c579e59332d70d1e801a8
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3093129
Reviewed-by: Mark Mentovai <mark@chromium.org>
Change-Id: I468f19048f6b48b230913e911d0da7a20d96cae8
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3222826
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Nelson Billing <nbilling@google.com>
Bug: chromium:794619
Change-Id: I7edb70a915ffb3c6f945dce77b0bd913e32e85eb
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3229392
Reviewed-by: Mark Mentovai <mark@chromium.org>
Processor shows incorrect source file name if a frame have an inlined
frame and their source files are different.
Consider this example:
FILE 0 /tmp/a.h
FILE 1 /tmp/a.cpp
INLINE_ORIGIN 0 0 foo()
FUNC 1110 a 0 main
INLINE 0 22 0 1110 7
1110 7 3 0
1117 3 23 1

When querying the address 0x1110, we know this line 0x1110 corresponds
to /tmp/a.h line 3 and it's inside a inlined function foo() which is
defined at /tmp/a.h and called at line 22. But we don't know at which
file it's being called at line 22. So, we will get stacks like this:
void foo() /tmp/a.h:3
int main() /tmp/a.h:22

The correct stacks should be this:
void foo() /tmp/a.h:3
int main() /tmp/a.cpp:22

In this change:
1. Remove file_id field for INLINE_ORIGIN record.
2. Add call_site_file_id for INLINE record to represents the file where
this call being inlined.

After adding call_site_file_id to it (as third field), it looks like
this:
FILE 0 /tmp/a.h
FILE 1 /tmp/a.cpp
INLINE_ORIGIN 0 foo()
FUNC 1110 a 0 main
INLINE 0 22 1 0 1110 7
1110 7 3 0
1117 3 23 1

Bug: 1190878
Change-Id: Ibbb697d2f7e1b6ac3208cac6fae4353c8743198d
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3232838
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Introduces Arm's Pointer Authentication and Branch Target Identification
to breakpad.

The changes are similar to changes for PA/BTI to Marl, see
google/marl#204

Bug: 1145581
Change-Id: I6a770316ad333bfcfad2ce7f3c1ff78afb35c010
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3226471
Reviewed-by: Primiano Tucci <primiano@chromium.org>
This reverts commit 54d878a.

54d878a changed the dump_syms format incompatibly. This must be
redone in a multi-step process: the processor must be made to understand
the old and new formats simultaneously and the processor service must be
rebuilt and run with that update before dump_syms output can change to
use the new format.

Bug: chromium:1263390
Change-Id: I5b6f8aff8ea2916b2c07ac6a74b569fa27db51b9
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3244775
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
…ORIGIN

This is similar to the processor part of
https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3232838/,
but added compatibility to process both old and new format of
INLINE/INLINE_ORIGIN records in symbol file.

Old INLINE format:
INLINE <inline_nest_level> <call_site_line> <origin_id>
[<address> <size>]+
New INLINE format:
INLINE <inline_nest_level> <call_site_line> <call_site_file_id>
<origin_id> [<address> <size>]+
Old INLINE_ORIGIN format:
INLINE_ORIGIN <origin_id> <file_id> <name>
New INLINE_ORIGIN format:
INLINE_ORIGIN <origin_id> <name>

Change-Id: I555d9747bfd44a1a95113b9946dcd509b7710876
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3248433
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
speednoisemovement and others added 11 commits April 27, 2023 14:56
These are reimported from Apple's Github source drops, see exact
provenance in README. Most were imported as is, some were edited
to match previous versions, and as noted below

- Added arm headers where needed
- Removed (now) unused `/mach/i386/vm_param.h`
- Removed availability annotations
- Removed `__kernel_ptr_semantics`
- Added `defined(__aarch64__)` to all arm64 define guards

Bug: chromium:1420654, google-breakpad:880, b/257505171
Change-Id: I17bd03fa871a8f1dc4285daafa3d7b26c2186e2b
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4482294
Reviewed-by: Mark Mentovai <mark@chromium.org>
 The NXArch* family is deprecated in macOS 13. This change:
 - Uses the replacements where available
 - Silences deprecation warnings otherwise
 - Removes the Linux cross-compile shims in favor of having completely
 separate implementations for Mac and non-Mac. The logic of the Linux
 versions uses the same prepopulated data as before, but they no longer
 use NXArchInfo.

clang diagnostic disables are necessary due to https://crbug.com/1406057

Bug: chromium:1420654, google-breakpad:880, b/257505171
Change-Id: Iad777915a5a058551cfb3a7d3cf681cce180dfea
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4437109
Reviewed-by: Mark Mentovai <mark@chromium.org>
Use MD_CONTEXT_AMD64_DEBUG_REGISTERS instead of
MD_CONTEXT_AMD64_DEBUG_REGISTERS in the definition of
MD_CONTEXT_AMD64_ALL. This previously happened to work because the two
flags happened to have the same values and every includer of
minidump_cpu_amd64.h also happened to previously include
minidump_cpu_x86.h.

Change-Id: If8b422d3623936f4a0b57a4cf6dac4f348daa024
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4480251
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Use the exception record's context for the crashed thread instead of
the thread's own context. For the crashed thread the thread's own
context is the state inside the exception handler. Using it would not
result in the expected stack trace from the time of the crash.

This change aligns the behavior of minidump-2-core with the behavior of
minidump_stackwalk.

Bug: google-breakpad:885
Change-Id: I5cd3e9d39807308491b64fcd335f5f85b1dcd084
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4473128
Reviewed-by: Joshua Peraza <jperaza@google.com>
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Bug: b/257505171
Change-Id: I210b6689683ff2cf561997584924fd9b568943cb
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4494631
Reviewed-by: Mark Mentovai <mark@chromium.org>
Bug: None
Change-Id: I0409a0c2ab8e60b1f84f72b50a1fd400b5a41cbd
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4500379
Reviewed-by: Mark Mentovai <mark@chromium.org>
Bug: chromium:1420654
Change-Id: Id0281089962147040b6332223bf4593bf4fc60cd
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4500259
Reviewed-by: Mark Mentovai <mark@chromium.org>
MDRawModuleCrashpadInfoList::modules is a flexible array of
MDRawModuleCrashpadInfoLink and not MDLocationDescriptor. Breakpad does
not currently use the MDRawModuleCrashpadInfoList type, but its
definition should be updated to reflect the correct type to avoid
confusion.

Change-Id: If97f490db8d41529b59a225a275a37116746c2b7
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4504150
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
MDRawCrashpadAnnotationList::objects is a flexible array of
MDRawCrashpadAnnotation and not MDLocationDescriptor. Breakpad does not
currently use the MDRawCrashpadAnnotationList type, but its definition
should be updated to reflect the correct type to avoid confusion.

Change-Id: I58b5b0e4f7f95bc003b103e2750e3759c3e31292
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4503630
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
dump_syms was using x0...x31 notation, while the rest of Breakpad was
using the ABI names. This mismatch was causing stackwalking to not fully
succeed.

Fixed: 1432426
Change-Id: I0713e76e65ff6dad492b51bc3607e94e25dc2c3a
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4505156
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Comment on lines +331 to +353
case MD_CONTEXT_RISCV:
{
const MDRawContextRISCV* context_riscv = context->GetContextRISCV();
// memory = GetActualStackMemory(context_riscv->__gregs[MD_CONTEXT_RISCV_REG_SP],
// memory, memory_list);
cpu_stackwalker = new StackwalkerRISCV(system_info,
context_riscv,
memory, modules,
frame_symbolizer);
break;
}

case MD_CONTEXT_RISCV64:
{
const MDRawContextRISCV64* context_riscv64 = context->GetContextRISCV64();
// memory = GetActualStackMemory(context_riscv64->__gregs[MD_CONTEXT_RISCV_REG_SP],
// memory, memory_list);
cpu_stackwalker = new StackwalkerRISCV64(system_info,
context_riscv64,
memory, modules,
frame_symbolizer);
break;
}
Copy link
Author

Choose a reason for hiding this comment

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

This is the only conflict (out of two) I could not resolve: haven't figured out where the stack-pointer register is.

getsentry#1

@MarijnS95
Copy link
Author

I'm not sure if we should also go through the sentry fork, or do a clean rebase

Sentry's handler (default) branch seems to be actively updated with Google's main, but misses some of the changes that were picked to Embark's fork, or so it seems.

@MarijnS95
Copy link
Author

@Jake-Shadle how do you want to proceed here and in EmbarkStudios/sentry-contrib-rust#24?

@Jake-Shadle Jake-Shadle merged commit 7a5aba7 into EmbarkStudios:main May 23, 2023
Jake-Shadle added a commit to EmbarkStudios/sentry-contrib-rust that referenced this pull request May 23, 2023
* breakpad-sys: Update `breakpad` submodule with merged upstream

EmbarkStudios/breakpad#2

* Update submodule

* Fix formatting

* Remove mac

* Fix mac compilation

* Fix mac compilation harder

---------

Co-authored-by: Jake Shadle <jake.shadle@embark-studios.com>
@MarijnS95 MarijnS95 deleted the merge-upstream branch May 23, 2023 08:49
@MarijnS95
Copy link
Author

MarijnS95 commented May 23, 2023

It's ever so slightly painful that the squash merge now does not track when we last integrated the upstream/main branch, resulting in all kinds of conflicts when we merge again :(

(And the "x commits behind upstream" text is also wrong - and how about Sentry's own fork of Google Breakpad? 😰)

@Jake-Shadle
Copy link
Member

Oops.

@MarijnS95
Copy link
Author

MarijnS95 commented May 23, 2023

Oops.

https://github.com/EmbarkStudios/breakpad/compare/main..MarijnS95:breakpad:merge-upstream contains the remaining changes of merging Google/breakpad:main into this PR branch before it was squash-merged into this repo, as well as merging in getsentry/breakpad:handler which doesn't diverge much from Embark's fork which was based on it initially, but does diverge quite a bit from Google's upstream. They do merge in Google's upstream quite often though, so we might also "just" rebase on top of their handler branch instead for better tracking?

I can help address that if you want :)

@Jake-Shadle
Copy link
Member

Sure.

@MarijnS95
Copy link
Author

@Jake-Shadle done, the branch is identical to this original branch with the latest getsentry/handler branch merged in (as shared 2 weeks ago):

https://github.com/MarijnS95/breakpad/compare/rebase-upstream..merge-upstream

@MarijnS95
Copy link
Author

@Jake-Shadle do you still intend to go the rebase-route?

@Jake-Shadle
Copy link
Member

Yes, need a PR

@MarijnS95
Copy link
Author

@Jake-Shadle sure, if you reset this repo to the upstream branch first, make sure you then rebase-merge instead of squash-merge it 😬

@MarijnS95 MarijnS95 mentioned this pull request Jun 26, 2023
@MarijnS95
Copy link
Author

Done in #3 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet