[Webkit-unassigned] [Bug 53747] Add some dynamic annotations to JavaScriptCore/wtf

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53747





--- Comment #41 from Alexey Proskuryakov <ap at webkit.org>  2011-03-18 15:49:04 PST ---
(From update of attachment 85441)
View in context: https://bugs.webkit.org/attachment.cgi?id=85441&action=review

Seems fine to me to have this. I hesitate to say r+ because I suspect that this breaks Mac build.

> Source/JavaScriptCore/ChangeLog:19
> +        * wtf/DynamicAnnotations.c: Added.

Is there a reason for this to be a plain C file? Most people working on WebKit are more comfortable with C++.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1173
> +		D75AF59612F8CB9500FC0ADF /* DynamicAnnotations.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DynamicAnnotations.h; sourceTree = "<group>"; };

Did you verify that Mac build isn't broken? The header doesn't have a Private designation as far as I can tell, so it won't be visible from WebCore even with a forwarding header.

> Source/JavaScriptCore/wtf/DynamicAnnotations.c:2
> +/* Copyright (c) 2011, Google Inc.
> + * All rights reserved.

Please put these on one line.

> Source/JavaScriptCore/wtf/DynamicAnnotations.c:33
> +void WTFAnnotateBenignRaceSized(const char*, int, const volatile void*, long, const char*) {}
> +void WTFAnnotateHappensBefore(const char*, int, const volatile void*) {}
> +void WTFAnnotateHappensAfter(const char*, int, const volatile void*) {}

We normally add spaces in "{ }".

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:109
> +        // The atomic decrement should be annotated to be understood
> +        // by data race detection tools, e.g. ThreadSanitizer.
> +        // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations#Reference_counting
> +        WTF_ANNOTATE_HAPPENS_BEFORE(&m_refCount);

I don't see how adding three lines of comments each time we want to use WTF_ANNOTATE_HAPPENS_BEFORE is a good thing. Can this comment just be removed?

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:112
> +        if (atomicDecrement(&m_refCount) <= 0) {
> +            WTF_ANNOTATE_HAPPENS_AFTER(&m_refCount);
>              return true;

When is WTF_ANNOTATE_HAPPENS_AFTER called otherwise? Does it need to be?

> Tools/DumpRenderTree/ForwardingHeaders/wtf/DynamicAnnotations.h:1
> +#include <JavaScriptCore/DynamicAnnotations.h>

I suspect that you need to add forwarding headers in WebKit, WebKit2 and other projects which probably include ThreadSafeShared indirectly. I wish EWS could tell us.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.