Bug 26997 - [Chromium] Remove buggy usage of DEFINE_STATIC_LOCAL in V8Binding.cpp
Summary: [Chromium] Remove buggy usage of DEFINE_STATIC_LOCAL in V8Binding.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-06 13:28 PDT by Nate Chapin
Modified: 2009-07-10 13:30 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.48 KB, patch)
2009-07-06 13:31 PDT, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-07-06 13:28:48 PDT
....this usage of DEFINE_STATIC_LOCAL is causing a bunch of layout test failures for Chromium.
Comment 1 Nate Chapin 2009-07-06 13:31:44 PDT
Created attachment 32313 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-07-06 13:46:56 PDT
Comment on attachment 32313 [details]
patch

ok.
Comment 3 Eric Seidel (no email) 2009-07-06 15:34:01 PDT
Why is this bad usage?
Comment 4 Eric Seidel (no email) 2009-07-06 15:34:24 PDT
CCing Darin who knows more about this macro than I do.
Comment 5 Nate Chapin 2009-07-06 15:39:34 PDT
To be honest, I'm not 100% sure what I did wrong here.  All I know is that I clearly didn't initialize it correctly, because the test shell was consistently crashing when lowNumbers[foo] was used a couple lines below the DEFINE_STATIC_LOCAL().

I probably should have included a FIXME to use DEFINE_STATIC_LOCAL properly instead of the traditional static variable declaration.  I just wanted to get it working so that we could finish upstreaming V8Binding and get the chromium canary builder green again.  

However, it's looking like I'm going to be rolling this change and the one that introduced the bug back due to the chromium build mess and the fact that I won't be able to clean up after myself tonight.  If someone can help me figure out how to correctly use DEFINE_STATIC_LOCAL in this situation, I'll happily do it right when I re-upstream V8Binding.cpp tomorrow.
Comment 6 Eric Seidel (no email) 2009-07-06 15:51:28 PDT
From StdLibExtras.h:

// Use these to declare and define a static local variable (static T;) so that
//  it is leaked so that its destructors are not called at exit. Using this
//  macro also allows workarounds a compiler bug present in Apple's version of GCC 4.0.1.
#if COMPILER(GCC) && defined(__APPLE_CC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 0 && __GNUC_PATCHLEVEL__ == 1
#define DEFINE_STATIC_LOCAL(type, name, arguments) \
    static type* name##Ptr = new type arguments; \
    type& name = *name##Ptr
#else
#define DEFINE_STATIC_LOCAL(type, name, arguments) \
    static type& name = *new type arguments
#endif

These exist only to work around bugs in GCC 4.0.1 on Macs.  I don't know enough about the bug to comment further however.
Comment 7 Dimitri Glazkov (Google) 2009-07-10 13:22:29 PDT
Has this landed?
Comment 8 Nate Chapin 2009-07-10 13:30:25 PDT
This has landed, but I left it open since I wasn't sure whether the conversation was going to continue regarding my original usage of DEFINE_STATIC_LOCAL.  The current version of the file in the repository does not use DEFINE_STATIC_LOCAL.  If it should, I'm happy to take care of it, but I guess I'll close this issue out in the meantime.