Bug 33084 – Fix for RVCT -Otime fatal compiler error

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Norbert Leser]

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.

[WebKit Review Bot]

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

[Norbert Leser]

Created attachment 45714 [details]
Updated patch for 33084 to fix tab issue

Fix for accidental tab.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45714 [details] without any errors.

[Antonio Gomes]

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+

[Eric Seidel (no email)]

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.

[Simon Hausmann]

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.

[Norbert Leser]

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.

[Norbert Leser]

Issue can be reproduced with both RVCT 2.2.686 and RVCT 4.0.697.

We have raised case 444841 with ARM.

[Darin Adler]

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.

[Norbert Leser]

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

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Norbert Leser]

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

[Simon Hausmann]

(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 [e-mail]:+qtwebkit-developers/webkit/qtwebkit.git and merged into qtwebkit-4.6 in commit 3ad0918ee40f7cf185626ca30906d75f3f46fa6a

[Norbert Leser]

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

[David Leong]

Can we flag the changes for RVCT only since this is a RVCT specific bug?

[Darin Adler]

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

[Darin Adler]

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

[Simon Hausmann]

Thanks Darin for the review! Landed with your suggested modifications.

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