Bug 53747 – Add some dynamic annotations to JavaScriptCore/wtf

• Component: New Bugs
• Status: RESOLVED FIXED

[Timur Iskhodzhanov]

Add some dynamic annotations to JavaScriptCore/wtf

[Timur Iskhodzhanov]

Created attachment 81171 [details]
Patch

[Timur Iskhodzhanov]

Created attachment 81172 [details]
Patch

[James Robinson]

Comment on attachment 81172 [details]
Patch

Looks good to me!  I think having these annotations available will be quite useful and we've already found some WebKit bugs using this.

[WebKit Commit Bot]

Comment on attachment 81172 [details]
Patch

Rejecting attachment 81172 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
g file Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
patching file Source/JavaScriptCore/wtf/CMakeLists.txt
patching file Source/JavaScriptCore/wtf/DynamicAnnotations.c
patching file Source/JavaScriptCore/wtf/DynamicAnnotations.h
patching file Source/JavaScriptCore/wtf/ThreadSafeShared.h
patching file Source/JavaScriptCore/wtf/text/StringStatics.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/7691872

[James Robinson]

The patch is against r77582 and the last change to WTF.vcproj was r77062 according to http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj so I have no idea why it failed to apply.  Eric/Adam, any thoughts?

[Eric Seidel (no email)]

It's an svn-apply bug.  See https://bugs.webkit.org/show_bug.cgi?id=39607 and related.

[Mark Rowe (bdash)]

Comment on attachment 81172 [details]
Patch

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

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:27
> + * Author: Kostya Serebryany

We don’t list author information in source files.  The version control system tracks this information.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:31
> +#define DynamicAnnotations_h

The convention in the wtf directory appears to be to use WTF_FileName_h as the include guard.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:33
> +#include "Platform.h"

This include should not be present.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:35
> +/* This file defines dynamic annotations for use with dynamic analysis

IMO the term “dynamic annotations” is far too generic to clearly communicate what it is that this file provides.  Is there some clearer term that can be used instead?

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:63
> +#define ANNOTATE_HAPPENS_AFTER(obj) WebKitAnnotateCondVarWait(__FILE__, __LINE__, obj, 0)

The naming of these two macros is confusing to me.  The names don’t appear to communicate what their purpose is.  Something happens before or after something else?  Can we come up with self-documenting names so that we don’t need to add multi-line comments at every use?

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:71
> +void WebKitAnnotateCondVarWait(const char *file, int line, const volatile void *cv, const volatile void *lock);

The WebKit prefix on these functions seems less than ideal given that they are in WTF.  A WTF prefix would be more in line with other straight C functions in WTF.

The functions should be updated to match the WebKit style guide: Abbreviations shouldn’t be used in the names, the * for pointers should be next to the type name, and argument names shouldn’t be abbreviated.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:80
> +#define ANNOTATE_HAPPENS_AFTER(obj) /* empty */

It’s not clear to me that the comments here add any value.

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:65
> +#include <wtf/DynamicAnnotations.h>

Given that ThreadSafeShared.h is included by projects outside of JavaScriptCore (e.g., by WebCore) I suspect that this #include will break the Mac build unless you add a forwarding header.

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:108
> +        // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations#Reference_counting

I can’t help but feel that if the macro names were clearer then we wouldn’t need comments like this where they’re used.

[Mark Rowe (bdash)]

Comment on attachment 81172 [details]
Patch

Setting to r- since it’s very likely that this will break the Mac build (see comment above about ThreadSafeShared.h).

[David Levin]

Nit:  * and &, etc. pair with the type in WebKit style and opposed to the variable. This patch is consistently not following this.

i.e. "const char *" should be "const char*"

[Timur Iskhodzhanov]

Created attachment 81246 [details]
Patch

[Timur Iskhodzhanov]

(In reply to comment #7)
> (From update of attachment 81172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81172&action=review

Mark, thanks for your review!
I've addressed most of your comments in my new patch though I have a couple of questions to ask.

So, the structure of my reply is:
1) Done, done, done
2) A couple of questions
3) A couple of comments I did not address on purpose

1.
> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:33
> > +#include "Platform.h"
> 
> This include should not be present.
Done.
I'm slightly worried that some files may not include config.h/Platform.h before DynamicAnnotations.h but it would violate your convention. 

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:71
> > +void WebKitAnnotateCondVarWait(const char *file, int line, const volatile void *cv, const volatile void *lock);
> 
> The WebKit prefix on these functions seems less than ideal given that they are in WTF.  A WTF prefix would be more in line with other straight C functions in WTF.
Done.

> The functions should be updated to match the WebKit style guide: Abbreviations shouldn’t be used in the names, the * for pointers should be next to the type name, and argument names shouldn’t be abbreviated.
Done except for abbrevations in the function names.
These dynamic analysis tools intercept annotations based on their names (disregarding the prefix), for example they do intercept
AnnotateCondVarSignal, WTFAnnotateCondVarSignal, WhatEverAnnotateCondVarSignal but they won't intercept AnnotateConditionVariableSignal.
In order to fix this we'll need to patch the tools upstream. This will take some extra time to roll new binaries everywhere and I'm not sure it's worth the effort, especially because developers are discouraged to use these functions directly.  

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:80
> > +#define ANNOTATE_HAPPENS_AFTER(obj) /* empty */
> 
> It’s not clear to me that the comments here add any value.
Done - I wrote a new comment for the whole bunch of these macros with some explanation. 

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:27
> > + * Author: Kostya Serebryany
> We don’t list author information in source files.  The version control system tracks this information.
After all, I'm not Kostya Serebryany :)
I'll talked to him and he removed that line upstream; I've updated the patch

2.
> > Source/JavaScriptCore/wtf/ThreadSafeShared.h:65
> > +#include <wtf/DynamicAnnotations.h>
> 
> Given that ThreadSafeShared.h is included by projects outside of JavaScriptCore (e.g., by WebCore) I suspect that this #include will break the Mac build unless you add a forwarding header.
Good catch!
I think build-webkit was succeeding locally on my MacBook but anyways I've added these files (similar to the forwarding headers for wtf/Atomics.h)
Should I also add these forwarding headers to some Makefiles/gyp/xcode/etc? If so - where?  

3.
A little background:
the files I'm trying to add are based on
http://code.google.com/p/data-race-test/source/browse/trunk/dynamic_annotations/dynamic_annotations.h
and
http://code.google.com/p/data-race-test/source/browse/trunk/dynamic_annotations/dynamic_annotations.c

We've been using those files at Google for more than two years already, including open-source projects like Chromium, NativeClient and more.

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:35
> > +/* This file defines dynamic annotations for use with dynamic analysis
> 
> IMO the term “dynamic annotations” is far too generic to clearly communicate what it is that this file provides.  Is there some clearer term that can be used instead?
> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:63
> > +#define ANNOTATE_HAPPENS_AFTER(obj) WebKitAnnotateCondVarWait(__FILE__, __LINE__, obj, 0)
> The naming of these two macros is confusing to me.  The names don’t appear to communicate what their purpose is.  Something happens before or after something else?  Can we come up with self-documenting names so that we don’t need to add multi-line comments at every use?
> > Source/JavaScriptCore/wtf/ThreadSafeShared.h:108
> > +        // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations#Reference_counting
> I can’t help but feel that if the macro names were clearer then we wouldn’t need comments like this where they’re used.
Yeah, naming of this stuff is a tough thing.
Personally, I totally agree with you.
The problem is that to really understand what these macros are doing you have to know the guts of data race detection tools which is not what we expect from the developers...
We've already tried renaming them a couple of times discussing them people working in this domain (including Mike Burrows, author of one of the first production-quality race detector) and what we have now is the best we could advance.
ANNOTATE_HAPPENS_BEFORE/AFTER is not confusing too much in my mind :) At least everyone understands that something happens-before something else and we give a link to a comprehensive explanation why it should be done that way.

Looking forward for your comments!

[Timur Iskhodzhanov]

Created attachment 81253 [details]
Patch

[WebKit Review Bot]

Attachment 81246 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7701083

[Kostya Serebryany]

I wonder if we can put these files into WebKit unchanged (maybe some kind of third_party directory or something). 
We already maintain a handful of different copies of dynamic_annotations.[ch] in different projects. 
Maintainance pain grows too fast with each extra copy, if the copy differs from the original.

[Build Bot]

Attachment 81246 [details] did not build on win:
Build output: http://queues.webkit.org/results/7692967

[Build Bot]

Attachment 81253 [details] did not build on win:
Build output: http://queues.webkit.org/results/7697646

[WebKit Review Bot]

Attachment 81253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7698607

[Build Bot]

Attachment 81253 [details] did not build on win:
Build output: http://queues.webkit.org/results/7702114

[Timur Iskhodzhanov]

Sorry for the delay, I was travelling and then had some other things to do.

So, the Chromium build problem arises from the following:
a) WebKit --chromium includes some Chromium headers that have their own (upstream!) copy of dynamic_annotations.h
  As a result, there are two inclusions: DynamicAnnotations.h, then dynamic_annotations.h and they have different include gueards.
  Chromium (presumably) includes WebKit headers as well in a different orders.
  Possible options are:
    - Upload upstream dynamic_annotations headers to WebKit third_party/
       I was told this this may be extremely difficult given the number of project generators used in WebKit and the fact that there's only one third_party project in WebKit atm (ANGLE)
    - Make a _full_ copy of dynamic_annotations and put them into wtf/ following WebKit codestyle.
       I think this is very fragile and also it will make updating these annotations a nightmare (WebKit headers will be sometimes used in Chromium instead of Chromium ones - ouch!).
    - Rename the WebKit annotations macros to WTF_ANNOTATE_BLA_BLA_BLA
I'm going to try the latter

b) Win - I've no idea what to do.
First, I've searched for all references to Atomics.h (in the directory structure as well as in project files) and added similar references to DynamicAnnotations.h but looks like it's not enough
Second, I can't get my only VS2005-enabled Windows machine to build a clean WebKit checkout to understand what happens...

Precaution: Maybe I'll upload a few updated version of this patch to see how the try queue bots do with them

[Timur Iskhodzhanov]

Ah, the Windows failures now make [no] sense to me:
http://queues.webkit.org/results/7702114
"22>------ Build started: Project: MiniBrowser, Configuration: Debug Win32 ------
22>Performing Pre-Build Event...
22>Project : error PRJ0002 : Error result 1 returned from 'C:\Windows\system32\cmd.exe'.
22>Project : warning PRJ0018 : The following environment variables were not found:
22>$(PRODUCTION)"

I'm seeing exactly the same failure when trying to do a build of a clean WebKit checkout.
I'm going to disregard the windows failures unless asked otherwise.

[Timur Iskhodzhanov]

Created attachment 84441 [details]
Patch

[Timur Iskhodzhanov]

Mark, David, James,

I think I've addressed your comments and it has passed on the bots (except for those that failed to "svn apply" -> how can I clobber them?)
Also, "Tools/Scripts/build-webkit [--chromium]" succeeded for me locally on Mac.

Should I do anything else with this patch?

Thanks,
Timur Iskhodzhanov

[Build Bot]

Attachment 84441 [details] did not build on win:
Build output: http://queues.webkit.org/results/8084278

[Timur Iskhodzhanov]

Windows failures are still unrelated

22>Project : error PRJ0002 : Error result 1 returned from 'C:\Windows\system32\cmd.exe'.
22>Project : warning PRJ0018 : The following environment variables were not found:
22>$(PRODUCTION)

[Mark Rowe (bdash)]

The Windows failure is most certainly not unrelated:

> 14>C:\cygwin\home\buildbot\Webkit\WebKitBuild\Debug\Include\private\JavaScriptCore/ThreadSafeShared.h(65) : fatal error C1083: Cannot open include file: 'wtf/DynamicAnnotations.h': No such file or directory

[Mark Rowe (bdash)]

Comment on attachment 84441 [details]
Patch

Marking as r- since this breaks the Windows build and includes random unrelated changes inside the Tools directory.

[Timur Iskhodzhanov]

Ouch!

Sorry for the Tools/ changes introduced - webkit-patch had problems accessing my keychain on Mac for some reason so I temporary hacked the scripts and didn't notice that.
Maybe I'll try to do a clean checkout and upload a patch from there...

Regarding Windows failures - any ideas or tips how/where to fix that?
I have only one Windows machine with VS2005 and it fails to build a clean checkout with same "$(PRODUCTION)" errors all over the place.

Sorry for the mess with this patch, I'm not yet familiar with all WebKit contributing specifics and don't know the build system / webkit-update internals to work around the issues that shouldn't be there in the first place IMO.

[James Robinson]

The $(PRODUCTION) messages are not fatal.

[Mark Rowe (bdash)]

(In reply to comment #28)
> The $(PRODUCTION) messages are not fatal.

Correct.  They even helpfully start with “warning” rather than “error” to indicate this.

[Timur Iskhodzhanov]

22>------ Build started: Project: MiniBrowser, Configuration: Debug Win32 ------
22>Project : error PRJ0002 : Error result 1 returned from 'C:\Windows\system32\cmd.exe'.
22>Project : warning PRJ0018 : The following environment variables were not found:
22>$(PRODUCTION)
22>MiniBrowser - 1 error(s), 0 warning(s)
<- That didn't look like non-fatal to me: "error" was mentioned two times...

Can you please give me a hint where should I add a reference to wtf/DynamicAnnotations.h to be copied to a location required by
14>Webkit\WebKitBuild\Debug\Include\private\JavaScriptCore/ThreadSafeShared.h(65) : fatal error C1083: Cannot open include file: 'wtf/DynamicAnnotations.h':
?

[Timur Iskhodzhanov]

... especially given that from the Windows log

1> xcopy /y /d "..\..\wtf\*.h" "%ConfigurationBuildDir%\include\private\JavaScriptCore"
1>..\..\wtf\DynamicAnnotations.h
1>..\..\wtf\ThreadSafeShared.h
<- should have copied the DynamicAnnotations.h to the same dir as "ThreadSafeShared.h"

Should I replace 
#include <wtf/Atomics.h>
#include <wtf/DynamicAnnotations.h>
#include <wtf/ThreadingPrimitives.h>
with
#include <DynamicAnnotations.h>
#include <wtf/Atomics.h>
#include <wtf/ThreadingPrimitives.h>
?
(I don't think so)

I can find neither "Atomics.h" nor "ThreadingPrimitives.h" in the windows log to do to the same xcopy command for my new header.

[Timur Iskhodzhanov]

Created attachment 84607 [details]
Patch

[Timur Iskhodzhanov]

Just found that I was probably missing a reference to my forwarding header in 
Source/JavaScriptGlue/gyp/JavaScriptGlue.gypi
(uploaded a new patch)

Do you use this GYP file on Windows?

Now my "grep -R -H" and "find ." for Atomics.h and DynamicAnnotations.h and ThreadingPrimitives.h are the equal...

[Build Bot]

Attachment 84607 [details] did not build on win:
Build output: http://queues.webkit.org/results/8087364

[Timur Iskhodzhanov]

Created attachment 85329 [details]
Patch

[Timur Iskhodzhanov]

Created attachment 85333 [details]
Patch

[Timur Iskhodzhanov]

Looks like I've finally found the missing Forwarding header - it should have been in Tools/DumpRenderTree.
It's {no surprise,sad} I was not searching for the headers in Tools/ ...

[Timur Iskhodzhanov]

My patch finally passed the Windows bots.

Anything else I should do?

[Timur Iskhodzhanov]

Created attachment 85441 [details]
Patch

[James Robinson]

This seems pretty good to me - do you have any other comments, Mark, now that it's compiling on Windows?

[Alexey Proskuryakov]

Comment on attachment 85441 [details]
Patch

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.

[Mark Rowe (bdash)]

I pointed out via email that this patch also doesn’t update the case where atomic operations are not used in ThreadSafeShared.h.

[Kostya Serebryany]

FYI: 

Similar annotations are already present in a similar code in libstdc++. 
Search for _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER in these files: 
http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a00780_source.html
http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a00769_source.html
http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a01030_source.html

The documentation in libstdc++ contains a paragraph about race detection:
http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug.html#debug.races
The doc mentions support for 3 different tools (DRD , Helgrind , ThreadSanitizer). In reality, at least 4 tools are supported (these three and Intel Parallel Inspector).

[Timur Iskhodzhanov]

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

[David Levin]

(In reply to comment #44)
> 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.

wtf/ThreadSafeShared.h was renamed to wtf/ThreadSafeRefCounted.h this past weekend.
wtf/ThreadSafeShared.h is no longer around.

[Timur Iskhodzhanov]

(In reply to comment #45)
> (In reply to comment #44)
> > 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.
> 
> wtf/ThreadSafeShared.h was renamed to wtf/ThreadSafeRefCounted.h this past weekend.
> wtf/ThreadSafeShared.h is no longer around.
Thanks David,
I had a gut feeling that this is the case :)
I wonder why update-webkit didn't bark though...

[Alexey Proskuryakov]

> Do you think it's worth changing to C++? 

Yes. Keeping it as C would be an exception, and let's not make an exception if that's not required.

> Mark pointed out that the annotations may be unreadable for non-experts.
> This is one of the reasons to keep at least *some* comments.

If one sees a mysterious WTF_ANNOTATE_HAPPENS_BEFORE macro, they should jump to definition and read a comment there. A comment at call site can explain why we need it here in particular, not what it is.

I don't think that an external link is a particularly good thing to have in WebKit sources, but it's OK.

> and call HAPPENS_AFTER(&ref) before calling free()/operator delete.

OK. The "before"/"after" names are pretty misleading then - you need to call "before" before doing something, but you don't need to call "after" after doing that - instead, you call it before doing something else.

> How can I do that?
> Is there an automated tool to do that?

There is no automated tool to add forwarding headers. You just need to build on Mac, and add necessary headers in each project complaining that it can't find them.

[Timur Iskhodzhanov]

Created attachment 87120 [details]
Patch

[Timur Iskhodzhanov]

(In reply to comment #47)
> > Do you think it's worth changing to C++? 
> 
> Yes. Keeping it as C would be an exception, and let's not make an exception if that's not required.
Done

> > Mark pointed out that the annotations may be unreadable for non-experts.
> > This is one of the reasons to keep at least *some* comments.
> If one sees a mysterious WTF_ANNOTATE_HAPPENS_BEFORE macro, they should jump to definition and read a comment there. A comment at call site can explain why we need it here in particular, not what it is.
> I don't think that an external link is a particularly good thing to have in WebKit sources, but it's OK.
Done - removed the comment from ThreadSafeRefcounted.h, added more comments to 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?
> > How can I do that?
> > Is there an automated tool to do that?
> There is no automated tool to add forwarding headers. You just need to build on Mac, and add necessary headers in each project complaining that it can't find them.

$ ./Tools/Scripts/build-webkit
...
** BUILD SUCCEEDED **
===========================================================
 WebKit is now built (1h:10m:36s). 
 To run Safari with this newly-built code, use the
 "Tools/Scripts/run-safari" script.
===========================================================

How can I test "WebKit2 and other projects" locally?
Should I?

[Timur Iskhodzhanov]

Alexey,
Thank you for your review!

Should I wait for Mark to review as well
or go ask someone with commit access (or commit-bot) to commit?

[David Levin]

Comment on attachment 87120 [details]
Patch

Mark's comment was addressed (and he seems to have stopped commenting) plus the cq will catch a Mac build break if there would be one.

So setting cq+.

[Timur Iskhodzhanov]

David,
Thank you very much!

[WebKit Commit Bot]

Comment on attachment 87120 [details]
Patch

Rejecting attachment 87120 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:
atching file Source/JavaScriptGlue/ForwardingHeaders/wtf/DynamicAnnotations.h
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/ForwardingHeaders/wtf/DynamicAnnotations.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/ForwardingHeaders/wtf/DynamicAnnotations.h

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Alexey Proskuryakov', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/8281085

[Timur Iskhodzhanov]

Created attachment 87377 [details]
Patch

[Timur Iskhodzhanov]

I've no idea why the commit bot failed - there's nothing relevant in the last 500 characters of logs.

I just did update-webkit and then upload-webkit to rebase and it did not show any conflicts (except for auto-merged ChangeLog conflicts)

Any ideas what could go wrong / what should I do?

[Alexey Proskuryakov]

Comment on attachment 87377 [details]
Patch

OK, let's try again.

[James Robinson]

I wonder if this is getting bit by the svn-apply with windows line endings bug.

[Timur Iskhodzhanov]

(In reply to comment #57)
> I wonder if this is getting bit by the svn-apply with windows line endings bug.
I don't think I ever used Windows while preparing this patch

Also, the win/linux "try" bots (or how you call them?) are fine

[WebKit Commit Bot]

Comment on attachment 87377 [details]
Patch

Rejecting attachment 87377 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
atching file Source/JavaScriptGlue/ForwardingHeaders/wtf/DynamicAnnotations.h
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/ForwardingHeaders/wtf/DynamicAnnotations.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/ForwardingHeaders/wtf/DynamicAnnotations.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Alexey Proskuryakov', ..." exit_code: 1

Full output: http://queues.webkit.org/results/8283528

[Timur Iskhodzhanov]

Created attachment 87603 [details]
Patch

[Timur Iskhodzhanov]

(In reply to comment #57)
> I wonder if this is getting bit by the svn-apply with windows line endings bug.
How can I check?

Any ideas how to work-around the commit-bot issue?

FTR,
I've changed the first ChangeLog line to "Reviewed by Alexey Proskuryakov."

[David Levin]

Comment on attachment 87603 [details]
Patch

This doesn't need review. It just need a friend to land it.... I'll see if I can do that soon if no one else does it first.

[David Levin]

(In reply to comment #57)
> I wonder if this is getting bit by the svn-apply with windows line endings bug.

Yep.

Hunk #1 FAILED at 613.
1 out of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj.rej
Calling "patch -p0" for file "Source/JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj" returned status 1.  Pass --force to ignore patch failures.

[David Levin]

Committed as http://trac.webkit.org/changeset/82508

(In reply to comment #47)
> > and call HAPPENS_AFTER(&ref) before calling free()/operator delete.
> 
> OK. The "before"/"after" names are pretty misleading then - you need to call "before" before doing something, but you don't need to call "after" after doing that - instead, you call it before doing something else.

btw, I have to agree that not seeing an AFTER call is confusing in this case. It looked like a bug in the patch to me.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82511 might have broken Leopard Intel Release (Tests)

[David Levin]

(In reply to comment #65)
> http://trac.webkit.org/changeset/82511 might have broken Leopard Intel Release (Tests)

Badly reported due to bad commit entry and that change was only for vsprop files so it seems extremely unlikely to have broken the Leopard build.

[Timur Iskhodzhanov]

(In reply to comment #64)
> Committed as http://trac.webkit.org/changeset/82508

Thank you so much David!
We've made it under 2 months per patch :)