Bug 147101

Summary: Unexpected ABI diffference between armv7 and armv7s in WebCore::GraphicsLayerCA::~GraphicsLayerCA()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Layout and RenderingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, commit-queue, dino, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 none

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.