Remove 1 exit time destructor from ppapi, and possibly fix a bug.

Remove 1 exit time destructor from ppapi, and possibly fix a bug.

Previously, this was an inline function with a static member, and
since we're building with -fvisibility-inlines-hidden, this resulted
in one copy of the singleton in each translation unit that includes
this header.

Turn on -Wexit-time-destructors for ppapi_proxy.

BUG=104323
TEST=none
TBR=noelallen@google.com (noelallen@chromium.org did LGTM already)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110339

[Nico, 2011-11-16 01:13:23]

The change to the gyp file is wrong.

I can't figure out how to build the file I'm changing, "make
out/Release/obj.target/nacl_ppapi_browser/ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.o"
fails complaining about not finding the .h include at the top :-/

[bradn, 2011-11-16 01:28:38]

Hmmn, not sure why its not working.
It work on other modules?

Otherwise LGTM

But Please get Noel's sign off, as the proxy does magical things in general and
I wanna make sure this isn't intentional for some crazy reason.

[noelallen1, 2011-11-16 18:22:40]

No that's too crazy even for us.  :0
Not sure about singleton race, otherwise LGTM.

http://codereview.chromium.org/8574027/diff/2001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.cc
(right):

http://codereview.chromium.org/8574027/diff/2001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.cc:22:
CR_DEFINE_STATIC_LOCAL(PluginResourceTracker, tracker, ());
Does this need to be some form of LazyInstance?  Is thre resource tracker racy
at startup?

[Nico, 2011-11-16 18:25:57]

I guess I just won't check in the gyp change then.

Thanks!

http://codereview.chromium.org/8574027/diff/2001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.cc
(right):

http://codereview.chromium.org/8574027/diff/2001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.cc:22:
CR_DEFINE_STATIC_LOCAL(PluginResourceTracker, tracker, ());
On 2011/11/16 18:22:40, noelallen1 wrote:
> Does this need to be some form of LazyInstance?  Is thre resource tracker racy
> at startup?

No; it's the same as a normal static except that it's allocated with new so that
no destructor is needed:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/basictypes.h&exa...

[commit-bot: I haz the power, 2011-11-16 18:27:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8574027/4002

[commit-bot: I haz the power, 2011-11-16 18:27:27]

Presubmit check for 8574027-4002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.cc,ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.h

[commit-bot: I haz the power, 2011-11-16 18:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8574027/4002

[noelallen_use_chromium, 2011-11-16 19:29:44]

On 2011/11/16 01:13:23, Nico wrote:
> The change to the gyp file is wrong.
> 
> I can't figure out how to build the file I'm changing, "make
>
out/Release/obj.target/nacl_ppapi_browser/ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.o"
> fails complaining about not finding the .h include at the top :-/
I don't see how this would affect your ability to compile.  Something else must
be wrong, or GYP is just broken.
    'target_conditions': [
      ['enable_wexit_time_destructors==1', {
        'conditions': [
          [ 'clang==1', {
            'cflags': [
              '-Wexit-time-destructors',
            ],
            'xcode_settings': {
              'WARNING_CFLAGS': [
                '-Wexit-time-destructors',
              ],
            },
          }],
        ],
      }],

[Nico, 2011-11-16 19:32:26]

On 2011/11/16 19:29:44, noelallen wrote:
> On 2011/11/16 01:13:23, Nico wrote:
> > The change to the gyp file is wrong.
> > 
> > I can't figure out how to build the file I'm changing, "make
> >
>
out/Release/obj.target/nacl_ppapi_browser/ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.o"
> > fails complaining about not finding the .h include at the top :-/

That was poorly worded on my part: I can't figure out how to compile that file,
with or without my gyp change.

I'm saying that my gyp change is wrong because it turns on that warning for a
target that doesn't contain the file I'm changing in this CL. But since I can't
figure out how to build it, I can't really add the warning to the right gyp
file.

[noelallen_use_chromium, 2011-11-16 19:46:55]

On 2011/11/16 19:32:26, Nico wrote:
> On 2011/11/16 19:29:44, noelallen wrote:
> > On 2011/11/16 01:13:23, Nico wrote:
> > > The change to the gyp file is wrong.
> > > 
> > > I can't figure out how to build the file I'm changing, "make
> > >
> >
>
out/Release/obj.target/nacl_ppapi_browser/ppapi/native_client/src/shared/ppapi_proxy/plugin_resource_tracker.o"
> > > fails complaining about not finding the .h include at the top :-/
> 
> That was poorly worded on my part: I can't figure out how to compile that
file,
> with or without my gyp change.
> 
> I'm saying that my gyp change is wrong because it turns on that warning for a
> target that doesn't contain the file I'm changing in this CL. But since I
can't
> figure out how to build it, I can't really add the warning to the right gyp
> file.

Ah...  plugin_resource_tracker.cc is part of the untrusted plugin, so it's part
of the IRT.  The target you want is:
   ppruntime_lib

That will fire off the nacl-gcc compiler as part of an action which builds the
whole library.  You can't target the .o directly.

Which also means that the compiler will not get "-Wexit-time-destructors".

[commit-bot: I haz the power, 2011-11-16 19:50:24]

Change committed as 110339