Bug 129822 – REGRESSION(r165196): broke arm64 on hardware (Requested by bfulgham on #webkit).

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

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.

[Brent Fulgham]

Not committing yet. Trying this change locally to see if it indeed corrects the arm64 failure on hardware.

[Brent Fulgham]

This was the culprit.

[WebKit Commit Bot]

Comment on attachment 226030 [details]
ROLLOUT of r165196

Clearing flags on attachment: 226030

Committed r165216: <http://trac.webkit.org/changeset/165216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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.

[Brent Fulgham]

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

[Filip Pizlo]

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

[Brent Fulgham]

<rdar://problem/16251450>

[Michael Saboff]

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

[Brent Fulgham]

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