Summary: | Add some dynamic annotations to JavaScriptCore/wtf | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timur Iskhodzhanov <timurrrr> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, buildbot, commit-queue, dbates, dglazkov, eric, ggaren, jamesr, kcc, levin, mrowe, webkit.review.bot | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Timur Iskhodzhanov
2011-02-03 18:46:35 PST
Created attachment 81171 [details]
Patch
Created attachment 81172 [details]
Patch
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.
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 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? It's an svn-apply bug. See https://bugs.webkit.org/show_bug.cgi?id=39607 and related. 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. 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).
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*" Created attachment 81246 [details]
Patch
(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! Created attachment 81253 [details]
Patch
Attachment 81246 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7701083 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. Attachment 81246 [details] did not build on win: Build output: http://queues.webkit.org/results/7692967 Attachment 81253 [details] did not build on win: Build output: http://queues.webkit.org/results/7697646 Attachment 81253 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7698607 Attachment 81253 [details] did not build on win: Build output: http://queues.webkit.org/results/7702114 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 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. Created attachment 84441 [details]
Patch
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 Attachment 84441 [details] did not build on win: Build output: http://queues.webkit.org/results/8084278 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) 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
Comment on attachment 84441 [details]
Patch
Marking as r- since this breaks the Windows build and includes random unrelated changes inside the Tools directory.
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. The $(PRODUCTION) messages are not fatal. (In reply to comment #28) > The $(PRODUCTION) messages are not fatal. Correct. They even helpfully start with “warning” rather than “error” to indicate this. 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': ? ... 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. Created attachment 84607 [details]
Patch
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... Attachment 84607 [details] did not build on win: Build output: http://queues.webkit.org/results/8087364 Created attachment 85329 [details]
Patch
Created attachment 85333 [details]
Patch
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/ ... My patch finally passed the Windows bots. Anything else I should do? Created attachment 85441 [details]
Patch
This seems pretty good to me - do you have any other comments, Mark, now that it's compiling on Windows? 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. I pointed out via email that this patch also doesn’t update the case where atomic operations are not used in ThreadSafeShared.h. 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). 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(). (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. (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... > 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. Created attachment 87120 [details]
Patch
(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? 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? 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+.
David, Thank you very much! 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 Created attachment 87377 [details]
Patch
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? Comment on attachment 87377 [details]
Patch
OK, let's try again.
I wonder if this is getting bit by the svn-apply with windows line endings bug. (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 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 Created attachment 87603 [details]
Patch
(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." 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.
(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. 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. http://trac.webkit.org/changeset/82511 might have broken Leopard Intel Release (Tests) (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. (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 :) |