Mac/iOS support, including aarch64 port

[zwaldowski]

Supersedes #19.

As before: rewrites the Xcode project, as referenced in issue #28. Includes fixes for ARM-32 targets under newer LLVM assemblers and uses unified syntax.

Integrates AArch64 support for this year's crop of iOS devices. I'd like to see some regression testing from the ARM/Linaro folk against legitimate AArch64 devices. It's working for me, but the changes were mostly workarounds for compiler errors aped from the ARM port.

[a2]

👍

[atgreen]

Thanks @zwaldowski ! I'll merge this and ping the Linaro guys to test. I've also prepared a ChangeLog entry. It would be great if you could include one in the future.

[atgreen]

So the ARM guys confirmed build problems and test regressions on Linux with this patch. They're looking at it.

[jgreenhalgh-arm]

This patch series is not OK as it stands and will need significant reworking to fix. I strongly suggest that it is reverted immediately.

libffi is too fundamental and bugs introduced can be too subtle for the changes in this patch series to be applied without deep scrutiny.

To pick up some of the most obvious issues:

• No consideration for differing ABIs.
  The iOS ABI Function Call Guide defines a number of points at which iOS' calling conventions differ from ARM's Procedure Call Standard for the ARM 64-bit Architecture. These differences are not reflected in the patch series. This code is not safe even on the platform for which it was developed.

• Fundamentally broken code
  This patch:
  zwaldowski@9775446
  Can never work on any machine.

  You change the loop variable from i to j, but don't update the i++ or the use of i within the loop.

  If this code is ever triggered it will hang, doing the wrong thing forever.

  This patch:
  zwaldowski@c713a55

  Takes a 3-argument macro and changes it to a 0-argument macro, without fixing up the call-sites of the macro. In particular, it leaves the call at line 414 broken. Worse, the changes you make fundamentally change the behaviour of the macro in this case, removing the register restore of r0-r3.

• Nonsense Whitespace changes
  These fall in to two categories, those that are just churn and those which are actively destructive. Both are innapropriate outside of dedicated whitespace cleanup patches, and are only borderline acceptable there.

  The principal problem we are aiming to avoid is "not being able to see the wood for the trees". Take this patch for example:
  zwaldowski@16ba1b8
  If you look at the changes to src/x86/ffi.c, the one interesting change is line 45, the other 21 changes are noise.

  At the other side we have destructive whitespace changes, take this patch:
  zwaldowski@9775446
  Here, code is reformatted out of line with the surrounding code.

  If you wish to clean up whitespace the changes must be correct and they should appear in their own patch.

• "Hacks"
  Silencing compiler warnings by adding explicit casts is totally wrong. The compiler is warning us because we are doing something it considers inappropriate. It is not correct to just over-ride that - especially without detailed comment as to why the change is more appropriate than actually fixing the issue or at least providing the warning you are masking.

  I'm thinking about changes such as changing a char to an int (why!?), adding redundant casts between types, explicitly downcasting a 64-bit value to a 32-bit value. If we are potentially causing problems for ourself that the compiler has been kind enough to warn us about we should be paying attention and fixing the broken code, not hiding it.

• Unhelpful commit messages
  Things like:
  "Darwin/aarch64: Potentially(?) fix compile error"
  Are not OK. What is the error? Why is this change correct? Why is the author unsure?

  Likewise:
  "Darwin/aarch64: double == long double" is throughly undescriptive of the change you are proposing.

I therefore believe that a revert is the only sensible response here, to reiterate:

• This patch series breaks ARM support with the GNU toolchain.
• It causes regressions on AArch64.
• It is not reviewable in its current form due to erroneous whitespace changes and lack of detail from the author.
• It is fundamentally broken in at least two key areas, making me very suspicious about the series as a whole.
• It is filled with hacks in a key area of the stack which we cannot afford to add hacks to.
• It does not take in to consideration the differences between the procedure Call Standard for the ARM 64-bit Architecture and Apple's ARM64 Function Calling Conventions. The ABI is fundamentally different, in particular in terms of consecutive argument calling conventions.
• As it stands this patch series breaks two functioning targets and adds a new, broken target.

Anthony, please revert this patch series, Zachary, if you would like to see this code merged, please respin the series taking in to consideration the points above.

[zwaldowski]

Yes, @atgreen, please revert if that makes sense for now.

[atgreen]

I've reverted the patch.

I would really like to get this port in there, unfortunately I don't have a Mac or any ios devices. Are those really prerequisites, or can you build a cross-compilation environment from Linux and test on an emulator?

[zwaldowski]

They are prerequisites, unfortunately. I've already fixed the more boneheaded issues @jgreenhalgh-arm was nice enough to point out and am reading up on what's necessary to more correctly match the divergent ABIs.

[jgreenhalgh-arm]

Thanks @atgreen.

@zwaldowski I'm willing to review patches to the AArch64 port before they go in, though I'm not an expert on the iOS ABI so I won't be much help there.

I'm also not an expert at setting GitHub up to notify me that I have some review to do, so if you were willing to ping me at james.greenhalgh@arm.com - or post on the libffi mailing list - if you want review feedback that would be great.