Bug 49112 – Fix Accelerate.framework issues in VectorMath 32-bit ppc & i386 vs. other architectures (64-bit, ARM, etc.)

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Fix 32-bit vs. 64-bit Accelerate.framework issues in VectorMath

[Chris Rogers]

Created attachment 73139 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 73139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73139&action=review

What is the effect of this patch? Does the current code fail to build on 32-bit, or does it build but behave differently when changed this way? Is there a reason we can't use the same code path for both 32-bit and 64-bit platforms?

Also of note: ARM is a 32-bit platform not covered by your macros. Is it ok for it to go down the same code path as 64-bit? I think either your macro test is wrong (if ARM should be included with 32-bit)) or the ChangeLog is wrong (if the issue is specific to just i386 and ppc, rather than all 32-bit platforms).

> WebCore/platform/audio/VectorMath.cpp:41
> +// In 32-bit mode __ppc__ or __i386__ is defined and <vecLib/vDSP_translate.h> is included which defines macros, so we must handle this case differently.

This comment does not seem useful. It says the 32-bit case is handled differently, but does not say why.

[Chris Rogers]

Maciej, thanks for having a look.  The current code builds on 32-bit but goes into a loop and crashes when vadd() calls back into itself recursively because of a macro which is defined only in the __ppc__ and __i386__ case.  The vDSP_translate.h file defines these macros, but only gets included in the 32-bit case. 

Even though my vadd() and vsmul() functions are in a namespace, the macro screws things up, and it seems unfortunate that Accelerate.h is defining these kinds of macros.

The ARM case should be OK, since I'm using exactly the same compile-time conditional as the framework is using.

[Chris Rogers]

Created attachment 73261 [details]
Patch

[Chris Rogers]

Changed name of this bug and uploaded a new patch with a more descriptive comment.

[Kenneth Russell]

Comment on attachment 73261 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 73261 [details]
Patch

Clearing flags on attachment: 73261

Committed r71877: <http://trac.webkit.org/changeset/71877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.