Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 2559053002: Instrument setup.exe in the SyzyAsan builds.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by Sébastien Marchand
Modified:
3 months, 2 weeks ago
Reviewers:
grt (UTC plus 2)
CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Instrument setup.exe in the SyzyAsan builds. This does several things: - Update the rules that generate syzygy/setup.exe so it's an instrumented copy of setup.exe (instead of a simple copy). - Use this instrumented version in syzygy/mini_installer.exe - Add the SyzyAsan runtime library (syzyasan_rtl) as a runtime dependency for setup.exe, make sure that it can be found by the installer. BUG=557759

Patch Set 1 #

Patch Set 2 : create_installer_archive changes. #

Patch Set 3 : create_installer_archive changes. #

Total comments: 11

Patch Set 4 : Rebase #

Total comments: 6

Patch Set 5 : BL compress syzyasan_rtl.dll #

Total comments: 1

Patch Set 6 : Fix the component build #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+284 lines, -87 lines) Patch
M build/win/syzygy/BUILD.gn View 1 chunk +26 lines, -8 lines 0 comments Download
M build/win/syzygy/instrument.py View 1 2 3 4 chunks +15 lines, -4 lines 0 comments Download
M build/win/syzygy/syzygy.gni View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/installer/mini_installer/BUILD.gn View 1 2 3 5 chunks +56 lines, -18 lines 0 comments Download
M chrome/installer/mini_installer/mini_installer.cc View 1 2 3 4 5 9 chunks +61 lines, -36 lines 2 comments Download
M chrome/installer/mini_installer/mini_installer_constants.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/installer/mini_installer/mini_installer_constants.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 1 comment Download
M chrome/installer/setup/setup_constants.h View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/installer/setup/setup_constants.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/installer/setup/setup_main.cc View 1 2 3 4 4 chunks +41 lines, -0 lines 0 comments Download
M chrome/tools/build/win/create_installer_archive.py View 1 2 3 4 5 6 chunks +48 lines, -21 lines 1 comment Download
M chrome/tools/build/win/syzygy/BUILD.gn View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 48 (29 generated)
grt (UTC plus 2)
Cool start! If I'm not mistaken, we'll need changes to: - create_installer_archive.py to include the ...
8 months, 1 week ago (2016-12-08 14:20:07 UTC) #6
Sébastien Marchand
On 2016/12/08 14:20:07, grt (UTC plus 1) wrote: > Cool start! If I'm not mistaken, ...
8 months, 1 week ago (2016-12-08 16:10:50 UTC) #7
Sébastien Marchand
This still isn't ready for real review, but as you're on the other side of ...
8 months, 1 week ago (2016-12-09 02:03:33 UTC) #13
grt (UTC plus 2)
This is looking pretty good! ...\Chrome\Application\W.X.Y.Z\Installer\setup.exe is a copy of setup that is used for ...
8 months, 1 week ago (2016-12-09 08:30:54 UTC) #14
grt (UTC plus 2)
Hey Gab: for component builds, does setup.exe have its dependent DLLs next to it in ...
8 months, 1 week ago (2016-12-09 08:34:54 UTC) #16
gab
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py File chrome/tools/build/win/create_installer_archive.py (left): https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py#oldcode494 chrome/tools/build/win/create_installer_archive.py:494: # TODO(jbauman): Remove when GYP is deprecated on Windows. ...
8 months, 1 week ago (2016-12-09 19:23:25 UTC) #18
gab
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py#newcode506 chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove when GYP is deprecated on Windows. ...
8 months, 1 week ago (2016-12-09 19:24:46 UTC) #19
gab
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py#newcode506 chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove when GYP is deprecated on Windows. ...
8 months, 1 week ago (2016-12-12 16:20:00 UTC) #20
grt (UTC plus 2)
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py#newcode506 chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove when GYP is deprecated on Windows. ...
8 months ago (2016-12-12 20:12:33 UTC) #21
Sébastien Marchand
There's nothing blocking this anymore, I've tried a mini_installer with this patchset on a VM ...
5 months, 1 week ago (2017-03-06 21:52:26 UTC) #28
grt (UTC plus 2)
Nice! I gave it a try in my checkout to see how things worked. I ...
5 months, 1 week ago (2017-03-10 07:57:36 UTC) #29
grt (UTC plus 2)
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py File chrome/tools/build/win/create_installer_archive.py (left): https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py#oldcode373 chrome/tools/build/win/create_installer_archive.py:373: resources.append((file, 'BN', os.path.join(installer_dir, file))) On 2017/03/06 21:52:26, Sébastien Marchand ...
5 months, 1 week ago (2017-03-10 09:08:10 UTC) #30
Sébastien Marchand
On 2017/03/10 09:08:10, grt (UTC plus 1) wrote: > https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/create_installer_archive.py > File chrome/tools/build/win/create_installer_archive.py (left): > ...
5 months, 1 week ago (2017-03-10 15:17:15 UTC) #31
grt (UTC plus 2)
On 2017/03/10 15:17:15, Sébastien Marchand wrote: > On 2017/03/10 09:08:10, grt (UTC plus 1) wrote: ...
5 months, 1 week ago (2017-03-10 16:28:25 UTC) #32
Sébastien Marchand
Using Makecab on all the deps (in a component build) is really slow, so I'm ...
4 months, 1 week ago (2017-04-05 21:07:02 UTC) #34
grt (UTC plus 2)
On 2017/03/10 07:57:36, grt (no reviews Apr 7-17) wrote: > Nice! I gave it a ...
4 months, 1 week ago (2017-04-06 10:38:54 UTC) #43
Sébastien Marchand
On 2017/04/06 10:38:54, grt (no reviews Apr 7-17) wrote: > On 2017/03/10 07:57:36, grt (no ...
4 months, 1 week ago (2017-04-06 17:35:46 UTC) #44
grt (UTC plus 2)
On 2017/04/06 17:35:46, Sébastien Marchand wrote: > Ha, forgot about this but I've tried this ...
3 months, 4 weeks ago (2017-04-19 13:17:36 UTC) #45
grt (UTC plus 2)
3 months, 4 weeks ago (2017-04-20 09:30:51 UTC) #47
i think you're getting really close. maybe now is a good time to clean up the CL
description.

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer.cc:343: Context* context) {
i had thought of Context as an implementation detail of UnpackBinaryResources.
did you change it to avoid having a variable number of args here? if you think
this is cleaner, please add some additional documentation to this fn so that
callers know how to prepare |context|.

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer.cc:865: Context context = {
maybe it's cleaner have Context contain the output PathString instances
themselves rather than pointers to them. also, it's seems a bit awkward to pass
|base_path| in Context rather than as its own arg to UnpackBinaryResources. if
you really want to avoid the #if around the args to UBR, maybe this public
Context should become ResourcePaths with the two or three output paths,
base_bath should be passed separately, and Context should stay an impl detail of
UBR (containing a pointer to base_path and a pointer to ResourcePaths). wdyt?

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer_constants.cc:17: const wchar_t
kSyzyAsanRuntimePrefix[] = L"syzyasan_rtl";
if this is only used in mini_installer.cc, move it there.

https://codereview.chromium.org/2559053002/diff/240001/chrome/tools/build/win...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/tools/build/win...
chrome/tools/build/win/create_installer_archive.py:338:
os.path.join(options.build_dir, SETUP_EXEC),
in my checkout, i find that this is compressing the pre-syzyasan setup.exe, not
the one in the syzygy dir. i think you need to plumb through a way for the
script invoker to pass in an optional path to setup.exe, or something, so that
the mini_installer_syzygy target uses syzygy/setup.exe rather than setup.exe. or
am i building it wrong?

C:\src\chromium\src>type out\asan\args.gn
enable_precompiled_headers = false
is_chrome_branded = true
is_debug = false
is_syzyasan = true
target_cpu = "x86"

C:\src\chromium\src>ninja -c out\asan mini_installer_syzygy
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b