Add option to export tracing events to ETW.

Add option to export tracing events to ETW.

- Exporting of ETW events can be turned on using --trace-export-events-to-etw.

BUG=

Committed: https://crrev.com/1cc86c4f686869f32dfede093a07828c73563892
Cr-Commit-Position: refs/heads/master@{#324982}

Committed: https://crrev.com/28ad5e6b1100a7f0d25a5e6741f7241a86cf61bd
Cr-Commit-Position: refs/heads/master@{#325408}

[Georges Khalil, 2015-03-24 18:32:41]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2015-03-24 18:39:40]

Patchset #1 (id:20001) has been deleted

[Georges Khalil, 2015-03-24 18:40:34]

georgesak@chromium.org changed reviewers:
+ brucedawson@chromium.org

[Georges Khalil, 2015-03-24 19:01:42]

Patchset #1 (id:40001) has been deleted

[Georges Khalil, 2015-03-24 22:52:18]

Patchset #2 (id:80001) has been deleted

[Georges Khalil, 2015-03-24 22:58:12]

Patchset #2 (id:100001) has been deleted

[Georges Khalil, 2015-03-24 23:01:15]

georgesak@chromium.org changed reviewers:
+ chrisha@chromium.org, gab@chromium.org

[Georges Khalil, 2015-03-25 21:19:39]

Patchset #4 (id:160001) has been deleted

[Georges Khalil, 2015-03-25 22:49:16]

Patchset #4 (id:180001) has been deleted

[Georges Khalil, 2015-03-25 22:58:06]

Patchset #4 (id:200001) has been deleted

[Georges Khalil, 2015-03-26 16:05:29]

Patchset #4 (id:210001) has been deleted

[Georges Khalil, 2015-03-26 16:05:37]

Patchset #4 (id:230001) has been deleted

[Georges Khalil, 2015-03-26 16:05:44]

Patchset #4 (id:250001) has been deleted

[Georges Khalil, 2015-03-26 16:05:51]

Patchset #4 (id:270001) has been deleted

[Georges Khalil, 2015-03-26 16:47:37]

Patchset #4 (id:290001) has been deleted

[Georges Khalil, 2015-03-26 16:47:52]

Patchset #4 (id:290001) has been deleted

[Georges Khalil, 2015-03-26 16:47:59]

Patchset #4 (id:310001) has been deleted

[Georges Khalil, 2015-03-26 17:28:12]

georgesak@chromium.org changed reviewers:
+ dsinclair@chromium.org
- brucedawson@chromium.org, chrisha@chromium.org, gab@chromium.org

[dsinclair, 2015-03-26 18:18:17]

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:62: case TRACE_EVENT_PHASE_BEGIN:
This seems fragile to me. This list is going to get out of date as soon as we
add a new phase. Would be better to just output the phase code as that would
continue to work no matter what.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.cc:1906: if (*category_group_enabled &
ENABLED_FOR_ETW_EXPORT)
I think this should be down around where the android call is below.   We should
also move this to a method on TraceEvent like Android does with SendToATrace. So
this would have a SendToETW that would be called if needed.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.cc:2132:
TraceEventETWExport::AddEvent(TRACE_EVENT_PHASE_END, category_group_enabled,
This isn't what you want here. This is just going to log an end phase, but you
lose the X event. The event is created down around line 2151 which is what needs
to be sent.

(Similar to how the event is SendToATrace())

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_impl.h (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.h:28: // Only these macros result in
publishing data to ETW as currently implemented.
This is no longer correct.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.h:29: // TODO(georgesak): Update/replace these
with new ETW macros.
In theory, we won't need ETW custom macros anymore, yea?

https://codereview.chromium.org/1038453002/diff/330001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/content/app/content_ma...
content/app/content_main_runner.cc:639:
base::trace_event::TraceEventETWExport::EnableETWExport();
There is more that needs to be done with this switch. It needs to be passed down
to sub processes when they're started. If you search for one of the other
tracing flags you should be able to find where it is also passed.

[nduca, 2015-03-26 18:48:41]

nduca@chromium.org changed reviewers:
+ nduca@chromium.org

[nduca, 2015-03-26 18:48:42]

this seems good enough. lgtm on my part but please await dan's lg

[dsinclair, 2015-03-26 19:04:35]

On 2015/03/26 at 18:48:42, nduca wrote:
> this seems good enough. lgtm on my part but please await dan's lg

See https://codereview.chromium.org/335103007/ for an example of removing a flag
and where it needed to be added.

[Georges Khalil, 2015-03-26 23:08:40]

Patchset #5 (id:350001) has been deleted

[Georges Khalil, 2015-03-26 23:09:20]

PTAnL.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:62: case TRACE_EVENT_PHASE_BEGIN:
On 2015/03/26 18:18:17, dsinclair wrote:
> This seems fragile to me. This list is going to get out of date as soon as we
> add a new phase. Would be better to just output the phase code as that would
> continue to work no matter what.

I added a default statement that outputs the code. That way, we get to keep the
formatted names for existing phases but still get meaningful info for new phases
until they get added here too.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.cc:1906: if (*category_group_enabled &
ENABLED_FOR_ETW_EXPORT)
On 2015/03/26 18:18:17, dsinclair wrote:
> I think this should be down around where the android call is below.   We
should
> also move this to a method on TraceEvent like Android does with SendToATrace.
So
> this would have a SendToETW that would be called if needed.

After discussion, brought this down but before the lock gets acquired, as this
is not needed for ETW. It also saves us from creating the event if internal
tracing is not enabled.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.cc:2132:
TraceEventETWExport::AddEvent(TRACE_EVENT_PHASE_END, category_group_enabled,
On 2015/03/26 18:18:17, dsinclair wrote:
> This isn't what you want here. This is just going to log an end phase, but you
> lose the X event. The event is created down around line 2151 which is what
needs
> to be sent.
> 
> (Similar to how the event is SendToATrace())

After review and discussion, this code should be removed for now as there is no
notion of duration in ETW.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
File base/trace_event/trace_event_impl.h (right):

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.h:28: // Only these macros result in
publishing data to ETW as currently implemented.
On 2015/03/26 18:18:17, dsinclair wrote:
> This is no longer correct.

Acknowledged.

https://codereview.chromium.org/1038453002/diff/330001/base/trace_event/trace...
base/trace_event/trace_event_impl.h:29: // TODO(georgesak): Update/replace these
with new ETW macros.
On 2015/03/26 18:18:17, dsinclair wrote:
> In theory, we won't need ETW custom macros anymore, yea?

Correct. Right now, we have old ETW tracing as well as the new one added in this
CL.

I'm planning on converting the old code to use the new one.

https://codereview.chromium.org/1038453002/diff/330001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1038453002/diff/330001/content/app/content_ma...
content/app/content_main_runner.cc:639:
base::trace_event::TraceEventETWExport::EnableETWExport();
On 2015/03/26 18:18:17, dsinclair wrote:
> There is more that needs to be done with this switch. It needs to be passed
down
> to sub processes when they're started. If you search for one of the other
> tracing flags you should be able to find where it is also passed.

Done.

[dsinclair, 2015-03-26 23:12:47]

lgtm

[brucedawson, 2015-03-27 19:57:22]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-03-27 19:57:23]

I have a couple of small efficiency suggestions - if possible we want to avoid
doing any memory allocations when emitting events.

But I have a separate larger concern. The code as it stands will prevent Chrome
from launching on Windows XP, I think. This is easy to fix.

The problem is that we call EventWrite, EventRegister, and EventUnregister. We
need to delay load or do the GetProcAddress dance. If we do GetProcAddress then
we need to provide EventWrite, EventRegister, and EventUnregister functions so
that the ETW macros will use them.

The efficiency changes could be made in a later CL but if I am correct about
Windows XP then that should be addressed prior to landing.

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:60: std::string phase_string;
This could easily be a const char*, which would avoid memory allocation. In most
cases the switch statement wouldn't need to change. The default case would need
a two-byte array to put the phase character in.

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:127: std::string
arg_values_string[3];
I suspect that these string objects could be implemented as some mixture of
char* and char[].

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:141:
arg_values_string[0].c_str(), num_args > 1 ? arg_names[1] : "",
It seems that this would be clearer if each line had arg_names[n] and then
arg_values_string[n], instead of arg_values_string[n] followed by
arg_names[n+1].

That is, have two arguments on the very first line instead of three.

[Georges Khalil, 2015-03-28 23:11:07]

Patchset #6 (id:390001) has been deleted

[Georges Khalil, 2015-03-29 01:00:21]

PTAnL.

Following Bruce's comments:
- Fixed the Windows XP issue by using GetProcAddress (tested on an XP machine).
- Memory optimizations will be tackled in a subsequent CL.

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:60: std::string phase_string;
On 2015/03/27 19:57:23, brucedawson wrote:
> This could easily be a const char*, which would avoid memory allocation. In
most
> cases the switch statement wouldn't need to change. The default case would
need
> a two-byte array to put the phase character in.

Acknowledged.

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:127: std::string
arg_values_string[3];
On 2015/03/27 19:57:23, brucedawson wrote:
> I suspect that these string objects could be implemented as some mixture of
> char* and char[].

Acknowledged.

https://codereview.chromium.org/1038453002/diff/370001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:141:
arg_values_string[0].c_str(), num_args > 1 ? arg_names[1] : "",
On 2015/03/27 19:57:23, brucedawson wrote:
> It seems that this would be clearer if each line had arg_names[n] and then
> arg_values_string[n], instead of arg_values_string[n] followed by
> arg_names[n+1].
> 
> That is, have two arguments on the very first line instead of three.

This will change with the optimizations to be done (currently, that's the output
of git cl format).

[brucedawson, 2015-03-29 20:39:23]

The Windows XP changes lgtm with two comment fixes needed.

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:15: //
https://github.com/someonegg/heap_monitor/tree/master/MultiProvider
Can you update this reference to this address:
https://github.com/randomascii/main/tree/master/xperf/ETWProviders

You can trim off ETWProviders or xperf/ETWProviders if you want. The 'someonegg'
repo is just some random person who copied the original MultiProvider, whereas
the (just added) ETWProviders folder is the latest/greatest/canonical version.

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:24: #include
"base/trace_event/etw_manifest/chrome_events_win.h"  // In
SHARED_INTERMEDIATE_DIR. NOLINT.
Move the comment to stay in line limits?

[Georges Khalil, 2015-03-30 14:50:04]

georgesak@chromium.org changed reviewers:
+ nasko@chromium.org, thakis@chromium.org

[Georges Khalil, 2015-03-30 14:50:05]

thakis@chromium.org: Please review changes in 
- base/base.gyp
- chrome/BUILD.gn

nasko@chromium.org: Please review changes in 
- content/app/content_main_runner.cc
- content/browser/renderer_host/render_process_host_impl.cc
- content/public/common/content_switches.h
- content/public/common/content_switches.cc

Thank you.

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:15: //
https://github.com/someonegg/heap_monitor/tree/master/MultiProvider
On 2015/03/29 20:39:22, brucedawson wrote:
> Can you update this reference to this address:
> https://github.com/randomascii/main/tree/master/xperf/ETWProviders
> 
> You can trim off ETWProviders or xperf/ETWProviders if you want. The
'someonegg'
> repo is just some random person who copied the original MultiProvider, whereas
> the (just added) ETWProviders folder is the latest/greatest/canonical version.

Oops, didn't know that it was copied, should have dug deeper!

Link fixed.

https://codereview.chromium.org/1038453002/diff/410001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:24: #include
"base/trace_event/etw_manifest/chrome_events_win.h"  // In
SHARED_INTERMEDIATE_DIR. NOLINT.
On 2015/03/29 20:39:22, brucedawson wrote:
> Move the comment to stay in line limits?

Done.

- Moved In SHARED_INTERMEDIATE_DIR to earlier line.
- Also removed trailing dot.

[nasko, 2015-03-30 22:45:38]

content/ LGTM

[Nico, 2015-03-31 22:26:08]

https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:136:
'<(SHARED_INTERMEDIATE_DIR)/chrome_version/chrome_dll_version.rc',
huh, this looks wrong to me. I haven't seen <(SHARED_I_D) in sources sections.
Normally, the action generating the rc file would instead have a
process_outputs_as_sources: 1 entry.

[Georges Khalil, 2015-04-01 16:47:04]

Nico, PTAnL.

- Fix order of source files and add missing dependency in chrome_dll.gypi
- Use $root_gen_dir instead of $target_gen_dir in BUILD.gn

https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:136:
'<(SHARED_INTERMEDIATE_DIR)/chrome_version/chrome_dll_version.rc',
On 2015/03/31 22:26:08, Nico wrote:
> huh, this looks wrong to me. I haven't seen <(SHARED_I_D) in sources sections.
> Normally, the action generating the rc file would instead have a
> process_outputs_as_sources: 1 entry.

The diff is misleading as this is actually an existing line. What was added is
the line below it. SHARED_I_D was already being used here in other source files,
the added line is keeping the same format.

The order was wrong and a dependency was missing, both fixed.

[Nico, 2015-04-01 18:23:44]

On 2015/04/01 16:47:04, Georges Khalil wrote:
> Nico, PTAnL.
> 
> - Fix order of source files and add missing dependency in chrome_dll.gypi
> - Use $root_gen_dir instead of $target_gen_dir in BUILD.gn
> 
> https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi
> File chrome/chrome_dll.gypi (right):
> 
>
https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi...
> chrome/chrome_dll.gypi:136:
> '<(SHARED_INTERMEDIATE_DIR)/chrome_version/chrome_dll_version.rc',
> On 2015/03/31 22:26:08, Nico wrote:
> > huh, this looks wrong to me. I haven't seen <(SHARED_I_D) in sources
sections.
> > Normally, the action generating the rc file would instead have a
> > process_outputs_as_sources: 1 entry.
> 
> The diff is misleading as this is actually an existing line. What was added is
> the line below it. SHARED_I_D was already being used here in other source
files,

Yes, understood. I'm saying it looks wrong. Does process_output_as_sources not
work for these files?

[Georges Khalil, 2015-04-01 21:13:09]

On 2015/04/01 18:23:44, Nico wrote:
> On 2015/04/01 16:47:04, Georges Khalil wrote:
> > Nico, PTAnL.
> > 
> > - Fix order of source files and add missing dependency in chrome_dll.gypi
> > - Use $root_gen_dir instead of $target_gen_dir in BUILD.gn
> > 
> >
https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi
> > File chrome/chrome_dll.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1038453002/diff/430001/chrome/chrome_dll.gypi...
> > chrome/chrome_dll.gypi:136:
> > '<(SHARED_INTERMEDIATE_DIR)/chrome_version/chrome_dll_version.rc',
> > On 2015/03/31 22:26:08, Nico wrote:
> > > huh, this looks wrong to me. I haven't seen <(SHARED_I_D) in sources
> sections.
> > > Normally, the action generating the rc file would instead have a
> > > process_outputs_as_sources: 1 entry.
> > 
> > The diff is misleading as this is actually an existing line. What was added
is
> > the line below it. SHARED_I_D was already being used here in other source
> files,
> 
> Yes, understood. I'm saying it looks wrong. Does process_output_as_sources not
> work for these files?

It doesn't seem to want to work. I have process_outputs_as_sources enabled in 
etw_manifest.gyp. But I can't simply use this as source file, it fails:
  ../base/trace_event/etw_manifest/chrome_events_win.rc

Unless I'm missing something?

[Nico, 2015-04-09 23:05:53]

thakis@chromium.org changed reviewers:
+ scottmg@chromium.org
- thakis@chromium.org

[Nico, 2015-04-09 23:05:54]

I looked through gyp's ninja generator, and the process_outputs_as_sources thing
should work from what I understand.

I'm away the next few days – scottmg, can you look at the references to the
generated files in this CL? Having a <(IMMEDIATE_DIR) thingy in 'sources' looks
weird to me, and I'm not sure if it's necessary (see last few comments on this
review). But you probably know better than me how to do this, it being about win
rc files.

[scottmg, 2015-04-09 23:45:34]

I think process_outputs_as_sources will mean that the .rc should get built in
the etw_manifest target, and the corresponding .res will be in the
etw_manifest.lib. So in chrome_dll, it shouldn't be necessary to also mention
the .rc again.

Is that not what's happening? Or there's some reason why the .res in the .lib is
insufficient?

https://codereview.chromium.org/1038453002/diff/450001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/etw_manifest.gyp (right):

https://codereview.chromium.org/1038453002/diff/450001/base/trace_event/etw_m...
base/trace_event/etw_manifest/etw_manifest.gyp:10: 'conditions': [
please use 2 space indent, not tabs

https://codereview.chromium.org/1038453002/diff/450001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/450001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:136: 
no tabs, and remove whitespace at eol

[brucedawson, 2015-04-10 00:19:42]

I don't think you can insert a resource into a .lib file -- they can only be
inserted into PE files.

[scottmg, 2015-04-10 01:20:22]

On 2015/04/10 00:19:42, brucedawson wrote:
> I don't think you can insert a resource into a .lib file -- they can only be
> inserted into PE files.

http://stackoverflow.com/questions/531502/vc-resources-in-a-static-library would
seem to agree. So I guess, remove process_outputs_as_sources from etw_manifest
to avoid confusion since that's not doing anything useful, and the whitespace
changes, and then lg.

[Georges Khalil, 2015-04-10 22:29:50]

scottmg@, PTAnL.

- process_outputs_as_sources removed.
- Whitespace fixed.
- Removed dependency in chrome_dll.gypi, that was causing the ressource to be
added twice when building in component mode (linking would fail).

https://codereview.chromium.org/1038453002/diff/450001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/etw_manifest.gyp (right):

https://codereview.chromium.org/1038453002/diff/450001/base/trace_event/etw_m...
base/trace_event/etw_manifest/etw_manifest.gyp:10: 'conditions': [
On 2015/04/09 23:45:34, scottmg wrote:
> please use 2 space indent, not tabs

Done.

https://codereview.chromium.org/1038453002/diff/450001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/450001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:136: 
On 2015/04/09 23:45:34, scottmg wrote:
> no tabs, and remove whitespace at eol

Removed tabs. Cannot fine whitespace at eol, which line is it?

[scottmg, 2015-04-10 22:40:50]

gyp[i] lgtm

[scottmg, 2015-04-10 22:41:57]

https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/etw_manifest.gyp (right):

https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
base/trace_event/etw_manifest/etw_manifest.gyp:29: '-r', '<(man_output_dir)/.',
Actually, sorry. Why do you need the trailing "/."? There was a bug in MSVS
actions long ago for that, be we don't need that any more.

[Georges Khalil, 2015-04-10 23:04:33]

On 2015/04/10 22:41:57, scottmg wrote:
>
https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
> File base/trace_event/etw_manifest/etw_manifest.gyp (right):
> 
>
https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
> base/trace_event/etw_manifest/etw_manifest.gyp:29: '-r',
'<(man_output_dir)/.',
> Actually, sorry. Why do you need the trailing "/."? There was a bug in MSVS
> actions long ago for that, be we don't need that any more.

If I remove it, mc.exe fails. I'm guessing it's still needed for that particular
tool.

[scottmg, 2015-04-10 23:08:46]

On 2015/04/10 23:04:33, Georges Khalil wrote:
> On 2015/04/10 22:41:57, scottmg wrote:
> >
>
https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
> > File base/trace_event/etw_manifest/etw_manifest.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1038453002/diff/470001/base/trace_event/etw_m...
> > base/trace_event/etw_manifest/etw_manifest.gyp:29: '-r',
> '<(man_output_dir)/.',
> > Actually, sorry. Why do you need the trailing "/."? There was a bug in MSVS
> > actions long ago for that, be we don't need that any more.
> 
> If I remove it, mc.exe fails. I'm guessing it's still needed for that
particular
> tool.

OK. Does the GN path work then? It looks like it doesn't have the trailer.

[Georges Khalil, 2015-04-10 23:20:17]

georgesak@chromium.org changed reviewers:
+ thestig@chromium.org

[Georges Khalil, 2015-04-10 23:20:18]

thestig@chromium.org: Please review changes in 

- base/base.gyp (LGTMed by scottmg)
- chrome/BUILD.gn

[Lei Zhang, 2015-04-10 23:50:13]

Mostly trying to make sure the GYP and GN changes match.

https://codereview.chromium.org/1038453002/diff/490001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1038453002/diff/490001/base/base.gyp#newcode692
base/base.gyp:692: 'trace_event/etw_manifest/etw_manifest.gyp:etw_manifest',
Where's the equivalent GN dependency?

https://codereview.chromium.org/1038453002/diff/490001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/BUILD.gn (right):

https://codereview.chromium.org/1038453002/diff/490001/base/trace_event/etw_m...
base/trace_event/etw_manifest/BUILD.gn:10: "//chrome:main_dll",
nit: alphabetical order

https://codereview.chromium.org/1038453002/diff/490001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/1038453002/diff/490001/chrome/BUILD.gn#newcod...
chrome/BUILD.gn:176: "//base/trace_event/etw_manifest:chrome_events_win",
Should the chrome gypi files have a similar dependency?

[Georges Khalil, 2015-04-11 18:06:17]

Patchset #11 (id:510001) has been deleted

[Georges Khalil, 2015-04-11 19:01:24]

Patchset #11 (id:530001) has been deleted

[Georges Khalil, 2015-04-11 19:01:36]

Patchset #11 (id:530001) has been deleted

[Georges Khalil, 2015-04-11 19:07:45]

Patchset #11 (id:550001) has been deleted

[Georges Khalil, 2015-04-13 13:44:46]

thestig@, PTAnL.

- Fixed discrepancy between GYP and GN. They should both be similar now.

https://codereview.chromium.org/1038453002/diff/490001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1038453002/diff/490001/base/base.gyp#newcode692
base/base.gyp:692: 'trace_event/etw_manifest/etw_manifest.gyp:etw_manifest',
On 2015/04/10 23:50:13, Lei Zhang wrote:
> Where's the equivalent GN dependency?

After verification, this is actually misplaced and has been moved to
chrome_dll.gypi instead.

https://codereview.chromium.org/1038453002/diff/490001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/BUILD.gn (right):

https://codereview.chromium.org/1038453002/diff/490001/base/trace_event/etw_m...
base/trace_event/etw_manifest/BUILD.gn:10: "//chrome:main_dll",
On 2015/04/10 23:50:13, Lei Zhang wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/1038453002/diff/490001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/1038453002/diff/490001/chrome/BUILD.gn#newcod...
chrome/BUILD.gn:176: "//base/trace_event/etw_manifest:chrome_events_win",
On 2015/04/10 23:50:13, Lei Zhang wrote:
> Should the chrome gypi files have a similar dependency?

Yes, it was wrongly placed in base/base.gyp. Dependency added in chrome_dll.gypi
instead.

[Lei Zhang, 2015-04-13 19:55:31]

lgtm

[Georges Khalil, 2015-04-13 20:05:04]

The CQ bit was checked by georgesak@chromium.org

[Georges Khalil, 2015-04-13 20:05:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
dsinclair@chromium.org, brucedawson@chromium.org, nasko@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1038453002/#ps570001
(title: "thestig@ review.")

[commit-bot: I haz the power, 2015-04-13 20:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038453002/570001

[commit-bot: I haz the power, 2015-04-14 01:36:26]

Committed patchset #11 (id:570001)

[commit-bot: I haz the power, 2015-04-14 01:38:38]

Patchset 11 (id:??) landed as
https://crrev.com/1cc86c4f686869f32dfede093a07828c73563892
Cr-Commit-Position: refs/heads/master@{#324982}

[Nico, 2015-04-14 02:25:08]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-04-14 02:25:09]

https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:46: // chrome_events_win.h
I can't see a file chrome_events_win.h, and I also don't really see macros in
this CL – what does this refer to?

[Nico, 2015-04-14 02:27:13]

On 2015/04/14 02:25:09, Nico (away until Wed Apr 15) wrote:
>
https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
> File base/trace_event/trace_event_etw_export_win.cc (right):
> 
>
https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
> base/trace_event/trace_event_etw_export_win.cc:46: // chrome_events_win.h
> I can't see a file chrome_events_win.h, and I also don't really see macros in
> this CL – what does this refer to?

Nevermind, apparently it's generated from the .man file?

[Nico, 2015-04-14 02:31:58]

https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:138:
'<(SHARED_INTERMEDIATE_DIR)/base/trace_event/etw_manifest/chrome_events_win.rc',
Shouldn't this be in base/base.gyp? As-is, the clang/win bot reports

base.trace_event_etw_export_win.obj :error LNK2019: unresolved external symbol
ChromeEnableBits referenced in function "public: static void __cdecl
base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char const *,char
const *,unsigned __int64,int,char const * *,unsigned char const *,unsigned
__int64 const *,class scoped_refptr<class
base::trace_event::ConvertableToTraceFormat> const *)"
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPEBEPEBD_KHPEAPEBD0PEB_KPEBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)

and I'm guessing that symbol comes from that .rc file.

(and cl.exe happens to inline differently)

[pdr., 2015-04-14 03:29:20]

On 2015/04/14 at 02:31:58, thakis wrote:
> https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi
> File chrome/chrome_dll.gypi (right):
> 
>
https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi...
> chrome/chrome_dll.gypi:138:
'<(SHARED_INTERMEDIATE_DIR)/base/trace_event/etw_manifest/chrome_events_win.rc',
> Shouldn't this be in base/base.gyp? As-is, the clang/win bot reports
> 
> base.trace_event_etw_export_win.obj :error LNK2019: unresolved external symbol
ChromeEnableBits referenced in function "public: static void __cdecl
base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char const *,char
const *,unsigned __int64,int,char const * *,unsigned char const *,unsigned
__int64 const *,class scoped_refptr<class
base::trace_event::ConvertableToTraceFormat> const *)"
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPEBEPEBD_KHPEAPEBD0PEB_KPEBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)
> 
> and I'm guessing that symbol comes from that .rc file.
> 
> (and cl.exe happens to inline differently)

I think this CL broke the blink win bots:
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...

[pdr., 2015-04-14 03:55:05]

Our revert button is not quite teched up to handle this patch. I created a
revert at https://codereview.chromium.org/1087733002 and filed
http://crbug.com/476806.

[Georges Khalil, 2015-04-14 15:56:50]

Patchset #12 (id:590001) has been deleted

[Georges Khalil, 2015-04-14 16:37:59]

Patchset #12 (id:610001) has been deleted

[Georges Khalil, 2015-04-14 17:57:42]

thestig@, PTAnL.

This got reverted as it broke a buildbot. The dependency in base.gyp is needed
after all, the fact that it was compiling without it was luck (race condition).

The equivalent is already there in base/trace_event/BUILD.gn.

[brucedawson, 2015-04-14 18:05:16]

And, to respond to pdr@'s question, function definitions don't come from .rc
files -- the AddEvent function comes from a .cc file.

[Lei Zhang, 2015-04-15 08:10:36]

Can you address Nico's most recent comment?

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/BUILD/message_compiler.py (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
base/trace_event/etw_manifest/BUILD/message_compiler.py:6: # GYP build, which
can only run Python and not native binaries.
message_compiler.py is only being referenced from
base/trace_event/etw_manifest/BUILD.gn, and
base/trace_event/etw_manifest/etw_manifest.gyp calls mc.exe directly. Did the
original script you copied from mean to say GN?

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/etw_manifest.gyp (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
base/trace_event/etw_manifest/etw_manifest.gyp:29: '-r', '<(man_output_dir)/.',
why do you need the extra /. at the end?

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.h (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:32: static void AddEvent(
The above methods seem more obvious, but AddEvent() and AddCustomEvent() can use
some documentation.

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:43: static void
AddCustomEvent(const char* name,
This doesn't seem to have any callers?

https://codereview.chromium.org/1038453002/diff/630001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/630001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:138:
'<(SHARED_INTERMEDIATE_DIR)/base/trace_event/etw_manifest/chrome_events_win.rc',
Based on your content/ changes, it looks like the new code is being used in the
renderer process... so do you need to add this to chrome_child.dll as well?

[Georges Khalil, 2015-04-15 15:10:33]

Nico, Lei, PTAnl.

My apologies for having missed Nico's comments, they got lost in the midst of
dealing with the revert.

Everything seems to be compiling fine now. And I put in more comments in the
code.

https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.cc (right):

https://codereview.chromium.org/1038453002/diff/570001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.cc:46: // chrome_events_win.h
On 2015/04/14 02:25:09, Nico (away until Wed Apr 15) wrote:
> I can't see a file chrome_events_win.h, and I also don't really see macros in
> this CL – what does this refer to?

chrome_events_win.h is generated from chrome_events_win.rc using
mc.exe and contains a bunch of macros to interact with ETW (mainly
un/registering and writing events).

Lines 28 to 74 are needed to avoid breaking Windows XP, as these
functions do not exist in XP. The comment right above details this.

https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/570001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:138:
'<(SHARED_INTERMEDIATE_DIR)/base/trace_event/etw_manifest/chrome_events_win.rc',
On 2015/04/14 02:31:58, Nico (away until Wed Apr 15) wrote:
> Shouldn't this be in base/base.gyp? As-is, the clang/win bot reports
> 
> base.trace_event_etw_export_win.obj :error LNK2019: unresolved external symbol
> ChromeEnableBits referenced in function "public: static void __cdecl
> base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char const
*,char
> const *,unsigned __int64,int,char const * *,unsigned char const *,unsigned
> __int64 const *,class scoped_refptr<class
> base::trace_event::ConvertableToTraceFormat> const *)"
>
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPEBEPEBD_KHPEAPEBD0PEB_KPEBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)
> 
> and I'm guessing that symbol comes from that .rc file.
> 
> (and cl.exe happens to inline differently)

Yes, I added it back. I had it in base too at first, but had issues compiling
in shared mode. Turns out the culprit was process_outputs_as_sources in
etw_manifest.gyp which added the resource twice and failed on linking. Now that
this is removed, all bots seem to be compiling just fine.

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/BUILD/message_compiler.py (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
base/trace_event/etw_manifest/BUILD/message_compiler.py:6: # GYP build, which
can only run Python and not native binaries.
On 2015/04/15 08:10:35, Lei Zhang (slow to respond) wrote:
> message_compiler.py is only being referenced from
> base/trace_event/etw_manifest/BUILD.gn, and
> base/trace_event/etw_manifest/etw_manifest.gyp calls mc.exe directly. Did the
> original script you copied from mean to say GN?

Fixed.

This is the same script as remoting/tools/build/message_compiler.py
which has the same error.

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
File base/trace_event/etw_manifest/etw_manifest.gyp (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/etw_m...
base/trace_event/etw_manifest/etw_manifest.gyp:29: '-r', '<(man_output_dir)/.',
On 2015/04/15 08:10:35, Lei Zhang (slow to respond) wrote:
> why do you need the extra /. at the end?

It's needed because of an apparent bug in the mc.exe, otherwise
it fails. I tried removing it, no go unfortunately.

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.h (right):

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/04/15 08:10:36, Lei Zhang (slow to respond) wrote:
> nit: no (c)

Done (same in .cc too).

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:32: static void AddEvent(
On 2015/04/15 08:10:36, Lei Zhang (slow to respond) wrote:
> The above methods seem more obvious, but AddEvent() and AddCustomEvent() can
use
> some documentation.

I added documentation for all functions. Should be clearer now.

https://codereview.chromium.org/1038453002/diff/630001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:43: static void
AddCustomEvent(const char* name,
On 2015/04/15 08:10:36, Lei Zhang (slow to respond) wrote:
> This doesn't seem to have any callers?

Not yet, this is used to add an event that is only exported to ETW (and not
to the internal tracer). Eventually, I'm planning on having multiple
providers, but for now, it shares the same one as the internal events (they
all appear on the same series when visualized).

https://codereview.chromium.org/1038453002/diff/630001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1038453002/diff/630001/chrome/chrome_dll.gypi...
chrome/chrome_dll.gypi:138:
'<(SHARED_INTERMEDIATE_DIR)/base/trace_event/etw_manifest/chrome_events_win.rc',
On 2015/04/15 08:10:36, Lei Zhang (slow to respond) wrote:
> Based on your content/ changes, it looks like the new code is being used in
the
> renderer process... so do you need to add this to chrome_child.dll as well?

Yes, the code is also being used by renderers. However, this is only needed in
one
dll/exe, as it's used by the external tools to decode the messages. We chose to
put it in Chrome.dll. Having it in an other executable would be redundant.

[Nico, 2015-04-15 17:02:31]

Thanks!

I patched this in and built with clang locally, and it still fails though:

[24/24] LINK_EMBED check_example.exe
FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool
link-with-manifests environment.x86 True check_example.exe
"d:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
environment.x86 False link.exe /nologo /OUT:check_example.exe
@check_example.exe.rsp" 1 mt.exe rc.exe
"obj\base\check_example.check_example.exe.intermediate.manifest"
obj\base\check_example.check_example.exe.generated.manifest
..\..\build\win\compatibility.manifest
base.lib(base.trace_event_etw_export_win.obj) : error LNK2019: unresolved
external symbol _ChromeEnableBits referenced in function "public: static void
__cdecl base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char
const *,char const *,unsigned __int64,int,char const * *,unsigned char const
*,unsigned __int64 const *,class scoped_refptr<class
base::trace_event::ConvertableToTraceFormat> const *)"
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPBEPBD_KHPAPBD0PB_KPBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)
check_example.exe : fatal error LNK1120: 1 unresolved externals
Traceback (most recent call last):
  File "gyp-win-tool", line 313, in <module>
    sys.exit(main(sys.argv[1:]))
  File "gyp-win-tool", line 29, in main
    exit_code = executor.Dispatch(args)
  File "gyp-win-tool", line 71, in Dispatch
    return getattr(self, method)(*args[1:])
  File "gyp-win-tool", line 169, in ExecLinkWithManifests
    subprocess.check_call(ldcmd + add_to_ld)
  File "d:\src\depot_tools\python276_bin\lib\subprocess.py", line 540, in
check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command
'd:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
environment.x86 False link.exe /nologo /OUT:check_example.exe
@check_example.exe.rsp check_example.exe.manifest.res' returned non-zero exit
status 1120
ninja: build stopped: subcommand failed.

[Georges Khalil, 2015-04-15 17:37:32]

On 2015/04/15 17:02:31, Nico (away until Wed Apr 15) wrote:
> Thanks!
> 
> I patched this in and built with clang locally, and it still fails though:
> 
> [24/24] LINK_EMBED check_example.exe
> FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool
> link-with-manifests environment.x86 True check_example.exe
> "d:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
> environment.x86 False link.exe /nologo /OUT:check_example.exe
> @check_example.exe.rsp" 1 mt.exe rc.exe
> "obj\base\check_example.check_example.exe.intermediate.manifest"
> obj\base\check_example.check_example.exe.generated.manifest
> ..\..\build\win\compatibility.manifest
> base.lib(base.trace_event_etw_export_win.obj) : error LNK2019: unresolved
> external symbol _ChromeEnableBits referenced in function "public: static void
> __cdecl base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char
> const *,char const *,unsigned __int64,int,char const * *,unsigned char const
> *,unsigned __int64 const *,class scoped_refptr<class
> base::trace_event::ConvertableToTraceFormat> const *)"
>
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPBEPBD_KHPAPBD0PB_KPBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)
> check_example.exe : fatal error LNK1120: 1 unresolved externals
> Traceback (most recent call last):
>   File "gyp-win-tool", line 313, in <module>
>     sys.exit(main(sys.argv[1:]))
>   File "gyp-win-tool", line 29, in main
>     exit_code = executor.Dispatch(args)
>   File "gyp-win-tool", line 71, in Dispatch
>     return getattr(self, method)(*args[1:])
>   File "gyp-win-tool", line 169, in ExecLinkWithManifests
>     subprocess.check_call(ldcmd + add_to_ld)
>   File "d:\src\depot_tools\python276_bin\lib\subprocess.py", line 540, in
> check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command
> 'd:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
> environment.x86 False link.exe /nologo /OUT:check_example.exe
> @check_example.exe.rsp check_example.exe.manifest.res' returned non-zero exit
> status 1120
> ninja: build stopped: subcommand failed.

Got instructions from Nico on how to build using clang on Windows. Will try it
to hopefully fix the issue.

[Georges Khalil, 2015-04-15 20:27:41]

So it looks like the culprit is __declspec(selectany), which seems to not be
handled correctly by clang.


On 2015/04/15 17:37:32, Georges Khalil wrote:
> On 2015/04/15 17:02:31, Nico (away until Wed Apr 15) wrote:
> > Thanks!
> > 
> > I patched this in and built with clang locally, and it still fails though:
> > 
> > [24/24] LINK_EMBED check_example.exe
> > FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool
> > link-with-manifests environment.x86 True check_example.exe
> > "d:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
> > environment.x86 False link.exe /nologo /OUT:check_example.exe
> > @check_example.exe.rsp" 1 mt.exe rc.exe
> > "obj\base\check_example.check_example.exe.intermediate.manifest"
> > obj\base\check_example.check_example.exe.generated.manifest
> > ..\..\build\win\compatibility.manifest
> > base.lib(base.trace_event_etw_export_win.obj) : error LNK2019: unresolved
> > external symbol _ChromeEnableBits referenced in function "public: static
void
> > __cdecl base::trace_event::TraceEventETWExport::AddEvent(char,unsigned char
> > const *,char const *,unsigned __int64,int,char const * *,unsigned char const
> > *,unsigned __int64 const *,class scoped_refptr<class
> > base::trace_event::ConvertableToTraceFormat> const *)"
> >
>
(?AddEvent@TraceEventETWExport@trace_event@base@@SAXDPBEPBD_KHPAPBD0PB_KPBV?$scoped_refptr@VConvertableToTraceFormat@trace_event@base@@@@@Z)
> > check_example.exe : fatal error LNK1120: 1 unresolved externals
> > Traceback (most recent call last):
> >   File "gyp-win-tool", line 313, in <module>
> >     sys.exit(main(sys.argv[1:]))
> >   File "gyp-win-tool", line 29, in main
> >     exit_code = executor.Dispatch(args)
> >   File "gyp-win-tool", line 71, in Dispatch
> >     return getattr(self, method)(*args[1:])
> >   File "gyp-win-tool", line 169, in ExecLinkWithManifests
> >     subprocess.check_call(ldcmd + add_to_ld)
> >   File "d:\src\depot_tools\python276_bin\lib\subprocess.py", line 540, in
> > check_call
> >     raise CalledProcessError(retcode, cmd)
> > subprocess.CalledProcessError: Command
> > 'd:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
> > environment.x86 False link.exe /nologo /OUT:check_example.exe
> > @check_example.exe.rsp check_example.exe.manifest.res' returned non-zero
exit
> > status 1120
> > ninja: build stopped: subcommand failed.
> 
> Got instructions from Nico on how to build using clang on Windows. Will try it
> to hopefully fix the issue.

[Nico, 2015-04-15 20:40:42]

Yup, it's a clang bug. Filed https://llvm.org/bugs/show_bug.cgi?id=23242

Since that bot is on the fyi waterfall, it generally shouldn't block CLs – so go
ahead and land, and we'll hopefully be able to fix that quickly.

[Lei Zhang, 2015-04-16 08:11:00]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-04-16 08:11:02]

lgtm

https://codereview.chromium.org/1038453002/diff/650001/base/trace_event/trace...
File base/trace_event/trace_event_etw_export_win.h (right):

https://codereview.chromium.org/1038453002/diff/650001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:32: static bool
isETWExportEnabled() { return GetInstance()->ETWExportEnabled_; }
nit: Is...

https://codereview.chromium.org/1038453002/diff/650001/base/trace_event/trace...
base/trace_event/trace_event_etw_export_win.h:51: char const* phase,
why is this a char const?

[Lei Zhang, 2015-04-16 08:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
brucedawson@chromium.org, dsinclair@chromium.org, scottmg@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1038453002/#ps650001
(title: "thakis@ and thestig@ reviews.")

[commit-bot: I haz the power, 2015-04-16 08:11:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038453002/650001

[commit-bot: I haz the power, 2015-04-16 09:25:28]

Committed patchset #13 (id:650001)

[commit-bot: I haz the power, 2015-04-16 09:26:16]

Patchset 13 (id:??) landed as
https://crrev.com/28ad5e6b1100a7f0d25a5e6741f7241a86cf61bd
Cr-Commit-Position: refs/heads/master@{#325408}

[scottmg, 2015-04-17 20:57:30]

I think there's a missing dependency here, the generated file is missing and
failing compile sometimes, e.g.:

http://build.chromium.org/p/chromium/builders/Win/builds/31012/steps/compile/...

[Georges Khalil, 2015-04-17 21:01:05]

On 2015/04/17 20:57:30, scottmg wrote:
> I think there's a missing dependency here, the generated file is missing and
> failing compile sometimes, e.g.:
> 
>
http://build.chromium.org/p/chromium/builders/Win/builds/31012/steps/compile/...

Yes, there was a missing dependency in base_win64, this has been fixed in a CL
that has just
been committed:

https://codereview.chromium.org/1090223002/