Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12095105:
Add a script to produce a syzygy-instrumented image of Chrome.dll (Closed)
|
Created:
7 years, 10 months ago by Sébastien Marchand Modified:
7 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org, syzygy-team_google.com Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd a script to produce a syzygy-instrumented image of Chrome.dll
I've also modified the chrome_syzygy.gyp file to automaticaly produce the SyzyAsan'd version of Chrome.dll if 'asan'=1 on Windows.
TBR=sky@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181401
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Removes the logger from the archive. #
Total comments: 4
Patch Set 3 : Address Chris comments. #
Total comments: 6
Patch Set 4 : Address Greg's comments. #
Total comments: 1
Patch Set 5 : Address nits. #
Messages
Total messages: 19 (0 generated)
Here is a new approach to produce an instrumented version of Chrome's DLL. Is it the right way to do it ?
Sweet! Really nice! https://codereview.chromium.org/12095105/diff/2001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/12095105/diff/2001/chrome/chrome_syzygy.gyp#n... chrome/chrome_syzygy.gyp:66: '<(DEPTH)/chrome/tools/build/win/syzygy_instrument.py', stray thought: In principle you could go straight to instrument.exe here. But having the python script in the middle, as you've done, is probably a better choice as it creates a customization hook for the future. https://codereview.chromium.org/12095105/diff/2001/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/12095105/diff/2001/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:33: logger.exe: %(ChromeDir)s\ I'm not sure we want to include the logger in the package. To do so would further require (or at least benefit from) the inclusion of symsrv.dll as well. That said, does this actually result in the logger being included?
+chrisha@
A few nits. lgtm https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... File chrome/tools/build/win/syzygy_instrument.py (right): https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... chrome/tools/build/win/syzygy_instrument.py:19: 'third_party/syzygy/binaries/exe/instrument.exe') Why the nested joins, when one would do? Also, maybe use an os.path.abspath(os.path.join( .... )) so that the final path is as clear and concise as possible. https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... chrome/tools/build/win/syzygy_instrument.py:88: help='Instrumenter exectuable to use, defaults to "%s"' executable*
Thanks. +grt@ for owner approval. +syzygy-team@ https://codereview.chromium.org/12095105/diff/2001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/12095105/diff/2001/chrome/chrome_syzygy.gyp#n... chrome/chrome_syzygy.gyp:66: '<(DEPTH)/chrome/tools/build/win/syzygy_instrument.py', On 2013/02/01 18:54:30, Roger McFarlane (Chromium) wrote: > stray thought: > > In principle you could go straight to instrument.exe here. But having the > python script in the middle, as you've done, is probably a better choice as it > creates a customization hook for the future. Yeah, the python script allows us to do more stuff than just instrumenting the binary (i.e. Copying the agent DLL to the destination directory), and I think it's a good way to support more mode of instrumentation in the future... https://codereview.chromium.org/12095105/diff/2001/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/12095105/diff/2001/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:33: logger.exe: %(ChromeDir)s\ On 2013/02/01 18:54:30, Roger McFarlane (Chromium) wrote: > I'm not sure we want to include the logger in the package. To do so would > further require (or at least benefit from) the inclusion of symsrv.dll as well. > That said, does this actually result in the logger being included? It'll only include the logger if it finds it in the output directories (in the case of SyzyAsan this means out\Release and out\Release\syzygy). So the general use-case for our users would be to run an instrumented version on their machine, and if an error is found a minidump will be generated and send to us ? We'll be able to get the Asan report with just the minidump, and the matching version of chrome.dll/pdb ? In this case we should remove the logger from this list. And as we don't ship the PDB it's useless to include the logger.... https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... File chrome/tools/build/win/syzygy_instrument.py (right): https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... chrome/tools/build/win/syzygy_instrument.py:19: 'third_party/syzygy/binaries/exe/instrument.exe') On 2013/02/01 19:39:31, chrisha wrote: > Why the nested joins, when one would do? Also, maybe use an > os.path.abspath(os.path.join( .... )) so that the final path is as clear and > concise as possible. Done. https://codereview.chromium.org/12095105/diff/7001/chrome/tools/build/win/syz... chrome/tools/build/win/syzygy_instrument.py:88: help='Instrumenter exectuable to use, defaults to "%s"' On 2013/02/01 19:39:31, chrisha wrote: > executable* Done.
LGTM
Ping? +Gab@ has he seems to be an owner too.
Will you ever need the official builders to generate this? If so I think you also need to modify chrome/tools/build/win/FILES.cfg. @grt: I'm not 100% clear on the difference between chrome/tools/build/win/FILES.cfg and chrome/installer/mini_installer/chrome.release. I think the former is for the official build bots and chrome.release is just whatever is put in chrome.7z; they seem to overlap though... do the official build bots still use chrome.release (i.e. create_installer_archive.py)? Haven't looked at the details of the CL apart from the minor installer change which "looks good to me" apart from the comment below; I'll let Greg chime in as to the utility of FILES.cfg (looks like mmoss and kerz own that [1] if you want to ping them instead). Cheers! Gab [1] https://code.google.com/p/chromium/codesearch#chrome/src/chrome/tools/build/O... https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... chrome/installer/mini_installer/chrome.release:32: asan_rtl.dll: %(VersionDir)s\ Will you ever need this for Chromium? If not, put it in the GOOGLE_CHROME list below (note the sorting by destination, then by name in the GOOGLE_CHROME list).
FILES.cfg is primarily used to determine what build artifacts need need to be saved and in what "groupings" they belong (to prepare them for other types of post-build processing). It applies to both the archive_build step of the continuous builders (http://build.chromium.org/p/chromium/builders/Win/builds/14830/steps/archive_...) and the stage_build step of the official builders. Since the .7z files are listed in FILES.cfg, I don't think a file that is going in there needs to also be in FILES.cfg, unless you know you want it archived/processed separately for some reason. (Note, there probably are files listed in FILES.cfg which overlap with .7z contents, and which don't really need to be in FILES.cfg directly, but that's legacy junk, and not a configuration that should generally be emulated.)
On 2013/02/05 23:09:28, mmoss wrote: > FILES.cfg is primarily used to determine what build artifacts need need to be > saved and in what "groupings" they belong (to prepare them for other types of > post-build processing). It applies to both the archive_build step of the > continuous builders > (http://build.chromium.org/p/chromium/builders/Win/builds/14830/steps/archive_...) > and the stage_build step of the official builders. > > Since the .7z files are listed in FILES.cfg, I don't think a file that is going > in there needs to also be in FILES.cfg, unless you know you want it > archived/processed separately for some reason. (Note, there probably are files > listed in FILES.cfg which overlap with .7z contents, and which don't really need > to be in FILES.cfg directly, but that's legacy junk, and not a configuration > that should generally be emulated.) Ah, I see, thanks for the clarification Michael! So it sounds like, if all you're looking for is for your DLL to be in the version directory on ASAN versions of Chrome, what you have now is fine. LGTM!
On 2013/02/05 23:13:17, gab wrote: > On 2013/02/05 23:09:28, mmoss wrote: > > FILES.cfg is primarily used to determine what build artifacts need need to be > > saved and in what "groupings" they belong (to prepare them for other types of > > post-build processing). It applies to both the archive_build step of the > > continuous builders > > > (http://build.chromium.org/p/chromium/builders/Win/builds/14830/steps/archive_...) > > and the stage_build step of the official builders. > > > > Since the .7z files are listed in FILES.cfg, I don't think a file that is > going > > in there needs to also be in FILES.cfg, unless you know you want it > > archived/processed separately for some reason. (Note, there probably are files > > listed in FILES.cfg which overlap with .7z contents, and which don't really > need > > to be in FILES.cfg directly, but that's legacy junk, and not a configuration > > that should generally be emulated.) > > Ah, I see, thanks for the clarification Michael! > > So it sounds like, if all you're looking for is for your DLL to be in the > version directory on ASAN versions of Chrome, what you have now is fine. > > LGTM! (lgtm, but still with placement nit in chrome.release though!)
https://codereview.chromium.org/12095105/diff/10001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/12095105/diff/10001/chrome/chrome_syzygy.gyp#... chrome/chrome_syzygy.gyp:50: ['asan==1', { instead of a new condition, put this in the 'else' block of the asan!=1 condition; i.e.: ['asan!=1', {...the if block...}, {...the else block...}] see https://code.google.com/p/gyp/wiki/GypLanguageSpecification#Conditionals https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... chrome/installer/mini_installer/chrome.release:32: asan_rtl.dll: %(VersionDir)s\ On 2013/02/05 22:46:49, gab wrote: > Will you ever need this for Chromium? If not, put it in the GOOGLE_CHROME list > below (note the sorting by destination, then by name in the GOOGLE_CHROME list). If it does belong here in the GENERAL section, please move it up to line 9.
Done, waiting for Greg's approval to send it to the CQ :) https://codereview.chromium.org/12095105/diff/10001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/12095105/diff/10001/chrome/chrome_syzygy.gyp#... chrome/chrome_syzygy.gyp:50: ['asan==1', { On 2013/02/06 14:15:43, grt wrote: > instead of a new condition, put this in the 'else' block of the asan!=1 > condition; i.e.: > ['asan!=1', {...the if block...}, {...the else block...}] > see https://code.google.com/p/gyp/wiki/GypLanguageSpecification#Conditionals Done. https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... chrome/installer/mini_installer/chrome.release:32: asan_rtl.dll: %(VersionDir)s\ On 2013/02/05 22:46:49, gab wrote: > Will you ever need this for Chromium? If not, put it in the GOOGLE_CHROME list > below (note the sorting by destination, then by name in the GOOGLE_CHROME list). We might want to do an Asan build of Chromium (it's what I usually do), so I think our DLL belongs to this section :) https://codereview.chromium.org/12095105/diff/10001/chrome/installer/mini_ins... chrome/installer/mini_installer/chrome.release:32: asan_rtl.dll: %(VersionDir)s\ On 2013/02/06 14:15:43, grt wrote: > On 2013/02/05 22:46:49, gab wrote: > > Will you ever need this for Chromium? If not, put it in the GOOGLE_CHROME list > > below (note the sorting by destination, then by name in the GOOGLE_CHROME > list). > > If it does belong here in the GENERAL section, please move it up to line 9. Done.
lgtm w/ a nit https://codereview.chromium.org/12095105/diff/17003/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/12095105/diff/17003/chrome/chrome_syzygy.gyp#... chrome/chrome_syzygy.gyp:50: { nit: this is commonly on the line above (e.g., "}, {").
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/12095105/27001
Presubmit check for 12095105-27001 failed and returned exit status 1.
/b/commit-queue/workdir/chromium/chrome/tools/build/win/syzygy_instrument.py:
Has shebang but not executable bit
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.2s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/12095105/27001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/12095105/27001
Message was sent while issue was closed.
Change committed as 181401 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
