[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 #44 from Timur Iskhodzhanov <timurrrr at google.com>  2011-03-22 10:15:08 PST ---
Thank you very much for your feedback.

Currently I'm running build-webkit locally on a newer version of the patch.
Besides addressing your comments (except for the comments comment) I've found one more place where these annotations should be added: wtf/ThreadSafeRefCounted.h, derefBase().
The code looks like a very precise copy of wtf/ThreadSafeShared.h there.

I'll upload a newer version as soon as build-webkit passes.

(In reply to comment #41)
> (From update of attachment 85441 [details])
> 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++.
The original dynamic_annotations.c file is a C file. We don't need any C++ features there and also we want to avoid mangling. Of course this can be done with `extern "C"`.
Do you think it's worth changing to C++? 
After all, we don't plan to change it often.

> > 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.
Very good catch!
Interestingly, it was working fine (./Tools/Scripts/build-webkit)
but started failing when I've added the annotations to another file: wtf/ThreadSafeRefCounted.h (to be added in the next patch)

> > Source/JavaScriptCore/wtf/DynamicAnnotations.c:2
> > +/* Copyright (c) 2011, Google Inc.
> > + * All rights reserved.
> Please put these on one line.
Done

> > 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 "{ }".
Done.

> > 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?
Mark pointed out that the annotations may be unreadable for non-experts.
This is one of the reasons to keep at least *some* comments.
Do it's enough to have only the link in the comments?
I can add some extra info on that wiki page if you like. 

> > 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?
Can you please re-phrase the question? I don't quite get it.
We need to call HAPPENS_BEFORE(&ref) on each deref()
and call HAPPENS_AFTER(&ref) before calling free()/operator delete.
You may read this as "free/delete happens-after all calls to deref()".

> > 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.
How can I do that?
Is there an automated tool to do that?
We already have 4 .h files in the patch, I'm starting to get confused with them :)

(In reply to comment #42)
> I pointed out via email that this patch also doesn’t update the case where atomic operations are not used in ThreadSafeShared.h.
You mean when synchronization is done using a mutex?
We don't need annotations in this case.
Data race detection tools usually work fine except for rare cases when synchronization is done using atomics - and that's why we use annotations in the atomic version of derefBase().

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