add TRACE_EVENT support to ANGLE

add TRACE_EVENT support to ANGLE

TRACE_EVENT header from Chromium/WebKit, with support functions to allow
plumbing back to an embedder.

Also adds some TRACE_EVENTs to startup flow.

R=apatrick@google.com, shannonwoods@google.com
BUG=270179

[scottmg, 2013-08-09 20:28:37]

+shannonwoods

[shannonwoods1, 2013-08-09 20:55:18]

In addition to the inline comments-- this'll break the build for anyone using
the build process described on our wiki. Can you update the Visual Studio
project files? (If not, one of us can help out with that.)

Since the event_tracer/trace_event files are from Chrome and use that project's
style instead of ours, it'd be better to include them under third_party, I
think. And should the copyright boilerplate be ANGLE's, or the original?

https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
File src/libGLESv2/renderer/Renderer9.cpp (right):

https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
src/libGLESv2/renderer/Renderer9.cpp:237: { TRACE_EVENT0("gpu",
"GetDeviceCaps");
I'd prefer that either the braces for the added scope sections follow our
bracing rules (nothing sharing the line with the brace, everything within them
indented), or the contents of these braces be factored out into helper
functions, so the trace can be done there without adding extraneous braces.

[shannonwoods1, 2013-08-09 20:56:12]

Also-- in src/common/version.h, the build revision will need to be updated
before checkin.

[scottmg, 2013-08-09 21:38:12]

On 2013/08/09 20:55:18, shannonwoods1 wrote:
> In addition to the inline comments-- this'll break the build for anyone using
> the build process described on our wiki. Can you update the Visual Studio
> project files? (If not, one of us can help out with that.)

Done. (I think?)

> 
> Since the event_tracer/trace_event files are from Chrome and use that
project's
> style instead of ours, it'd be better to include them under third_party, I
> think. And should the copyright boilerplate be ANGLE's, or the original?

Moved trace_event.h to third_party/trace_event/ and kept Chromium copyright
header. event_tracer.cpp/h are the interface to that code that was written for
ANGLE so kept that in common/. Seem OK?

> 
>
https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
> File src/libGLESv2/renderer/Renderer9.cpp (right):
> 
>
https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
> src/libGLESv2/renderer/Renderer9.cpp:237: { TRACE_EVENT0("gpu",
> "GetDeviceCaps");
> I'd prefer that either the braces for the added scope sections follow our
> bracing rules (nothing sharing the line with the brace, everything within them
> indented), or the contents of these braces be factored out into helper
> functions, so the trace can be done there without adding extraneous braces.

[scottmg, 2013-08-09 21:38:25]

On 2013/08/09 20:56:12, shannonwoods1 wrote:
> Also-- in src/common/version.h, the build revision will need to be updated
> before checkin.

Just bumped BUILD_REVISION, is that right?

[scottmg, 2013-08-09 21:38:44]

https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
File src/libGLESv2/renderer/Renderer9.cpp (right):

https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Rendere...
src/libGLESv2/renderer/Renderer9.cpp:237: { TRACE_EVENT0("gpu",
"GetDeviceCaps");
On 2013/08/09 20:55:18, shannonwoods1 wrote:
> I'd prefer that either the braces for the added scope sections follow our
> bracing rules (nothing sharing the line with the brace, everything within them
> indented), or the contents of these braces be factored out into helper
> functions, so the trace can be done there without adding extraneous braces.

Done.

[shannonwoods1, 2013-08-09 22:05:10]

On 2013/08/09 21:38:12, scottmg wrote:
> On 2013/08/09 20:55:18, shannonwoods1 wrote:
> > In addition to the inline comments-- this'll break the build for anyone
using
> > the build process described on our wiki. Can you update the Visual Studio
> > project files? (If not, one of us can help out with that.)
> 
> Done. (I think?)

Looks like Visual Studio can correctly find the new files. The filters aren't
quite right-- trace_event.h shows up in "Header Files" instead of "Third Party",
but if that's a pain to fix we can straighten it out; it doesn't break the
build.

However, there is a build problem -- Visual Studio complains about the body of a
__declspec(dllimport) function being defined when it's expected to be imported.
(Which I see during the build because ANGLE_IMPLEMENTATION isn't defined for the
VS build.)

> 
> Moved trace_event.h to third_party/trace_event/ and kept Chromium copyright
> header. event_tracer.cpp/h are the interface to that code that was written for
> ANGLE so kept that in common/. Seem OK?
>

Anything written just for ANGLE should use the ANGLE coding style:
https://code.google.com/p/angleproject/wiki/CodingStandard
I think you've generally followed the ANGLE standard aside from using underscore
separation instead of CamelCase, and one stray K&R brace in event_tracer.cpp at
line 32. The only other nit I have style-wise is that ANGLE generally starts the
parameter list on the same line as the function definition, doesn't separate
them one-per-line, and when using line breaks indents to the level of the open
paren. If those items are straightened out, then the location of everything is
cool by me.

[shannonwoods1, 2013-08-09 22:14:07]

On 2013/08/09 21:38:25, scottmg wrote:
> On 2013/08/09 20:56:12, shannonwoods1 wrote:
> > Also-- in src/common/version.h, the build revision will need to be updated
> > before checkin.
> 
> Just bumped BUILD_REVISION, is that right?

Oh-- yes, that's exactly right.

[scottmg, 2013-08-09 22:28:49]

Thanks!

On 2013/08/09 22:05:10, shannonwoods1 wrote:
> On 2013/08/09 21:38:12, scottmg wrote:
> > On 2013/08/09 20:55:18, shannonwoods1 wrote:
> > > In addition to the inline comments-- this'll break the build for anyone
> using
> > > the build process described on our wiki. Can you update the Visual Studio
> > > project files? (If not, one of us can help out with that.)
> > 
> > Done. (I think?)
> 
> Looks like Visual Studio can correctly find the new files. The filters aren't
> quite right-- trace_event.h shows up in "Header Files" instead of "Third
Party",
> but if that's a pain to fix we can straighten it out; it doesn't break the
> build.
> 
> However, there is a build problem -- Visual Studio complains about the body of
a
> __declspec(dllimport) function being defined when it's expected to be
imported.
> (Which I see during the build because ANGLE_IMPLEMENTATION isn't defined for
the
> VS build.)

Both fixed.

> 
> > 
> > Moved trace_event.h to third_party/trace_event/ and kept Chromium copyright
> > header. event_tracer.cpp/h are the interface to that code that was written
for
> > ANGLE so kept that in common/. Seem OK?
> >
> 
> Anything written just for ANGLE should use the ANGLE coding style:
> https://code.google.com/p/angleproject/wiki/CodingStandard
> I think you've generally followed the ANGLE standard aside from using
underscore
> separation instead of CamelCase, and one stray K&R brace in event_tracer.cpp
at
> line 32. The only other nit I have style-wise is that ANGLE generally starts
the
> parameter list on the same line as the function definition, doesn't separate
> them one-per-line, and when using line breaks indents to the level of the open
> paren. If those items are straightened out, then the location of everything is
> cool by me.

Thanks for your patience. I think that's all tidied up now (or I have
style-blindness at least :).

[shannonwoods1, 2013-08-09 22:51:24]

On 2013/08/09 22:28:49, scottmg wrote:
> Thanks!
> 
> On 2013/08/09 22:05:10, shannonwoods1 wrote:
> > On 2013/08/09 21:38:12, scottmg wrote:
> > > On 2013/08/09 20:55:18, shannonwoods1 wrote:
> > > > In addition to the inline comments-- this'll break the build for anyone
> > using
> > > > the build process described on our wiki. Can you update the Visual
Studio
> > > > project files? (If not, one of us can help out with that.)
> > > 
> > > Done. (I think?)
> > 
> > Looks like Visual Studio can correctly find the new files. The filters
aren't
> > quite right-- trace_event.h shows up in "Header Files" instead of "Third
> Party",
> > but if that's a pain to fix we can straighten it out; it doesn't break the
> > build.
> > 
> > However, there is a build problem -- Visual Studio complains about the body
of
> a
> > __declspec(dllimport) function being defined when it's expected to be
> imported.
> > (Which I see during the build because ANGLE_IMPLEMENTATION isn't defined for
> the
> > VS build.)
> 
> Both fixed.
> 
> > 
> > > 
> > > Moved trace_event.h to third_party/trace_event/ and kept Chromium
copyright
> > > header. event_tracer.cpp/h are the interface to that code that was written
> for
> > > ANGLE so kept that in common/. Seem OK?
> > >
> > 
> > Anything written just for ANGLE should use the ANGLE coding style:
> > https://code.google.com/p/angleproject/wiki/CodingStandard
> > I think you've generally followed the ANGLE standard aside from using
> underscore
> > separation instead of CamelCase, and one stray K&R brace in event_tracer.cpp
> at
> > line 32. The only other nit I have style-wise is that ANGLE generally starts
> the
> > parameter list on the same line as the function definition, doesn't separate
> > them one-per-line, and when using line breaks indents to the level of the
open
> > paren. If those items are straightened out, then the location of everything
is
> > cool by me.
> 
> Thanks for your patience. I think that's all tidied up now (or I have
> style-blindness at least :).

Style looks great, thank you-- I know it's not fun to have to deal with
different standards.

Everything looks good and builds fine in Visual Studio. Thanks! My only
remaining question there is-- what's the intended behavior when
ANGLE_IMPLEMENTATION is undefined/what's the use case there? It seems like it
would cause a warning or error whenever undefined, because the definition of the
function isn't inside an #ifdef guard, so it'd be defining a function declared
as an import. But-- I don't really know how other compilers will handle it.
Would one leave ANGLE_IMPLEMENTATION undefined if they were importing the
definition of the function from elsewhere? If so, would the dual definition of
the function be an issue?

[scottmg, 2013-08-09 22:59:05]

On 2013/08/09 22:51:24, shannonwoods1 wrote:
> On 2013/08/09 22:28:49, scottmg wrote:
> > Thanks!
> > 
> > On 2013/08/09 22:05:10, shannonwoods1 wrote:
> > > On 2013/08/09 21:38:12, scottmg wrote:
> > > > On 2013/08/09 20:55:18, shannonwoods1 wrote:
> > > > > In addition to the inline comments-- this'll break the build for
anyone
> > > using
> > > > > the build process described on our wiki. Can you update the Visual
> Studio
> > > > > project files? (If not, one of us can help out with that.)
> > > > 
> > > > Done. (I think?)
> > > 
> > > Looks like Visual Studio can correctly find the new files. The filters
> aren't
> > > quite right-- trace_event.h shows up in "Header Files" instead of "Third
> > Party",
> > > but if that's a pain to fix we can straighten it out; it doesn't break the
> > > build.
> > > 
> > > However, there is a build problem -- Visual Studio complains about the
body
> of
> > a
> > > __declspec(dllimport) function being defined when it's expected to be
> > imported.
> > > (Which I see during the build because ANGLE_IMPLEMENTATION isn't defined
for
> > the
> > > VS build.)
> > 
> > Both fixed.
> > 
> > > 
> > > > 
> > > > Moved trace_event.h to third_party/trace_event/ and kept Chromium
> copyright
> > > > header. event_tracer.cpp/h are the interface to that code that was
written
> > for
> > > > ANGLE so kept that in common/. Seem OK?
> > > >
> > > 
> > > Anything written just for ANGLE should use the ANGLE coding style:
> > > https://code.google.com/p/angleproject/wiki/CodingStandard
> > > I think you've generally followed the ANGLE standard aside from using
> > underscore
> > > separation instead of CamelCase, and one stray K&R brace in
event_tracer.cpp
> > at
> > > line 32. The only other nit I have style-wise is that ANGLE generally
starts
> > the
> > > parameter list on the same line as the function definition, doesn't
separate
> > > them one-per-line, and when using line breaks indents to the level of the
> open
> > > paren. If those items are straightened out, then the location of
everything
> is
> > > cool by me.
> > 
> > Thanks for your patience. I think that's all tidied up now (or I have
> > style-blindness at least :).
> 
> Style looks great, thank you-- I know it's not fun to have to deal with
> different standards.
> 
> Everything looks good and builds fine in Visual Studio. Thanks! My only
> remaining question there is-- what's the intended behavior when
> ANGLE_IMPLEMENTATION is undefined/what's the use case there? It seems like it
> would cause a warning or error whenever undefined, because the definition of
the
> function isn't inside an #ifdef guard, so it'd be defining a function declared
> as an import. But-- I don't really know how other compilers will handle it.
> Would one leave ANGLE_IMPLEMENTATION undefined if they were importing the
> definition of the function from elsewhere? If so, would the dual definition of
> the function be an issue?

The goal (not sure if achieved or not, but works for me locally) is to have
libGLESv2.dll declare that function with __declspec(dllexport), and an external
includer of that header declare it as __declspec(dllimport), to let it be
referenced from a different dll. This is to use it in component build mode from
the gpu dll in chromium, as set up here
https://codereview.chromium.org/22744005/.

[shannonwoods1, 2013-08-09 23:02:47]

> The goal (not sure if achieved or not, but works for me locally) is to have
> libGLESv2.dll declare that function with __declspec(dllexport), and an
external
> includer of that header declare it as __declspec(dllimport), to let it be
> referenced from a different dll. This is to use it in component build mode
from
> the gpu dll in chromium, as set up here
> https://codereview.chromium.org/22744005/.

Ah, ok, got it! LGTM, and thank you.

[scottmg, 2013-08-09 23:07:52]

On 2013/08/09 23:02:47, shannonwoods1 wrote:
> > The goal (not sure if achieved or not, but works for me locally) is to have
> > libGLESv2.dll declare that function with __declspec(dllexport), and an
> external
> > includer of that header declare it as __declspec(dllimport), to let it be
> > referenced from a different dll. This is to use it in component build mode
> from
> > the gpu dll in chromium, as set up here
> > https://codereview.chromium.org/22744005/.
> 
> Ah, ok, got it! LGTM, and thank you.

Thanks! Could I bug one of you to land this?

[apatrick1, 2013-08-12 19:32:05]

LGTM. I'll land this if it's not already underway?

[shannonwoods1, 2013-08-12 19:38:20]

On 2013/08/12 19:32:05, apatrick1 wrote:
> LGTM. I'll land this if it's not already underway?

Landing now, if the server ever gets around to responding.

[scottmg, 2013-08-12 21:43:16]

https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp
File src/common/event_tracer.cpp (right):

https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cp...
src/common/event_tracer.cpp:26: return 0;
Sorry, it seems 0 is the wrong default here. The tracing macros want a pointer
to an unsigned char containing 0, not NULL themselves.

I've uploaded a fix here: https://codereview.appspot.com/12806043/

Or feel free to revert if you prefer. Sorry for the hassle.

[scottmg, 2013-08-13 16:43:29]

On 2013/08/12 21:43:16, scottmg wrote:
> https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp
> File src/common/event_tracer.cpp (right):
> 
>
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cp...
> src/common/event_tracer.cpp:26: return 0;
> Sorry, it seems 0 is the wrong default here. The tracing macros want a pointer
> to an unsigned char containing 0, not NULL themselves.
> 
> I've uploaded a fix here: https://codereview.appspot.com/12806043/
> 
> Or feel free to revert if you prefer. Sorry for the hassle.

Hmm, actually, semi-conveniently, it looks like the file adds got missed in
landing this CL originally
https://code.google.com/p/angleproject/source/detail?r=889f9d7185fdceaa72b5e4...

So at least there's no crash checked in! :) (the project files are broken at
HEAD though).

I also need to change the dll import/export setup for
https://codereview.chromium.org/22744005/ to work, so maybe I'll pull the file
adds, the crash fix and the dll stuff into single fixup CL to make things less
confusing.

[shannonwoods1, 2013-08-13 17:42:03]

On 2013/08/13 16:43:29, scottmg wrote:
> On 2013/08/12 21:43:16, scottmg wrote:
> >
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp
> > File src/common/event_tracer.cpp (right):
> > 
> >
>
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cp...
> > src/common/event_tracer.cpp:26: return 0;
> > Sorry, it seems 0 is the wrong default here. The tracing macros want a
pointer
> > to an unsigned char containing 0, not NULL themselves.
> > 
> > I've uploaded a fix here: https://codereview.appspot.com/12806043/
> > 
> > Or feel free to revert if you prefer. Sorry for the hassle.
> 
> Hmm, actually, semi-conveniently, it looks like the file adds got missed in
> landing this CL originally
>
https://code.google.com/p/angleproject/source/detail?r=889f9d7185fdceaa72b5e4...
> 
> So at least there's no crash checked in! :) (the project files are broken at
> HEAD though).
> 
> I also need to change the dll import/export setup for
> https://codereview.chromium.org/22744005/ to work, so maybe I'll pull the file
> adds, the crash fix and the dll stuff into single fixup CL to make things less
> confusing.

Ah-- I think the file adds must have gone missing somewhere during my struggle
with cygwin's endline behavior. Apologies.