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.
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
Created attachment 45714 [details] Updated patch for 33084 to fix tab issue Fix for accidental tab.
style-queue ran check-webkit-style on attachment 45714 [details] without any errors.
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 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.
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.
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.
Issue can be reproduced with both RVCT 2.2.686 and RVCT 4.0.697. We have raised case 444841 with ARM.
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.
(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.
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?
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.
(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.
(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
(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.
Can we flag the changes for RVCT only since this is a RVCT specific bug?
(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 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
Thanks Darin for the review! Landed with your suggested modifications. Committed r52978: <http://trac.webkit.org/changeset/52978>