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

David Kilzer (:ddkilzer)
Reported 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.
Attachments
Patch v1 (1.52 KB, patch)
2015-07-19 21:56 PDT, David Kilzer (:ddkilzer) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1 2015-07-19 21:40:06 PDT
<rdar://problem/21867029>
David Kilzer (:ddkilzer)
Comment 2 2015-07-19 21:56:28 PDT
Created attachment 257078 [details] Patch v1
Alex Christensen
Comment 3 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.
David Kilzer (:ddkilzer)
Comment 4 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?.
Michael Saboff
Comment 5 2015-07-20 13:26:07 PDT
Comment on attachment 257078 [details] Patch v1 r=me
Benjamin Poulain
Comment 6 2015-07-20 13:31:50 PDT
+1 from me too. The ARMV7S in TransformationMatrix.cpp has to stay.
Alex Christensen
Comment 7 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.
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2015-07-20 14:21:56 PDT
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.