New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 753363 link

Starred by 1 user
Status: Fixed
Owner:
siggi@chromium.org
Closed: Sep 2017
Cc:
grt@chromium.org
robertshield@chromium.org
█ penny...@chromium.org
mark@chromium.org
scottmg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Chrome_elf.dll exports need discipline

Project Member Reported by siggi@chromium.org, Aug 8 2017 
chrome_elf.dll exports several functions to do with crash reporting. At the present time, these are declared with __declspec(dllexport) and some in /components/crash/content/app, which is then linked into all and sundry. As result, we export these functions from everything that directly or indirectly links it, including chrome_elf.dll, chrome.dll and chrome_child.dll.

There's other non-beauty in this code, notably the function signatures are declared at least twice, and on use there's at least two dips under the loader's lock for GetModuleHandle/GetProcAddress.

This https://chromium-review.googlesource.com/c/596416 CL eliminates one of those thunks for saner import binding, and it'd be a good idea to eliminate the rest as well.

Comment 1 by siggi@chromium.org, Aug 14 2017 

I investigated the size regression by replacing the offending __declspec(dllexport) declarations with a .def file, and it's not very big. Something on the order of 100K in the 32bit release build, seems to be the extent of it.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a36b8aa35df0b17f02056fc28494fdd3dcbc324e

commit a36b8aa35df0b17f02056fc28494fdd3dcbc324e
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Mon Aug 14 22:25:40 2017

Make chrome_elf exports explicit in .def files instead of using __declspec(dllexport).

The __declspec(dllexport) directives were bleeding across targets, so chrome.dll and
chrome_child.dll ended up exporting all the same things as chrome_elf.dll. This leads
to a small size regression - on the order of 100K in the 32 bit binaries.

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: I614c82dcb41f0372373b3942bff36215f057c4a1
Reviewed-on: https://chromium-review.googlesource.com/611963
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494214}
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/chrome_elf/BUILD.gn
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/chrome_elf/chrome_elf.def
[add] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/chrome_elf/chrome_elf_x64.def
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/chrome_elf/crash/crash_helper.cc
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/components/crash/content/app/crashpad.cc
[modify] https://crrev.com/a36b8aa35df0b17f02056fc28494fdd3dcbc324e/components/crash/content/app/crashpad_win.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c730d3c12cc2c67aaf16cecc4951277a67466aeb

commit c730d3c12cc2c67aaf16cecc4951277a67466aeb
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Thu Aug 17 11:52:59 2017

Fix CHECK failure in browser_tests.

This fixes the behavior in browser_tests to match what it was before
https://crrev.com/ea7ff6f0cd92cdf1c407f7f3a0873abb8c80e2d8, e.g. the
user data dir is not set in the path service unless a --user-data-dir
flag is provided.

TBR: grt@chromium.org

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: I80dfd90355b4e532fb94ff5d55df05bb96c5a597
Reviewed-on: https://chromium-review.googlesource.com/617605
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495144}
[modify] https://crrev.com/c730d3c12cc2c67aaf16cecc4951277a67466aeb/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/c730d3c12cc2c67aaf16cecc4951277a67466aeb/chrome_elf/chrome_elf_main.cc
[modify] https://crrev.com/c730d3c12cc2c67aaf16cecc4951277a67466aeb/chrome_elf/chrome_elf_main.h
[modify] https://crrev.com/c730d3c12cc2c67aaf16cecc4951277a67466aeb/chrome_elf/chrome_elf_test_stubs.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86ac09aa98e574ca69c9cd090f105683294a64b0

commit 86ac09aa98e574ca69c9cd090f105683294a64b0
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Tue Aug 22 16:18:41 2017

Use import binding instead of manual thunking for chrome_elf.

This replaces two of the manual GetModuleHandle/GetProcAddress 
dispatches to chrome_elf with import binding or static linking
where appropriate.

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: I74c2951daf53ef1d1e84154c0484c580f32c6df7
Reviewed-on: https://chromium-review.googlesource.com/606702
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496322}
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/chrome/app/BUILD.gn
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/chrome/installer/setup/BUILD.gn
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/chrome_elf/BUILD.gn
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/chrome_elf/crash/crash_helper.cc
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/components/crash/content/app/BUILD.gn
[add] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/components/crash/content/app/crash_export_thunks.cc
[add] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/components/crash/content/app/crash_export_thunks.h
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/components/crash/content/app/crashpad.cc
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/components/crash/content/app/crashpad.h
[modify] https://crrev.com/86ac09aa98e574ca69c9cd090f105683294a64b0/headless/BUILD.gn
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8cffa346e84dfa2a14653194a685301c88b3ecf

commit e8cffa346e84dfa2a14653194a685301c88b3ecf
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Mon Aug 28 22:47:58 2017

Replace explicit chrome_elf thunking with import binding.

Bug:  753363 
Change-Id: Ie16efbcd4b21a13f706d22ec934cde3752757a0a
Reviewed-on: https://chromium-review.googlesource.com/621367
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497916}
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/chrome/common/BUILD.gn
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/chrome/common/DEPS
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/chrome/common/child_process_logging_win.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/BUILD.gn
[add] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crash_export_stubs.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crash_export_thunks.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crash_export_thunks.h
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crashpad.cc
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crashpad.h
[modify] https://crrev.com/e8cffa346e84dfa2a14653194a685301c88b3ecf/components/crash/content/app/crashpad_win.cc

Comment 7 by siggi@chromium.org, Aug 29 2017 

Looks like there's only a few more stragglers:

chrome/browser/hang_monitor/hang_crash_dump_win.cc
- InjectDumpForHungInput
- InjectDumpForHungInputNoCrashKeys

chrome/child/v8_breakpad_support_win.cc:
- RegisterNonABICompliantCodeRange
- UnregisterNonABICompliantCodeRange

chrome/common/child_process_logging_win.cc:
- SetMetricsClientId
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 31 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9a1e914864e0b47faa4424a6e39fc2f7c09c972

commit a9a1e914864e0b47faa4424a6e39fc2f7c09c972
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Thu Aug 31 21:28:02 2017

Replace more explicit chrome_elf thunking with import binding.

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: Icb477bffe31f49ae2f2c5f99d80c309710b42e59
Reviewed-on: https://chromium-review.googlesource.com/643471
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499007}
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome/browser/hang_monitor/hang_crash_dump_win.cc
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome/child/BUILD.gn
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome/child/DEPS
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome/child/v8_breakpad_support_win.cc
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome_elf/BUILD.gn
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome_elf/chrome_elf_x64.def
[rename] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/chrome_elf/chrome_elf_x86.def
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/components/crash/content/app/crash_export_stubs.cc
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/components/crash/content/app/crash_export_thunks.cc
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/components/crash/content/app/crash_export_thunks.h
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/components/crash/content/app/crashpad.h
[modify] https://crrev.com/a9a1e914864e0b47faa4424a6e39fc2f7c09c972/components/crash/content/app/crashpad_win.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db6acf727419e4a6f865e7f5323f58dff51798bc

commit db6acf727419e4a6f865e7f5323f58dff51798bc
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Fri Sep 01 19:22:58 2017

Refactor: Rename all elf-exported crash thunks with an ExportThunk suffix.

This should be unique enough that the link will fail if the dependencies
are goofed. Turns out the Visual Studio linker is overly "helpful"
and tries very hard to make the link succeed even in the absence of
the functions named in a .DEF file. See  https://crbug.com/760385#c11 .
This also fixes the "upload now" link in chrome://crashes, which
regressed at https://chromium.googlesource.com/chromium/src.git/+/86ac09aa98e574ca69c9cd090f105683294a64b0

TBR=brettw@chromium.org

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: Ic5a0e2352e9f7956af3d57668a16b7df5397b9e8
Reviewed-on: https://chromium-review.googlesource.com/647332
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499254}
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome/browser/hang_monitor/hang_crash_dump_win.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome/child/v8_breakpad_support_win.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome/common/child_process_logging_win.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome_elf/chrome_elf_x64.def
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/chrome_elf/chrome_elf_x86.def
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/components/crash/content/app/crash_export_stubs.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/components/crash/content/app/crash_export_thunks.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/components/crash/content/app/crash_export_thunks.h
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/components/crash/content/app/crashpad.cc
[modify] https://crrev.com/db6acf727419e4a6f865e7f5323f58dff51798bc/components/crash/content/app/crashpad_win.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/320aa6517167f4680bbec57b8e4f74abdf3e380b

commit 320aa6517167f4680bbec57b8e4f74abdf3e380b
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Tue Sep 26 19:13:58 2017

Eliminate the remaining manual chrome_elf thunk.

Bug:  753363 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: Ic9045893035329b8434fd8ca7fe18735c57f842c
Reviewed-on: https://chromium-review.googlesource.com/684874
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504452}
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome/common/BUILD.gn
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome/common/DEPS
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome/common/child_process_logging_win.cc
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome/common/chrome_constants.cc
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome/common/chrome_constants.h
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/BUILD.gn
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/chrome_elf_main.cc
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/chrome_elf_main.h
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/chrome_elf_test_stubs.cc
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/crash/crash_helper.cc
[modify] https://crrev.com/320aa6517167f4680bbec57b8e4f74abdf3e380b/chrome_elf/crash/crash_helper.h

Comment 11 by siggi@chromium.org, Sep 27 2017

Status: Fixed (was: Started)
No manual thunking remains to chrome_elf in the Chrome code base. SyzyASAN still dispatches to a crash key export, but that's outside the scope of this bug.
►

Sign in to add a comment