Copy asan_rtl.* and agent_logger.exe to the syzygy directory for the ASan builds.

Copy asan_rtl.* and agent_logger.exe to the syzygy directory for the ASan builds.

Currently the syzygy_optimize script was taking care of copying asan_rtl.[dll|pdb] to the syzygy directory, this was causing 2 warnings when building a SyzyASan splitted build with ninja:

- ninja: warning: multiple rules generate syzygy\asan_rtl.dll. builds involving this target will not be correct; continuing anyway
- ninja: warning: multiple rules generate syzygy\asan_rtl.dll.pdb. builds involving this target will not be correct; continuing anyway

The multiple rules come from the fact that both syzygy\chrome.dll and syzygy\chrome_child.dll were trying to generate those artifacts.

I've also added agent_logger.exe to the syzygy directory, with this it'll be easier for the person grabing a SyzyASan build from the LKGR builder to use it.

I've also removed all the "--agent-dll" logic from syzygy_instrument.py as it was only copying the asan binaries to the syzygy directory, it's now done directly in the 'copy_syzyasan_binaries' gyp target.

R=chrisha@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222650

[Sébastien Marchand, 2013-09-06 21:02:03]

PTAL>

[chrisha, 2013-09-06 21:06:35]

lgtm!

[Sébastien Marchand, 2013-09-06 21:08:04]

+gab@ for owner approval. (We're working on getting a committer in the team :)
!)

[gab, 2013-09-09 13:26:28]

In description: s/more easier/easier :)!

Other comments/questions below.

Cheers!
Gab

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp#newc...
chrome/chrome_syzygy.gyp:37: ['OS=="win" and asan==1 and fastbuild==0', {
Why depend on |fastbuild| here?

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp#newc...
chrome/chrome_syzygy.gyp:45: 'target_name': 'copy_syzyasan_binaries',
Shouldn't this target depend on the target generating the binaries being copied
below?

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
File chrome/tools/build/win/syzygy_instrument.py (left):

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
chrome/tools/build/win/syzygy_instrument.py:100: agent_pdbs =
glob.glob(os.path.splitext(agent_dll)[0] + '*.pdb')
Some is the problem that this would result in "chrome*.pdb" which would also
match "chrome_child.dll.pdb"?

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
chrome/tools/build/win/syzygy_instrument.py:143:
option_parser.add_option('--agent_dll',
So this also removes the --agent_dll option; isn't that the one to instrument
chrome_child.dll? If so, please document in the description why you are removing
this too.

[Sébastien Marchand, 2013-09-10 17:53:42]

Thanks, PTAnL.

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp#newc...
chrome/chrome_syzygy.gyp:37: ['OS=="win" and asan==1 and fastbuild==0', {
No reason, cut-and-paste error. Thanks.

On 2013/09/09 13:26:28, gab wrote:
> Why depend on |fastbuild| here?

https://codereview.chromium.org/23460023/diff/1/chrome/chrome_syzygy.gyp#newc...
chrome/chrome_syzygy.gyp:45: 'target_name': 'copy_syzyasan_binaries',
On 2013/09/09 13:26:28, gab wrote:
> Shouldn't this target depend on the target generating the binaries being
copied
> below?

No, the target chrome_dll_syzygy and chrome_child_dll_syzygy should depend on
this one, this way when you build one of them you'll get the syzygy binaries
(asan_rtl.dll and agent_logger.exe) and their binaries in the syzygy directory.

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
File chrome/tools/build/win/syzygy_instrument.py (left):

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
chrome/tools/build/win/syzygy_instrument.py:100: agent_pdbs =
glob.glob(os.path.splitext(agent_dll)[0] + '*.pdb')
On 2013/09/09 13:26:28, gab wrote:
> Some is the problem that this would result in "chrome*.pdb" which would also
> match "chrome_child.dll.pdb"?

No, this function was used to copy the agent dll (asan_rtl.dll) to the syzygy
directory.

https://codereview.chromium.org/23460023/diff/1/chrome/tools/build/win/syzygy...
chrome/tools/build/win/syzygy_instrument.py:143:
option_parser.add_option('--agent_dll',
On 2013/09/09 13:26:28, gab wrote:
> So this also removes the --agent_dll option; isn't that the one to instrument
> chrome_child.dll? If so, please document in the description why you are
removing
> this too.

Yeah, this option was only used to copy the runtime (the agent, asan_rtl.dll)
and its pdb to the syzygy directory, this is now done directly via a target in
the gyp file.

[gab, 2013-09-10 18:22:54]

Okay, lgtm w/ question below.

I assume it's okay to use the same agent DLL for the main and child Chrome DLLs?

https://codereview.chromium.org/23460023/diff/8001/chrome/tools/build/win/syz...
File chrome/tools/build/win/syzygy_instrument.py (left):

https://codereview.chromium.org/23460023/diff/8001/chrome/tools/build/win/syz...
chrome/tools/build/win/syzygy_instrument.py:99: # transition.
Are those crazy days where we needed to support both .pdb and .dll.pdb over?

[Sébastien Marchand, 2013-09-10 18:43:05]

Thanks.

Yeah, we could use the same agent for for the main and child Chrome DLLs, the
agent is just a DLL implementing the heap interceptors and the
"asan_check_access" hooks.

https://codereview.chromium.org/23460023/diff/8001/chrome/tools/build/win/syz...
File chrome/tools/build/win/syzygy_instrument.py (left):

https://codereview.chromium.org/23460023/diff/8001/chrome/tools/build/win/syz...
chrome/tools/build/win/syzygy_instrument.py:99: # transition.
On 2013/09/10 18:22:55, gab wrote:
> Are those crazy days where we needed to support both .pdb and .dll.pdb over?

Yep, at least for Syzygy !

[commit-bot: I haz the power, 2013-09-10 19:03:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/23460023/8001

[commit-bot: I haz the power, 2013-09-10 19:25:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Sébastien Marchand, 2013-09-10 19:35:08]

+jam for owner approval. We'll transfer the ownership of this file to chrisha@
soon.

[gab, 2013-09-10 19:40:50]

On 2013/09/10 19:35:08, Sébastien Marchand wrote:
> +jam for owner approval. We'll transfer the ownership of this file to chrisha@
> soon.

Weird, I thought any committer could LGTM gyp changes (or maybe the rule is that
it's fine to TBR gyp only changes).

Cheers,
Gab

[jam, 2013-09-11 03:00:56]

Nico is a better reviewer for this, redirecting

[Nico, 2013-09-11 03:17:55]

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#n...
chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
Is this identical to the real asan runtime? If not, please call these
syzyasan_foo (or similar)

[Sébastien Marchand, 2013-09-11 04:25:39]

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#n...
chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
On 2013/09/11 03:17:55, Nico wrote:
> Is this identical to the real asan runtime? If not, please call these
> syzyasan_foo (or similar)

It's not identical to the "real" asan runtime, and we plan to rename it to
syzygy_asan_rtl.dll (https://codereview.appspot.com/8586044/). We're not ready
to do this yet but this is something that we'll do in the coming weeks...

[Nico, 2013-09-11 04:26:35]

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#n...
chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
On 2013/09/11 04:25:39, Sébastien Marchand wrote:
> On 2013/09/11 03:17:55, Nico wrote:
> > Is this identical to the real asan runtime? If not, please call these
> > syzyasan_foo (or similar)
> 
> It's not identical to the "real" asan runtime, and we plan to rename it to
> syzygy_asan_rtl.dll (https://codereview.appspot.com/8586044/). We're not ready
> to do this yet but this is something that we'll do in the coming weeks... 

I think that's the reply I got when I asked for this a few months ago too :-/

[Sébastien Marchand, 2013-09-11 04:34:42]

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp
File chrome/chrome_syzygy.gyp (right):

https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#n...
chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
On 2013/09/11 04:26:35, Nico wrote:
> On 2013/09/11 04:25:39, Sébastien Marchand wrote:
> > On 2013/09/11 03:17:55, Nico wrote:
> > > Is this identical to the real asan runtime? If not, please call these
> > > syzyasan_foo (or similar)
> > 
> > It's not identical to the "real" asan runtime, and we plan to rename it to
> > syzygy_asan_rtl.dll (https://codereview.appspot.com/8586044/). We're not
ready
> > to do this yet but this is something that we'll do in the coming weeks... 
> 
> I think that's the reply I got when I asked for this a few months ago too :-/

Oops, I wasn't aware that someone asked to do the renaming. In this case I'll
work on it asap (tomorrow, perf can wait :).

[chrisha, 2013-09-11 16:21:17]

On 2013/09/11 03:17:55, Nico wrote:
> https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp
> File chrome/chrome_syzygy.gyp (right):
> 
>
https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#n...
> chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
> Is this identical to the real asan runtime? If not, please call these
> syzyasan_foo (or similar)

This is easier said than done. We'd have to update a bunch of stuff across many
repositories, with some of those changes having to be timed. We'd also break
scripts that other clients have put together the next time they did a DEPS roll
(ClusterFuzz, LKGR builder, external security researchers, etc). If it were as
simple as a copy with rename I'd be on board, but then we have the issue that
the hardcoded paths in the .pdb and .dll that refer to each other no longer
match, and symbol file search derails.

From a practical point of view the likelihood of collision is quite small. The
original binary lives in third_party/syzygy and this build step only copies it
into the output directory if SyzyASAN is enabled. When ASAN gets to running with
Chrome on Windows there will be an entirely different set of build rules being
invoked, and the RTL will be copied by an ASAN specific build rule. The source
file will likely be coming from third_party/asan, and the only conflict would be
that ASAN and SyzyASAN build archives would each contain an asan_rtl.dll, of
different provenance. Is this a big issue?

(The proposed rename also bothers me because of its aesthetic impact on the
Syzygy repository; we have many instrumentation RTLs and binaries, and none of
them currently have a 'syzy' or 'syzygy' prefix.)

[Nico, 2013-09-11 18:05:45]

On Wed, Sep 11, 2013 at 9:21 AM, <chrisha@chromium.org> wrote:

> On 2013/09/11 03:17:55, Nico wrote:
>
>> https://codereview.chromium.**org/23460023/diff/8001/chrome/**
>>
chrome_syzygy.gyp<https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp>
>> File chrome/chrome_syzygy.gyp (right):
>>
>
>
> https://codereview.chromium.**org/23460023/diff/8001/chrome/**
>
chrome_syzygy.gyp#newcode78<https://codereview.chromium.org/23460023/diff/8001/chrome/chrome_syzygy.gyp#newcode78>
>
>> chrome/chrome_syzygy.gyp:78: '<(dest_dir)/asan_rtl.dll',
>>
>> Is this identical to the real asan runtime? If not, please call these
>> syzyasan_foo (or similar)
>>
>
> This is easier said than done. We'd have to update a bunch of stuff across
> many
> repositories, with some of those changes having to be timed. We'd also
> break
> scripts that other clients have put together the next time they did a DEPS
> roll
> (ClusterFuzz, LKGR builder, external security researchers, etc). If it
> were as
> simple as a copy with rename I'd be on board, but then we have the issue
> that
> the hardcoded paths in the .pdb and .dll that refer to each other no longer
> match, and symbol file search derails.
>

Understood, but this is only going to get worse over time.


>
> From a practical point of view the likelihood of collision is quite small.
> The
> original binary lives in third_party/syzygy and this build step only
> copies it
> into the output directory if SyzyASAN is enabled. When ASAN gets to
> running with
> Chrome on Windows there will be an entirely different set of build rules
> being
> invoked, and the RTL will be copied by an ASAN specific build rule. The
> source
> file will likely be coming from third_party/asan, and the only conflict
> would be
> that ASAN and SyzyASAN build archives would each contain an asan_rtl.dll,
> of
> different provenance. Is this a big issue?
>
> (The proposed rename also bothers me because of its aesthetic impact on the
> Syzygy repository; we have many instrumentation RTLs and binaries, and
> none of
> them currently have a 'syzy' or 'syzygy' prefix.)
>

I don't care much about the actual name as long as it's not just "asan".


>
>
https://codereview.chromium.**org/23460023/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chrisha, 2013-09-11 18:38:34]

> Understood, but this is only going to get worse over time.

I guess I just don't see what the problem is. The two RTLs will never be used
together. They live in different places in the repository. When Chrome is built,
they end up in different places in the output directory (the SyzyASAN runtime is
copied to the syzygy directory of the build output). The only place where there
would ever be chance for confusion is in looking at a physical installation of
an instrumented Chrome (or the contents of the mini-installer 7z), where the RTL
lives alongside chrome.dll and the name indicate whether it was SyzyASAN or ASAN
proper. But a cursory inspection of the module version would, as would an
inspection of the Chrome version string.

[Nico, 2013-09-11 18:40:20]

On Wed, Sep 11, 2013 at 11:38 AM, <chrisha@chromium.org> wrote:

> Understood, but this is only going to get worse over time.
>>
>
> I guess I just don't see what the problem is. The two RTLs will never be
> used
> together. They live in different places in the repository. When Chrome is
> built,
> they end up in different places in the output directory (the SyzyASAN
> runtime is
> copied to the syzygy directory of the build output). The only place where
> there
> would ever be chance for confusion is in looking at a physical
> installation of
> an instrumented Chrome (or the contents of the mini-installer 7z), where
> the RTL
> lives alongside chrome.dll and the name indicate whether it was SyzyASAN
> or ASAN
> proper. But a cursory inspection of the module version would, as would an
> inspection of the Chrome version string.
>

I think it's mostly confusing for people who try to get asan running.
That's already confusing enough that not enough people try.


>
>
>
https://codereview.chromium.**org/23460023/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chrisha, 2013-09-11 19:05:47]

Since this CL is blocking the LKGR builder we've deferred the rename for now.
I've opened a bug to track the rename and it should happen in pieces over the
next week or two.

http://crbug.com/289662

[Nico, 2013-09-11 19:12:06]

lgtm

[commit-bot: I haz the power, 2013-09-11 19:14:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/23460023/8001

[commit-bot: I haz the power, 2013-09-11 22:29:06]

Change committed as 222650