Bug 77300 – [chromium] enable -Wexit-time-destructors for webkit unit tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Ami Fischman]

Pacify clang shared_library build by leaking a global static.

[Ami Fischman]

Created attachment 124465 [details]
Patch

[Ami Fischman]

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'

[Ami Fischman]

Created attachment 124473 [details]
Patch

[Ami Fischman]

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.

[Tony Chang]

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.

[Ami Fischman]

Created attachment 124564 [details]
Patch

[Ami Fischman]

(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.

[Tony Chang]

Comment on attachment 124564 [details]
Patch

The patch doesn't apply. I'll rebase to tip-of-tree and cq+ for you.

[Tony Chang]

(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.

[Tony Chang]

Created attachment 124570 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 124570 [details]
Patch for landing

Clearing flags on attachment: 124570

Committed r106259: <http://trac.webkit.org/changeset/106259>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

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).

[Levi Weintraub]

Re-opening as I rolled this out in r106313.

[Ami Fischman]

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

[Tony Chang]

(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.

[Ami Fischman]

(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?

[Tony Chang]

(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?

[Ami Fischman]

Created attachment 124835 [details]
Patch

[Ami Fischman]

(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.

[Tony Chang]

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.

[Levi Weintraub]

Throwing the current WebKit Sheriff on the thread.

[WebKit Review Bot]

Comment on attachment 124835 [details]
Patch

Clearing flags on attachment: 124835

Committed r106410: <http://trac.webkit.org/changeset/106410>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.