Bug 147101 - Unexpected ABI diffference between armv7 and armv7s in WebCore::GraphicsLayerCA::~GraphicsLayerCA()
Summary: Unexpected ABI diffference between armv7 and armv7s in WebCore::GraphicsLayer...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-19 21:39 PDT by David Kilzer (:ddkilzer)
Modified: 2015-07-20 15:10 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (1.52 KB, patch)
2015-07-19 21:56 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2015-07-19 21:39:46 PDT
Unexpected ABI diffference between armv7 and armv7s in WebCore::GraphicsLayerCA::~GraphicsLayerCA() results in two different symbols:

__ZThn568_N7WebCore15GraphicsLayerCAD0Ev
__ZThn576_N7WebCore15GraphicsLayerCAD0Ev

This is due to the following code in Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:

#if CPU(APPLE_ARMV7S) || defined(TRANSFORMATION_MATRIX_USE_X86_64_SSE2)
#if COMPILER(MSVC)
    __declspec(align(16)) typedef double Matrix4[4][4];
#else
    typedef double Matrix4[4][4] __attribute__((aligned (16)));
#endif
#else
    typedef double Matrix4[4][4];
#endif

We should simply use the same alignment on armv7 to fix this.
Comment 1 David Kilzer (:ddkilzer) 2015-07-19 21:40:06 PDT
<rdar://problem/21867029>
Comment 2 David Kilzer (:ddkilzer) 2015-07-19 21:56:28 PDT
Created attachment 257078 [details]
Patch v1
Comment 3 Alex Christensen 2015-07-20 11:51:42 PDT
Comment on attachment 257078 [details]
Patch v1

TransformationMatrix.cpp has a corresponding #elif CPU(APPLE_ARMV7S) which should be changed with this.
Comment 4 David Kilzer (:ddkilzer) 2015-07-20 12:49:26 PDT
(In reply to comment #3)
> Comment on attachment 257078 [details]
> Patch v1
> 
> TransformationMatrix.cpp has a corresponding #elif CPU(APPLE_ARMV7S) which
> should be changed with this.

The assembly in TransformationMatrix.cpp in the #elif CPU(APPLE_ARMV7S) may use instructions that aren't available on non-armv7s platforms.  In that case, we fall back to the C code in the #else clause.

Ben Poulain needs to review this change.  Moving back to r?.
Comment 5 Michael Saboff 2015-07-20 13:26:07 PDT
Comment on attachment 257078 [details]
Patch v1

r=me
Comment 6 Benjamin Poulain 2015-07-20 13:31:50 PDT
+1 from me too.

The ARMV7S in TransformationMatrix.cpp has to stay.
Comment 7 Alex Christensen 2015-07-20 13:32:44 PDT
Comment on attachment 257078 [details]
Patch v1

ok, makes sense.  I just thought it may have been overlooked.
Comment 8 WebKit Commit Bot 2015-07-20 14:21:53 PDT
Comment on attachment 257078 [details]
Patch v1

Clearing flags on attachment: 257078

Committed r187034: <http://trac.webkit.org/changeset/187034>
Comment 9 WebKit Commit Bot 2015-07-20 14:21:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 David Kilzer (:ddkilzer) 2015-07-20 15:10:31 PDT
(In reply to comment #7)
> Comment on attachment 257078 [details]
> Patch v1
> 
> ok, makes sense.  I just thought it may have been overlooked.

It was a good question!  I had overlooked it until you mentioned it, but it turns out we shouldn't change it as noted above.