Bug 33084 - Fix for RVCT -Otime fatal compiler error
Summary: Fix for RVCT -Otime fatal compiler error
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 32715 32988
  Show dependency treegraph
 
Reported: 2009-12-31 09:21 PST by Norbert Leser
Modified: 2010-12-16 09:28 PST (History)
11 users (show)

See Also:


Attachments
Patch for Otime compiler error (3.98 KB, patch)
2009-12-31 09:21 PST, Norbert Leser
no flags Details | Formatted Diff | Diff
Updated patch for 33084 to fix tab issue (3.97 KB, patch)
2009-12-31 09:28 PST, Norbert Leser
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Leser 2009-12-31 09:21:41 PST
Created attachment 45713 [details]
Patch for Otime compiler error

RVCT compiler with "-Otime -O3" optimization tries to optimize out inline new'ed pointers that are passed as arguments.
Proposed patch assigns new'ed pointer explicitly outside function call.

This was initially submitted with bug #32988. I separated this out, per suggestion, for independent tracking/fixing.
Comment 1 WebKit Review Bot 2009-12-31 09:23:40 PST
Attachment 45713 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/API/JSClassRef.cpp:153:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1
Comment 2 Norbert Leser 2009-12-31 09:28:25 PST
Created attachment 45714 [details]
Updated patch for 33084 to fix tab issue

Fix for accidental tab.
Comment 3 WebKit Review Bot 2009-12-31 09:29:03 PST
style-queue ran check-webkit-style on attachment 45714 [details] without any errors.
Comment 4 Antonio Gomes 2010-01-01 20:29:53 PST
norbert, usually you do not need review for build fixes like this one.

please let me know if you want me to land, or set CQ+
Comment 5 Eric Seidel (no email) 2010-01-03 18:14:34 PST
Comment on attachment 45714 [details]
Updated patch for 33084 to fix tab issue

Have we filed a bug about this with RVCT?  There will be a zillion of these.  Every ::create() impelmentation in WebCore will have this problem as:
return adoptRef(new Foo);
 is how we create objects in Foo::create().

This definitely needs a RVCT bug number and more explanation. I don't think you're going to get far with this type of change in WebCore.
Comment 6 Simon Hausmann 2010-01-05 01:03:46 PST
It appears that this happens with normal -O2 and also with -Otime. I can reproduce this issue by executing the commandline manually. This currently breaks the build on the trunk.
Comment 7 Norbert Leser 2010-01-05 06:26:48 PST
I can confirm from our side as well that this compiler crash occurs even without the special -O options set, using the latest webkit from trunk.

I submitted the bug to RVCT already but still awaiting a bug# assignment.
Comment 8 Norbert Leser 2010-01-06 09:27:35 PST
Issue can be reproduced with both RVCT 2.2.686 and RVCT 4.0.697.

We have raised case 444841 with ARM.
Comment 9 Darin Adler 2010-01-06 09:35:12 PST
Comment on attachment 45714 [details]
Updated patch for 33084 to fix tab issue

This change is not a good idea unless it's sufficient by itself to work around the issue. I don't think it's going to be practical to program around this RVCT bug. As others pointed out, there are hundreds of call sites like this in WebCore.
Comment 10 Norbert Leser 2010-01-06 10:43:59 PST
(In reply to comment #9)
> (From update of attachment 45714 [details])
> This change is not a good idea unless it's sufficient by itself to work around
> the issue. I don't think it's going to be practical to program around this RVCT
> bug. As others pointed out, there are hundreds of call sites like this in
> WebCore.

Darin and Eric, I certainly cannot argue with your points. Since we don't have the compiler fix yet and don't know exactly the circumstances that cause the compiler crash, - sure there might potentially be endless places where the compiler can choke.

Thus, I cannot promise anything, but I know that with this single fix in the current webkit code, I can cleanly build for ARM target all the way through.

Since the compiler error can also be reproduced with the latest version of RVCT, I assume that the bug will be tracked with high attention and we can limit the need potential future fixes of this kind, as webkit grows.

Given this context, I ask you to approve this patch.
Comment 11 Eric Seidel (no email) 2010-01-06 10:46:24 PST
Do we have a link to the compiler bug?  I think that's a bare minimum for this ChangeLog.  I get the impression that we don't know why the compiler is crashing?
Comment 12 Eric Seidel (no email) 2010-01-06 10:50:35 PST
My worries here are:
1. Arbitrary/unnecessary changes to the code for a compiler bug we don't understand.
2. Changes which are incomplete (why does this only affect JSC and not WebCore??).
3. Changes w/o proper tracking back to a compiler bug so we know when we can revert them.
4. Changes which are not sustainable, such that the compiler is going to break again tomorrow because we don't understand the real issues, or because we're not making a proper, easily sustainable work around.

It's very possible that all of the above concerns are invalid and this patch is the right way to go.  But I think Darin and I could use a bit more convincing here. :)  A link to the compiler bug (assuming it's available on a public website) would be a step in the right direction.
Comment 13 Norbert Leser 2010-01-06 10:53:18 PST
(In reply to comment #11)
> Do we have a link to the compiler bug?  I think that's a bare minimum for this
> ChangeLog.  I get the impression that we don't know why the compiler is
> crashing?

Fair request. I don't know at the moment and will try to get a link.
Comment 14 Simon Hausmann 2010-01-07 08:50:35 PST
(In reply to comment #2)
> Created an attachment (id=45714) [details]
> Updated patch for 33084 to fix tab issue
> 
> Fix for accidental tab.

As a build issue this is now tracked in staging/4.6/webkit-bug-33084 at git@gitorious.org:+qtwebkit-developers/webkit/qtwebkit.git and merged into qtwebkit-4.6 in commit 3ad0918ee40f7cf185626ca30906d75f3f46fa6a
Comment 15 Norbert Leser 2010-01-07 10:05:48 PST
(In reply to comment #13)
> (In reply to comment #11)
> > Do we have a link to the compiler bug?  I think that's a bare minimum for this
> > ChangeLog.  I get the impression that we don't know why the compiler is
> > crashing?
> 
> Fair request. I don't know at the moment and will try to get a link.


These are the official bug (Defect/Enhancement) numbers for this issue from ARM Ltd.:

  RVCT 2.2: DE729564.
  RVCT 4.0: DE729513.

However, since this is proprietary code, what tracking is concerned, it appears to be a one-way street, even for us who filed the bug. We may see a reference to the bug once a patch was released.

Hope, this helps in moving this patch forward.
Comment 16 David Leong 2010-01-07 11:59:30 PST
Can we flag the changes for RVCT only since this is a RVCT specific bug?
Comment 17 Darin Adler 2010-01-07 12:40:25 PST
(In reply to comment #16)
> Can we flag the changes for RVCT only since this is a RVCT specific bug?

We could, but I don't think that makes things any better. If these really are the only affected sites, I think the change is OK.
Comment 18 Darin Adler 2010-01-07 12:43:36 PST
Comment on attachment 45714 [details]
Updated patch for 33084 to fix tab issue

I would prefer the name "entry" to the letter "e".

I would prefer that each call site has a brief comment like this one:

    // Use a local variable here to sidestep an RVCT compiler bug.

I withdraw my earlier exception because you say these are the only 4 sites that need to be patched.

r=me as is
Comment 19 Simon Hausmann 2010-01-08 00:54:33 PST
Thanks Darin for the review! Landed with your suggested modifications.

Committed r52978: <http://trac.webkit.org/changeset/52978>