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

fix: dump correct breakpad symbols on macOS #19042

Merged
merged 11 commits into from Jul 5, 2019

Conversation

4 participants
@nornagon
Copy link
Contributor

commented Jun 28, 2019

Description of Change

The breakpad symbols we're currently generating on macOS only include exported symbols, which makes them approximately useless for symbolication. This mimics how Chromium generates breakpad symbols.

Checklist

Release Notes

Notes: Fixed macOS breakpad symbol files to include non-public symbols.

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

(Draft state because I haven't yet integrated this into CI / release. But I've tested the GN targets and they work 👍)

@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 29, 2019

@nornagon nornagon marked this pull request as ready for review Jul 1, 2019

@nornagon nornagon requested a review from electron/wg-releases as a code owner Jul 1, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

This is ready for review now. I've refactored how symbol generation works to avoid using the not-very-well-maintained generate_breakpad_symbols.py from Chromium, and to reduce the number of possible code paths. Now we just call dump_syms directly. Some logic was borrowed from generate_breakpad_symbols.py to generate the directory layout. And ninja now drives the invocation of dump_syms, allowing parallel execution.

@jkleinsc
Copy link
Contributor

left a comment

The title of the PR says it is for macOS, but you are refactoring all of our symbol generation right?

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Test release builds running here:
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/249751 for status.
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/249753 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/249755 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/249756 for status.
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/249752 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/249754 for status.
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.90
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.63

@@ -937,6 +937,76 @@ if (is_mac) {
"@executable_path/../Frameworks",
]
}

if (enable_dsyms) {

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jul 2, 2019

Member

This never appears to be set? Unless I'm missing a thing?

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 2, 2019

Author Contributor

it's set in Chromium's GN files.

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

The title of the PR says it is for macOS, but you are refactoring all of our symbol generation right?

It refactors all platforms, but only fixes macOS. Windows (and Linux I think? haven't checked) were already working

@deepak1556

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@nornagon Should we backport this ? Seems like this must go all the way back to 4 ?

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@deepak1556 probably, yeah.

@jkleinsc
Copy link
Contributor

left a comment

Looks like Windows is failing on:

loadDataForPdb and loadDataFromExe failed for -i
Open failed

See https://ci.appveyor.com/project/electron-bot/electron-ia32-release/builds/25699648
And

C:/Python27/python.exe ../../electron/build/dump_syms.py ./dump_syms.exe electron.exe breakpad_symbols gen/electron/electron_app_symbols.stamp
CoCreateInstance CLSID_DiaSource {E6756135-1E65-4D17-8576-610761398C3C} failed (msdia*.dll unregistered?)
Open failed
Traceback (most recent call last):
  File "../../electron/build/dump_syms.py", line 49, in <module>

See https://ci.appveyor.com/project/electron-bot/electron-x64-release/builds/25699649

Linux release builds are failing with a missing breakpad_symbols.zip, eg see: https://circleci.com/gh/electron/electron/249756

@@ -68,7 +68,7 @@ def main():
upload_electron(release, electron_zip, args)
if get_target_arch() != 'mips64el':
symbols_zip = os.path.join(OUT_DIR, SYMBOLS_NAME)
shutil.copy2(os.path.join(OUT_DIR, 'symbols.zip'), symbols_zip)
shutil.copy2(os.path.join(OUT_DIR, 'breakpad_symbols.zip'), symbols_zip)

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 2, 2019

Contributor

Where does this file actually get created?

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 2, 2019

Author Contributor

oops, i started down a path of getting gn to make the zip file too but then half-undid it. i'll revert this to symbols.zip, which is what zip-symbols.py creates.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Release builds for testing:
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/250205 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/250207 for status.
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/250206 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/250204 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/250209 for status.
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.92
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/250208 for status.
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.65

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Release builds here:
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/250481 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/250477 for status.
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/250480 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/250482 for status.
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/250479 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/250478 for status.
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.66
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.93

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Release builds:
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/250690 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/250687 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/250689 for status.
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/250691 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/250688 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/250692 for status.
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.67
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.94

@nornagon nornagon force-pushed the mac-dump-syms branch from 9577887 to fdb2c85 Jul 3, 2019

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Release builds:
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/251673 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/251669 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/251674 for status.
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/251672 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/251670 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/251671 for status.
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.69
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.96

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I was unable to backport this PR to "4-2-x" cleanly;
you will need to perform this backport manually.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

/trop run backport-to 5-0-x

@trop trop bot added the needs-manual-bp/4-2-x label Jul 5, 2019

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "5-0-x" here we go! :D

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

/trop run backport-to 6-0-x

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "6-0-x" here we go! :D

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I was unable to backport this PR to "5-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot added the needs-manual-bp/5-0-x label Jul 5, 2019

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot added the needs-manual-bp/6-0-x label Jul 5, 2019

@jkleinsc jkleinsc referenced this pull request Jul 5, 2019

Merged

build: fix ffmpeg gn gen #19127

3 of 3 tasks complete
@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19155

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #19157

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

A maintainer has manually backported this PR to "4-2-x", please check out #19158

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.