Bug 28054 - use derefInNotNull function to work around winscw compiler bug regarding template arguments.
Summary: use derefInNotNull function to work around winscw compiler bug regarding temp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Emulator S60 3rd edition
: P2 Normal
Assignee: Yongjun Zhang
URL:
Keywords: Qt
Depends on:
Blocks: 27065 29738
  Show dependency treegraph
 
Reported: 2009-08-06 12:09 PDT by Yongjun Zhang
Modified: 2010-09-02 15:53 PDT (History)
6 users (show)

See Also:


Attachments
Fix the issue with a helper function. (2.96 KB, patch)
2009-08-06 12:23 PDT, Yongjun Zhang
darin: review-
Details | Formatted Diff | Diff
Modified patch as per Darin's comments. (2.64 KB, patch)
2009-08-18 13:59 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
add comments in the code to explain why these two changes are needed. (3.25 KB, patch)
2009-08-19 14:20 PDT, Yongjun Zhang
eric: review-
Details | Formatted Diff | Diff
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr. (2.49 KB, patch)
2009-09-04 07:18 PDT, Yongjun Zhang
eric: review-
eric: commit-queue-
Details | Formatted Diff | Diff
fix build break in mac build. (3.59 KB, patch)
2009-09-09 13:39 PDT, Yongjun Zhang
eric: review-
Details | Formatted Diff | Diff
what is still required (6.89 KB, text/plain)
2009-09-11 07:30 PDT, Janne Koskinen
no flags Details
modify the patch to include Eric's review comments. (3.94 KB, patch)
2009-09-25 12:26 PDT, Yongjun Zhang
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
remove the Tab in previous patch, please review, thanks. (3.94 KB, patch)
2009-09-28 09:13 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
include PassRefPtr.h explicitly to avoid possible building break. (4.01 KB, patch)
2009-10-06 12:33 PDT, Yongjun Zhang
abarth: review-
Details | Formatted Diff | Diff
Rebased against trunk, built successfully for both Safari/Mac and Qt/Mac (6.18 KB, patch)
2010-02-10 01:48 PST, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Split up removal of bool hack into separate patch (2.09 KB, patch)
2010-02-10 03:31 PST, Tor Arne Vestbø
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Split up drefIfNotNull() patch (5.21 KB, patch)
2010-02-10 03:35 PST, Tor Arne Vestbø
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2009-08-06 12:09:57 PDT
In the following code:

-----------------------------------------------------
class MyTemplatedArgument;

class ContainerClass {

private:
   PassRefPtr<MyTemplatedArgument> t;
};
-----------------------------------------------------

Winscw compiler will complain that MyTemplatedArugment is not defined so that it couldn't find the deref() implementation of MyTemplatedArgument. Forward declaration doesn't help. In previous S60 port, we had to explicitly include the MyTemplatedArugment.h before ContainerClass declaration to resolve this issue.  This leads to a lot of changes in WebCore files and some requires moving around header files.

This is because PassRefPtr's dtor is inlined and the MyTemplatedArguemnt::deref() is called inside PassRefPtr's dtor, and the compiler tries to further resolve deref() which is not visible yet if the header file is not included.

To avoid changing large number of WebCore files just to get around this compiler issue, we can use an helper function to defer resolving deref().  We flag this changes in COMPILER(WINSCW) to avoid any possible performance hit in hardware.
Comment 1 Yongjun Zhang 2009-08-06 12:23:27 PDT
Created attachment 34218 [details]
Fix the issue with a helper function.

Use a helper function to work around winscw compiler forward declaration bug regarding templated classes.

Add parenthesis around (PassRefPtr::*UnspecifiedBoolType) to make winscw compiler
work with the default UnSpecifiedBoolType() operator, which removes the winscw
specific bool cast hack.
Comment 2 Darin Adler 2009-08-06 12:32:42 PDT
Comment on attachment 34218 [details]
Fix the issue with a helper function.

> +#if COMPILER(WINSCW)
> +    template <typename T> void releaseRef(T*);
> +#endif

This doesn't need to be #ifdef'd compiler-specific. We can use this with the other compilers too. But it needs to be an inline function. It will hurt performance to not have it inlined. A good name for it would be derefIfNonNull or safeDeref.

Since this is designed to be used only internally, we should give it a name that won't tempt anyone to use it. And probably put it in a different namespace, as is done with WTF::Internal in FastAllocBase.h.

releaseRef is not a good name for this -- it's already the name for an unrelated member function.

> +#if COMPILER(WINSCW)
> +    template <typename T> void releaseRef(T* ptr)
> +    {
> +        if (UNLIKELY(ptr != 0))
> +            ptr->deref();
> +    }
> +#endif

This should be marked inline and probably also should be defined before the PassRefPtr template just in case order matters. For non-template code it can sometimes be important to define inline functions before using them.

Otherwise looks quite good.

review- because of the comments above.
Comment 3 Yongjun Zhang 2009-08-18 13:59:10 PDT
Created attachment 35070 [details]
Modified patch as per Darin's comments.

Thanks for the review!

I modified the patch according to the comments.  For winscw compiler, if inline is present in derefInNotNull templated function definition, the compiler will try to resolve deref() and will eventually report error if the argument class T is not defined before PassRefPtr<T> is used.  Therefore, I had to remove inline for winscw compiler, and add inline for other compilers to avoid introducing any performance hit.
Comment 4 Eric Seidel (no email) 2009-08-18 15:14:49 PDT
Comment on attachment 35070 [details]
Modified patch as per Darin's comments.

I think we need comments in the code to document what these two work-arounds do.

One about the forward declaration. And one about the need for the ().
Comment 5 Yongjun Zhang 2009-08-19 14:20:47 PDT
Created attachment 35143 [details]
add comments in the code to explain why these two changes are needed.
Comment 6 Eric Seidel (no email) 2009-08-19 14:45:10 PDT
Comment on attachment 35143 [details]
add comments in the code to explain why these two changes are needed.

Looks great!

typo:
 174     // which will fail compiling when PassRefPtr<T> is used as class member or fucntion 

if you were a committer I would r+ this and you could fix the typo when landing.
Comment 7 Darin Adler 2009-08-19 17:27:30 PDT
+#if COMPILER(WINSCW)
+    // Remove inline here to prevent winscw compiler agressively resolving T::deref(),
+    // which will fail compiling when PassRefPtr<T> is used as class member or fucntion 
+    // arguments before T is defined.
+    template<typename T> void derefIfNotNull(T* ptr)
+#else
+    template<typename T> inline void derefIfNotNull(T* ptr)
+#endif    

I think you should put the ifdef around only the word "inline".

I think the definition of the derefIfNotNull function template should come before the definition of the PassRefPtr class, since inlining is much more likely if they are in that order. You don't even need a separate declaration and definition.
Comment 8 Yongjun Zhang 2009-09-04 07:18:39 PDT
Created attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.
Comment 9 Janne Koskinen 2009-09-08 03:35:45 PDT
This needs bit more tweaking.

Every call of ptr->ref() and ptr->deref() from inlined function in refptr.h and passrefptr.h must call non-inlined function instead (derefIfNotNull/refIfNotNull). This way we can get away of using extra includes in all webkit code with exception of threading functions.
Comment 10 Eric Seidel (no email) 2009-09-08 15:13:19 PDT
Comment on attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.

LGTM.
Comment 11 Eric Seidel (no email) 2009-09-08 17:44:05 PDT
Comment on attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.

Rejecting patch 39056 from commit-queue.

This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1
Comment 12 Eric Seidel (no email) 2009-09-09 09:34:24 PDT
Sadly build logs are not yet available from the commit-bot.  But it appears this patch does not build on Mac in Release mode.
Comment 13 Yongjun Zhang 2009-09-09 10:10:53 PDT
(In reply to comment #12)
> Sadly build logs are not yet available from the commit-bot.  But it appears
> this patch does not build on Mac in Release mode.

Thanks a lot!

I will check why it fails in Mac build in Release mode.
Comment 14 Yongjun Zhang 2009-09-09 13:39:55 PDT
Created attachment 39298 [details]
fix build break in mac build.

Thanks a lot for review.

The reason it fails the mac build is JSObjectWrapper::ref() and deref() are defined as private methods in WebCore/bridge/jni/jni_instance.h.  The new patch moves them to public to fix the build break.

Is there any reason why ref() and deref() needs to be private in this case?  If yes, the patch might be wrong and I will figure out a different way to address this.
Comment 15 Janne Koskinen 2009-09-11 07:30:18 PDT
Created attachment 39431 [details]
what is still required

For refptr.h/passrefptr.h to work every inline function containing either ref() or deref() needs to be changed to non-inlined.
This is now addition to already commited patch that added the derefIfNotNull() - function.
Comment 16 Darin Adler 2009-09-11 10:49:38 PDT
> For refptr.h/passrefptr.h to work every inline function containing either ref()
> or deref() needs to be changed to non-inlined.

Won't that hurt performance?
Comment 17 Yongjun Zhang 2009-09-11 11:06:24 PDT
(In reply to comment #16)
> > For refptr.h/passrefptr.h to work every inline function containing either ref()
> > or deref() needs to be changed to non-inlined.
> 
> Won't that hurt performance?

Thanks for the comments.

Yes, it will hurt performance.  Good thing is winscw compiler is only used for building apps running on S60 emulator, and for real hardware, RVCT (armcc) compiler is used, so we wouldn't see performance penalty in real hardware because the inlined version is used for RVCT.

That being said, I don't think it is a good idea to change every inlined ref&deref to be non-inlined. Two reasons:

1. it makes the code a bit harder to read; 
      if (ptr)
          ptr->deref();
      is much more readable than:
         derfIfNotNull(ptr);

2. even in emulator, we still want performance hit to be as small as possible; therefore, the patch only changes ref&deref to be non-inline at places where it is really necessary.
Comment 18 Janne Koskinen 2009-09-11 12:17:02 PDT
> 1. it makes the code a bit harder to read; 

I think this is matter of opinion. I made it to be consistent all for all functions.
If wanted it can be changed e.g.
if (ptr) derefInternal(ptr); // or similar

> 2. even in emulator, we still want performance hit to be as small as possible;
> therefore, the patch only changes ref&deref to be non-inline at places where it
> is really necessary.

I don't care about the emulator performance at all as it hopefully will not <fingers crossed> live too long.

Yes, it is not needed for every function, but I know that some of the constructors, copy constructors and assignment operators will need the modification as well.
You have now covered destructors and clear() (in the new patch).
Comment 19 Eric Seidel (no email) 2009-09-24 13:14:09 PDT
Comment on attachment 39298 [details]
fix build break in mac build.

This patch looks OK.  It would be better with references to the winscw bugs.  You mention above that winscw might not be used for much longer?  We should have bugs on file about rolling out these kind of hacks once winscw is no longer used.

Have you filed a bug with the winscw team about this issue?  Will you please add a link to said bug to the ChangeLog so someone looking at this later might know if they can remove this hack?
Comment 20 Yongjun Zhang 2009-09-25 12:26:45 PDT
Created attachment 40132 [details]
modify the patch to include Eric's review comments.

thanks for the comments, Eric.

I added the winscw compiler bug url in the ChangeLog and created another webkit bug (https://bugs.webkit.org/show_bug.cgi?id=29738) to remind us to revert the changes when winscw compiler bug is fixed.
Comment 21 Eric Seidel (no email) 2009-09-25 12:29:14 PDT
Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

OK.  I'm OK with this hack for now.  Seeing cq+ since Yongjun is not a committer to my knowledge.
Comment 22 Eric Seidel (no email) 2009-09-25 12:29:52 PDT
Thank you for following up with the bugs btw.  I think it's important that we document this sort of thing for long-term code cleanliness. :)
Comment 23 Yongjun Zhang 2009-09-25 13:06:35 PDT
(In reply to comment #22)
> Thank you for following up with the bugs btw.  I think it's important that we
> document this sort of thing for long-term code cleanliness. :)

Thanks a lot, Eric.

I absolutely agree;  it is a good practice to document those temporary hacks to avoid clattering WebKit source code in long run.  I also find it is very helpful to tracking down those changes in the future.  Maybe we could add this practice to  WebKit coding guidelines?
Comment 24 WebKit Commit Bot 2009-09-25 19:51:01 PDT
Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

Rejecting patch 40132 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
criptCore/wtf/RefPtr.h
	M	WebCore/ChangeLog
	M	WebCore/bridge/jni/jni_instance.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/WebCore/bridge/jni/jni_instance.h

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 25 Yongjun Zhang 2009-09-28 09:13:23 PDT
Created attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

 
Maybe it would be better to modify WebKitTools/Script/svn-create-patch in a way that it can remove Tabs in the resulting patch.
Comment 26 Eric Seidel (no email) 2009-09-28 11:37:43 PDT
Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

svn-create-patch should certainly at least warn about tabs.  (bug 29509)  Having a --remove-tabs option wouldn't necessarily be bad either.
Comment 27 WebKit Commit Bot 2009-09-28 12:31:50 PDT
Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Rejecting patch 40239 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11337 test cases.
http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed

Exiting early after 1 failures. 8972 tests run.
259.20s total testing time

8971 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output
Comment 28 Eric Seidel (no email) 2009-09-28 12:56:12 PDT
Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Flakey test. bug 29322.
Comment 29 WebKit Commit Bot 2009-09-28 13:09:48 PDT
Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Clearing flags on attachment: 40239

Committed r48825: <http://trac.webkit.org/changeset/48825>
Comment 30 WebKit Commit Bot 2009-09-28 13:09:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Mark Rowe (bdash) 2009-09-28 15:10:58 PDT
This change is broken for two reasons (one serious, one less so):
1) RefPtr.h uses a function named derefIfNotNull which is not defined anywhere.  It frequently works when it picks up a definition from PassRefPtr.h, but there's no requirement that that header be included.
2) Adding function call overhead to every deref is a terrible idea.

Reason 1 will cause build failures so this change needs to be rolled out.
Comment 32 Mark Rowe (bdash) 2009-09-28 15:16:43 PDT
Rolled out in r48839.  Please revise the patch to address at least the first issue and resubmit for review.
Comment 33 Eric Seidel (no email) 2009-09-28 15:38:46 PDT
Thank you Mark.
Comment 34 Eric Seidel (no email) 2009-10-05 10:55:55 PDT
Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

This patch was rolled out, marking r-.
Comment 35 Yongjun Zhang 2009-10-06 12:33:28 PDT
Created attachment 40736 [details]
include PassRefPtr.h explicitly to avoid possible building break.

Mark and Eric, thank you both for the comments.

I copied the idea from adoptRef<T>() in PassRefPtr.h when introducing derefIfNotNull(); and I assume if adoptRef doesn't break the build, neither will derefIfNotNull().

To be safe, I modified the patch and explicitly include PassRefPtr.h to prevent possible building break.

Regarding the concern on performance hit, the patch only removes inline for winscw compiler; for other compilers, derefIfNotNull is still inlined.
Comment 36 Adam Barth 2009-10-14 23:27:15 PDT
Comment on attachment 40736 [details]
include PassRefPtr.h explicitly to avoid possible building break.

Why is there a random change to jni_instance in this patch?
Comment 37 Janne Koskinen 2009-10-15 00:06:52 PDT
(In reply to comment #36)
> (From update of attachment 40736 [details])
> Why is there a random change to jni_instance in this patch?

Well, it's not random but maybe it should be a separate patch.
Comment 38 Janne Koskinen 2009-12-14 03:38:07 PST
Could this be fixed ? webkit won't compile for s60 emulator environment before this issue is resolved.
Comment 39 Tor Arne Vestbø 2010-02-10 01:48:28 PST
Created attachment 48476 [details]
Rebased against trunk, built successfully for both Safari/Mac and Qt/Mac
Comment 40 WebKit Review Bot 2010-02-10 01:54:09 PST
Attachment 48476 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Tor Arne Vestbø 2010-02-10 03:23:46 PST
(In reply to comment #40)
> Attachment 48476 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line 
> [whitespace/newline] [4]
> Total errors found: 1

Seems we have quite a few of these already in header files:

[whopper:~/dev/webkit/wip] $ git grep -E ') {.*; [A-Za-z]+' -- **/*.h | wc -l
270

Eric, would it make sense to skip this check for headers, like we do for macros and switch cases?
Comment 42 Tor Arne Vestbø 2010-02-10 03:31:19 PST
Created attachment 48480 [details]
Split up removal of bool hack into separate patch
Comment 43 Tor Arne Vestbø 2010-02-10 03:35:54 PST
Created attachment 48481 [details]
Split up drefIfNotNull() patch
Comment 44 WebKit Review Bot 2010-02-10 03:38:27 PST
Attachment 48481 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 Simon Hausmann 2010-02-10 03:44:40 PST
Comment on attachment 48481 [details]
Split up drefIfNotNull() patch

Treating the style as false positive and considering that this is the same as r47592, it looks good to me.
Comment 46 Eric Seidel (no email) 2010-05-17 00:36:28 PDT
Did this land?  This has been in the pending-commit list for months. :(
Comment 47 WebKit Commit Bot 2010-09-02 15:11:12 PDT
Comment on attachment 48480 [details]
Split up removal of bool hack into separate patch

Rejecting patch 48480 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Parsed 2 diffs from patch file(s).
patching file JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file JavaScriptCore/wtf/RefPtr.h
Hunk #1 FAILED at 65.
1 out of 1 hunk FAILED -- saving rejects to file JavaScriptCore/wtf/RefPtr.h.rej

Full output: http://queues.webkit.org/results/3972045
Comment 48 WebKit Commit Bot 2010-09-02 15:47:15 PDT
Comment on attachment 48481 [details]
Split up drefIfNotNull() patch

Rejecting patch 48481 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Last 500 characters of output:
/wtf/RefPtr.h.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bridge/jni/jsc/JavaInstanceJSC.h
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 57 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/bridge/jni/jsc/JavaInstanceJSC.h.rej
patching file WebCore/bridge/jni/v8/JavaInstanceV8.h
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 57 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/bridge/jni/v8/JavaInstanceV8.h.rej

Full output: http://queues.webkit.org/results/3967060
Comment 49 Eric Seidel (no email) 2010-09-02 15:53:43 PDT
I'm suspecting these were already landed.  If they weren't please don't reopen this unless you plan do land it.  :)