Bug 77300 - [chromium] enable -Wexit-time-destructors for webkit unit tests
Summary: [chromium] enable -Wexit-time-destructors for webkit unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ami Fischman
URL:
Keywords:
Depends on: 77395
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-29 10:35 PST by Ami Fischman
Modified: 2012-01-31 15:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.38 KB, patch)
2012-01-29 10:36 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2012-01-29 15:26 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (2.27 KB, patch)
2012-01-30 09:51 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch for landing (1.29 KB, patch)
2012-01-30 10:41 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2012-01-31 15:34 PST, Ami Fischman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-01-29 10:35:55 PST
Pacify clang shared_library build by leaking a global static.
Comment 1 Ami Fischman 2012-01-29 10:36:38 PST
Created attachment 124465 [details]
Patch
Comment 2 Ami Fischman 2012-01-29 10:39:47 PST
FTR, this change enables me to build targets: 
chrome test_shell_tests webkit_unit_tests DumpRenderTree

with set|grep ^GYP:
GYP_DEFINES='disable_nacl=1 enable_svg=0 proprietary_codecs=1 ffmpeg_branding=ChromeOS component=shared_library clang=1 clang_use_chrome_plugins=1'
GYP_GENERATORS=ninja
GYP_GENERATOR_FLAGS='output_dir=ninja config=Debug'
Comment 3 Ami Fischman 2012-01-29 15:26:30 PST
Created attachment 124473 [details]
Patch
Comment 4 Ami Fischman 2012-01-29 15:28:22 PST
Nico pointed out new usages of exit-time dtors in tests will break the clang/shared build, which will likely be invisible to non-shared/non-clang builders.  Updated patch includes the exit-time-dtors warnings for the webkit_unit_tests target for even non-shared builds, since it's clean now this should help keep it clean.
Comment 5 Tony Chang 2012-01-30 09:45:20 PST
Comment on attachment 124473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124473&action=review

> Source/WebKit/chromium/tests/TextureManagerTest.cpp:42
> -FakeTextureAllocator fakeTextureAllocator;
> +DEFINE_STATIC_LOCAL(FakeTextureAllocator, fakeTextureAllocator, ());

Can we move this into the requestTexture function?  That appears to be the only place it's used.
Comment 6 Ami Fischman 2012-01-30 09:51:22 PST
Created attachment 124564 [details]
Patch
Comment 7 Ami Fischman 2012-01-30 09:51:47 PST
(In reply to comment #5)
> (From update of attachment 124473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124473&action=review
> 
> > Source/WebKit/chromium/tests/TextureManagerTest.cpp:42
> > -FakeTextureAllocator fakeTextureAllocator;
> > +DEFINE_STATIC_LOCAL(FakeTextureAllocator, fakeTextureAllocator, ());
> 
> Can we move this into the requestTexture function?  That appears to be the only place it's used.

Done.
Comment 8 Tony Chang 2012-01-30 10:36:01 PST
Comment on attachment 124564 [details]
Patch

The patch doesn't apply. I'll rebase to tip-of-tree and cq+ for you.
Comment 9 Tony Chang 2012-01-30 10:38:25 PST
(In reply to comment #8)
> (From update of attachment 124564 [details])
> The patch doesn't apply. I'll rebase to tip-of-tree and cq+ for you.

Looks like it was fixed in https://bugs.webkit.org/show_bug.cgi?id=77333.  I'll just land the gyp change.
Comment 10 Tony Chang 2012-01-30 10:41:48 PST
Created attachment 124570 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-01-30 11:07:14 PST
Comment on attachment 124570 [details]
Patch for landing

Clearing flags on attachment: 124570

Committed r106259: <http://trac.webkit.org/changeset/106259>
Comment 12 WebKit Review Bot 2012-01-30 11:07:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Levi Weintraub 2012-01-30 18:10:34 PST
I'm afraid this breaks the build when we include webkittests as we do in the shared builders. See build output here: http://build.chromium.org/p/chromium/builders/Linux%20Clang%20%28dbg%29/builds/19648/steps/compile/logs/stdio

I think this should be rolled back and re-opened pending a fix to webkittests or the build config (so WebKit isn't dependent on webkittests).
Comment 14 Levi Weintraub 2012-01-30 18:25:14 PST
Re-opening as I rolled this out in r106313.
Comment 15 Ami Fischman 2012-01-30 20:20:43 PST
How strange.  The gyp target webkit_unit_tests built fine for me, which is why this seemed like a safe change.

Tony/Nico/Levi: any ideas why 
find ninja/Debug/ -iname '*.ninja'|xargs grep FrameTestHelpers
finds nothing in my chromium checkout?  I.e. FrameTestHelpers.cpp doesn't seem to be referenced by any .ninja files, even though it is in webkit_unittest_files in third_party/WebKit/Source/WebKit/chromium/WebKit.gypi
Comment 16 Tony Chang 2012-01-31 12:17:15 PST
(In reply to comment #15)
> How strange.  The gyp target webkit_unit_tests built fine for me, which is why this seemed like a safe change.
> 
> Tony/Nico/Levi: any ideas why 
> find ninja/Debug/ -iname '*.ninja'|xargs grep FrameTestHelpers

According to WebKit.gyp, we don't compile FrameTestHelpers.cpp when using a components build.  This warning is only triggered when using clang + static build.
Comment 17 Ami Fischman 2012-01-31 12:21:38 PST
(In reply to comment #16)
> (In reply to comment #15)
> > How strange.  The gyp target webkit_unit_tests built fine for me, which is why this seemed like a safe change.
> > 
> > Tony/Nico/Levi: any ideas why 
> > find ninja/Debug/ -iname '*.ninja'|xargs grep FrameTestHelpers
> 
> According to WebKit.gyp, we don't compile FrameTestHelpers.cpp when using a components build.  This warning is only triggered when using clang + static build.

But that's only for the "webkit" target, right?
Why is FTH.cpp not included in the "webkit_unit_tests" target?
Comment 18 Tony Chang 2012-01-31 13:22:33 PST
(In reply to comment #17)
> But that's only for the "webkit" target, right?
> Why is FTH.cpp not included in the "webkit_unit_tests" target?

In WebKitUnitTests.gyp, it says:

                ['inside_chromium_build==1 and component=="shared_library"', {
                    'defines': [
                        'WEBKIT_DLL_UNITTEST',
                    ],
                }, {  
                    'sources': [
                        '<@(webkit_unittest_files)',
                    ],

FrameTestHelpers.cpp will be compiled if you're not building a shared library.

Does the clang error repro for you if you disable the shared build?
Comment 19 Ami Fischman 2012-01-31 15:34:48 PST
Created attachment 124835 [details]
Patch
Comment 20 Ami Fischman 2012-01-31 15:35:55 PST
(In reply to comment #18)
> Does the clang error repro for you if you disable the shared build?

D'oh!  I was looking at a version of the file in which I was trying to rip out the craxy that is the webkit-target-includes-test-files-but-only-in-shared-builds and forgot ToT still has the conditional.

Verified that the latest patch builds in static build.
Comment 21 Tony Chang 2012-01-31 15:39:04 PST
Comment on attachment 124835 [details]
Patch

FYI: Levi here's another attempt at enabling -Wexit-time-destructors.  It probably isn't tested by any of the canary bots so if this fails on the Chromium waterfall, feel free to revert.
Comment 22 Levi Weintraub 2012-01-31 15:49:32 PST
Throwing the current WebKit Sheriff on the thread.
Comment 23 WebKit Review Bot 2012-01-31 15:55:54 PST
Comment on attachment 124835 [details]
Patch

Clearing flags on attachment: 124835

Committed r106410: <http://trac.webkit.org/changeset/106410>
Comment 24 WebKit Review Bot 2012-01-31 15:56:01 PST
All reviewed patches have been landed.  Closing bug.