Bug 129822 - REGRESSION(r165196): broke arm64 on hardware (Requested by bfulgham on #webkit).
Summary: REGRESSION(r165196): broke arm64 on hardware (Requested by bfulgham on #webkit).
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: WebKit Commit Bot
URL:
Keywords: InRadar
Depends on:
Blocks: 129806
  Show dependency treegraph
 
Reported: 2014-03-06 13:13 PST by WebKit Commit Bot
Modified: 2014-03-07 14:36 PST (History)
3 users (show)

See Also:


Attachments
ROLLOUT of r165196 (14.26 KB, patch)
2014-03-06 13:14 PST, WebKit Commit Bot
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Commit Bot 2014-03-06 13:13:55 PST
http://trac.webkit.org/changeset/165196 broke the build:
broke arm64 on hardware (Requested by bfulgham on #webkit).

This is an automatic bug report generated by webkitbot. If this bug
report was created because of a flaky test, please file a bug for the flaky
test (if we don't already have one on file) and dup this bug against that bug
so that we can track how often these flaky tests fail.
Comment 1 WebKit Commit Bot 2014-03-06 13:14:09 PST
Created attachment 226030 [details]
ROLLOUT of r165196

Any committer can land this patch automatically by marking it commit-queue+.  The commit-queue will build and test the patch before landing to ensure that the rollout will be successful.  This process takes approximately 15 minutes.

If you would like to land the rollout faster, you can use the following command:

  webkit-patch land-attachment ATTACHMENT_ID

where ATTACHMENT_ID is the ID of this attachment.
Comment 2 Brent Fulgham 2014-03-06 13:15:17 PST
Not committing yet. Trying this change locally to see if it indeed corrects the arm64 failure on hardware.
Comment 3 Brent Fulgham 2014-03-06 14:26:06 PST
This was the culprit.
Comment 4 WebKit Commit Bot 2014-03-06 14:28:01 PST
Comment on attachment 226030 [details]
ROLLOUT of r165196

Clearing flags on attachment: 226030

Committed r165216: <http://trac.webkit.org/changeset/165216>
Comment 5 WebKit Commit Bot 2014-03-06 14:28:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2014-03-06 21:20:13 PST
This makes no sense.  The original patch, as well as the rollout, has no functional effect on the system unless you enable the FTL.  The FTL is not enabled on ARM64.  When the FTL is enabled, this patch fixes bugs.

Tests pass with this patch applied.

This feels like a fluke.  This rollout is blocking further work on the FTL, and I cannot reproduce the crash.
Comment 7 Brent Fulgham 2014-03-06 22:43:38 PST
(In reply to comment #6)
> This makes no sense.  The original patch, as well as the rollout, has no functional effect on the system unless you enable the FTL.  The FTL is not enabled on ARM64.  When the FTL is enabled, this patch fixes bugs.
> 
> Tests pass with this patch applied.
> 
> This feels like a fluke.  This rollout is blocking further work on the FTL, and I cannot reproduce the crash.

Let's avoid relying on feelings, and deal in  observable behavior. The patch reproducibly broke mobile safari, and rolling out the patch cleared the problem.

Perhaps the failure is not observable using the JSC test suite, or perhaps is not observable in simulated environments. Perhaps our default build is somehow enabling FTL when building for hardware.

If tests did pass on hardware, there must be significant behavior that is not modeled by the test suite.
Comment 8 Filip Pizlo 2014-03-07 08:34:55 PST
(In reply to comment #7)
> (In reply to comment #6)
> > This makes no sense.  The original patch, as well as the rollout, has no functional effect on the system unless you enable the FTL.  The FTL is not enabled on ARM64.  When the FTL is enabled, this patch fixes bugs.
> > 
> > Tests pass with this patch applied.
> > 
> > This feels like a fluke.  This rollout is blocking further work on the FTL, and I cannot reproduce the crash.
> 
> Let's avoid relying on feelings, and deal in  observable behavior. The patch reproducibly broke mobile safari, and rolling out the patch cleared the problem.
> 
> Perhaps the failure is not observable using the JSC test suite, or perhaps is not observable in simulated environments. Perhaps our default build is somehow enabling FTL when building for hardware.
> 
> If tests did pass on hardware, there must be significant behavior that is not modeled by the test suite.

I don't doubt that there is something wrong. I cannot delve deeper into it because I'm on vacation. What I do know is that:

- The patch changes code that was added for the FTL and my understanding of the code is that it shouldn't be in use if the FTL is disabled. If it is in use, then something is very suspicious. 
- I tested on arm64 hardware with and without FTL. It's true that I only ran the JSC test suite.
Comment 9 Brent Fulgham 2014-03-07 09:13:36 PST
<rdar://problem/16251450>
Comment 10 Michael Saboff 2014-03-07 13:09:36 PST
(In reply to comment #9)
> <rdar://problem/16251450>

I applied change set http://trac.webkit.org/changeset/165196 to ToT r165273 and tried both a debug and release build on an ARM64 device and had no problems with crashing on http://daringfireball.net or various other common sites.

Brent is going to build with the change set and see if it reproduces for him.  I'm going to prepare a patch to roll the change back in.
Comment 11 Brent Fulgham 2014-03-07 14:36:41 PST
(In reply to comment #10)
> (In reply to comment #9)
> > <rdar://problem/16251450>
> 
> I applied change set http://trac.webkit.org/changeset/165196 to ToT r165273 and tried both a debug and release build on an ARM64 device and had no problems with crashing on http://daringfireball.net or various other common sites.
> 
> Brent is going to build with the change set and see if it reproduces for him.  I'm going to prepare a patch to roll the change back in.

I did a full rebuild after updating to today's tool, and doing a full clean install of the OS image (i.e., not an "upgrade") and everything works.