search

SwiftAndroid / swift

Port of Apple's reference Swift toolchain to Android; doesn't quite work yet
Apache License 2.0
720 stars 32 forks source link

1_stdlib/VarArgs.swift #15

Closed modocache closed 8 years ago

modocache commented 8 years ago 
******************** TEST 'Swift :: 1_stdlib/VarArgs.swift' FAILED ********************
Script:
--
rm -rf /home/modocache/GitHub/apple/build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7/1_stdlib/Output/VarArgs.swift.tmp && mkdir -p /home/modocache/GitHub/apple/build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7/1_stdlib/Output/VarArgs.swift.tmp && /home/modocache/GitHub/apple/build/Ninja-ReleaseAssert/swift-linux-x86_64/bin/swiftc -target armv7-none-linux-androideabi -Xlinker --allow-shlib-undefined -Xlinker -pie  -module-cache-path '/tmp/swift-testsuite-clang-module-cache0D5zbw'   -module-cache-path '/tmp/swift-testsuite-clang-module-cache0D5zbw' /home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift -o /home/modocache/GitHub/apple/build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7/1_stdlib/Output/VarArgs.swift.tmp/a.out -module-name main -Xfrontend -disable-access-control && /home/modocache/GitHub/apple/swift/test/../../swift-android-extras/rundroid /home/modocache/GitHub/apple/build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7/1_stdlib/Output/VarArgs.swift.tmp/a.out -parse-stdlib /home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift | FileCheck /home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift
--
Exit Code: 1

Command Output (stderr):
--
/home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift:15:25: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(arguments) {
                        ^
/home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift:39:20: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(args) {
                   ^
/home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift:63:20: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(args) {
                   ^

test/1_stdlib/VarArgs.swift fails to compile when targeting android-armv7. @hpux735 tells me it also fails for Linux armv6/armv7.

I found the error message hard to understand. cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _' -- huh? So I made the following change:

diff --git a/test/1_stdlib/VarArgs.swift b/test/1_stdlib/VarArgs.swift
index 2791711..6e537f0 100644
--- a/test/1_stdlib/VarArgs.swift
+++ b/test/1_stdlib/VarArgs.swift
@@ -12,8 +12,8 @@ typealias CGFloat = Double
 #endif

 func my_printf(format: String, _ arguments: CVarArgType...) {
-  withVaList(arguments) {
-    vprintf(format, $0)
+  withVaList(arguments) { (vaList: CVaListPointer) in
+    vprintf(format, vaList)
   }
 }

This results in the following error message, which is much easier to understand:

/home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift:16:21: error: cannot convert value of type 'CVaListPointer' to expected argument type '__va_list'
    vprintf(format, vaList)
                    ^~~~~~

Aha! So vprintf expects a __va_list, but we're giving it a CVaListPointer.

As far as I understand, it's lib/ClangImporter's responsibility to convert certain raw C types like __va_list to CVaListPointer. Sure enough, there are a few commits related to va_list:

  1. https://github.com/apple/swift/commit/2b6bee7fb8063161ecb3365abf31eb2d2d93a18b -- Imports C va_list as CVaListPointer. The author struggles with behavior on arm vs. x86_64--perhaps the same sort of problem we're seeing here?
  2. https://github.com/apple/swift/commit/9d74d25e50290d3cdeff5b30ecb72aa77e627ddb -- "va_list on i386 and arm64 is a pointer. On armv7, it's a struct that /contains/ a pointer. But on x86_64, it's an array of one struct element, which means it decays to a pointer as an argument but not as a variable or struct field." The patch cleans up the va_list import implementation.

It appears that on android-armv7 and linux-armv7, va_list is not being imported as CVaListPointer, and as a result our vprintf function takes a __va_list instead of a CVaListPointer.

Just for fun I tried adding the following mapping:

diff --git a/lib/ClangImporter/MappedTypes.def b/lib/ClangImporter/MappedTypes.def
index 474a78b..ccf4530 100644
--- a/lib/ClangImporter/MappedTypes.def
+++ b/lib/ClangImporter/MappedTypes.def
@@ -126,6 +126,7 @@ MAP_STDLIB_TYPE("u_int64_t", UnsignedInt, 64, "UInt64", false, DoNothing)
 // stdarg.h types.
 // FIXME: why does this not catch va_list on x86_64?
 MAP_STDLIB_TYPE("va_list", VaList, 0, "CVaListPointer", false, DoNothing)
+MAP_STDLIB_TYPE("__va_list", VaList, 0, "CVaListPointer", false, DoNothing)

 // libkern/OSTypes.h types.
 MAP_STDLIB_TYPE("UInt", UnsignedInt, 32, "CUnsignedInt", false, DoNothing)

This actually gets 1_stdlib/VarArgs.swift to compile, but it fails with the following error:

/home/modocache/GitHub/apple/swift/test/1_stdlib/VarArgs.swift:21:12: error: expected string not found in input
 // CHECK: The answer to life and everything is 42, 42, -42, 3.14
           ^
<stdin>:1:1: note: scanning from here
The answer to life and everything is 42, 42, -42, 0.000000
^

Anyway, something is wrong with va_list importing on android-armv7 and linux-armv7, but I'm not sure what yet.

jrose-apple commented 8 years ago

va_list is part of an architecture's calling conventions, so it's important to do whatever's expected on that architecture. I'm a little surprised the existing mapping didn't work, but the one you're adding should be fine.

For the actual arguments being passed incorrectly, you'll need to make sure that the logic in stdlib/public/core/VarArgs.swift actually makes sense for your platform. In this case that's probably the AAPCS, although I'm surprised that either Apple or Linux diverged here.

modocache commented 8 years ago

Awesome, thanks @jrose-apple! I'll try adding the mapping and double-checking the logic in stdlib/public/core/VarArgs.swift. :bow:

modocache commented 8 years ago

The following gets this test passing for me:

diff --git a/lib/ClangImporter/MappedTypes.def b/lib/ClangImporter/MappedTypes.def
index 474a78b..77e4620 100644
--- a/lib/ClangImporter/MappedTypes.def
+++ b/lib/ClangImporter/MappedTypes.def
@@ -126,6 +126,7 @@ MAP_STDLIB_TYPE("u_int64_t", UnsignedInt, 64, "UInt64", false, DoNothing)
 // stdarg.h types.
 // FIXME: why does this not catch va_list on x86_64?
 MAP_STDLIB_TYPE("va_list", VaList, 0, "CVaListPointer", false, DoNothing)
+MAP_STDLIB_TYPE("__va_list", VaList, 0, "CVaListPointer", false, DoNothing)

 // libkern/OSTypes.h types.
 MAP_STDLIB_TYPE("UInt", UnsignedInt, 32, "CUnsignedInt", false, DoNothing)
diff --git a/stdlib/public/core/VarArgs.swift b/stdlib/public/core/VarArgs.swift
index ce4ee0e..83f5a7e 100644
--- a/stdlib/public/core/VarArgs.swift
+++ b/stdlib/public/core/VarArgs.swift
@@ -290,7 +290,7 @@ final public class VaListBuilder {
     // supported vararg type is greater than the alignment of Int.
     // FIXME: this implementation is not portable because
     // alignof differs from the ABI alignment on some architectures
-#if os(watchOS) && arch(arm)   // FIXME: rdar://21203036 should be arch(armv7k)
+#if (os(watchOS) || os(Android)) && arch(arm)   // FIXME: rdar://21203036 should be arch(armv7k)
     if let arg = arg as? _CVarArgAlignedType {
       let alignmentInWords = arg._cVarArgAlignment / sizeof(Int)
       let misalignmentInWords = count % alignmentInWords

android-armv7 is soft float, and I'm assuming that watchos-armv7k is as well--I think that's the real issue here. So instead of checking for #if os(watchOS) && arch(arm), we should probably check for something like #if arch(arm) && floattype(hard). Is something like floattype available? Should it be added? @jrose-apple

I think the work @hpux735 is doing on hard float, though... so I'm curious what sort of failure output you're seeing, @hpux735. Is it the compilation error from above? What happens if you add the __va_list mapping?

hpux735 commented 8 years ago

@modocache This is what I get from test/1_stdlib/VarArgs.swift on a linux-armv7 with hard float (GNUEABIHF)

FAIL: Swift :: 1_stdlib/VarArgs.swift (111 of 2471)
******************** TEST 'Swift :: 1_stdlib/VarArgs.swift' FAILED ********************
Script:
--
rm -rf /mnt/temp/Ninja-ReleaseAssert/swift-linux-armv7/test-linux-armv7/1_stdlib/Output/VarArgs.swift.tmp && mkdir -p /mnt/temp/Ninja-ReleaseAssert/swift-linux-armv7/test-linux-armv7/1_stdlib/Output/VarArgs.swift.tmp && /home/wdillon/swift-build/build/Ninja-ReleaseAssert/swift-linux-armv7/bin/swiftc -target armv7-unknown-linux-gnueabihf  -module-cache-path '/tmp/swift-testsuite-clang-module-cacheZiExII'   -module-cache-path '/tmp/swift-testsuite-clang-module-cacheZiExII' /home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift -o /mnt/temp/Ninja-ReleaseAssert/swift-linux-armv7/test-linux-armv7/1_stdlib/Output/VarArgs.swift.tmp/a.out -module-name main -Xfrontend -disable-access-control &&  /mnt/temp/Ninja-ReleaseAssert/swift-linux-armv7/test-linux-armv7/1_stdlib/Output/VarArgs.swift.tmp/a.out -parse-stdlib /home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift | FileCheck /home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift
--
Exit Code: 1

Command Output (stderr):
--
/home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift:15:25: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(arguments) {
                        ^
/home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift:39:20: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(args) {
                   ^
/home/wdillon/swift-build/swift/test/1_stdlib/VarArgs.swift:63:20: error: cannot convert value of type '(_) -> Int32' to expected argument type 'CVaListPointer -> _'
  withVaList(args) {
                   ^

--

********************
modocache commented 8 years ago

@hpux735 Awesome, thank you! And what if you apply the following patch?

```diff
diff --git a/lib/ClangImporter/MappedTypes.def b/lib/ClangImporter/MappedTypes.def
index 474a78b..77e4620 100644
--- a/lib/ClangImporter/MappedTypes.def
+++ b/lib/ClangImporter/MappedTypes.def
@@ -126,6 +126,7 @@ MAP_STDLIB_TYPE("u_int64_t", UnsignedInt, 64, "UInt64", false, DoNothing)
 // stdarg.h types.
 // FIXME: why does this not catch va_list on x86_64?
 MAP_STDLIB_TYPE("va_list", VaList, 0, "CVaListPointer", false, DoNothing)
+MAP_STDLIB_TYPE("__va_list", VaList, 0, "CVaListPointer", false, DoNothing)

 // libkern/OSTypes.h types.
 MAP_STDLIB_TYPE("UInt", UnsignedInt, 32, "CUnsignedInt", false, DoNothing)

My theory is that the above patch will get your test to compile and pass.

jrose-apple commented 8 years ago

I'm not sure if floattype is something we want as a first-class predicate just yet, but maybe if we underscored it (_floattype) for now. That's a discussion for a separate PR on the main Swift repo, though.

(I think you also meant "watchOS uses hard float". At least, that's what I see from reading the LLVM target info; I can't find official documentation on developer.apple.com, and I don't actually know myself.)

modocache commented 8 years ago

After further investigation we're beginning to think this isn't a hard float vs. soft float issue. android-armv7 is soft float, whereas linux-armv7 is hard float, but both have the same problem.

We're thinking that actually iOS and tvOS don't conform to AAPCS, so those are the only ARM-based systems that won't need this check. Every other ARM-based system will.

The solution we're going with is going to be adding va_list mappings for each platform and adding os(Linux) || os(Android) to the watchOS os check. Alternatively, it might be a good idea to check !os(iOS) && !os(tvOS) -- but we can discuss in a pull request.

jrose-apple commented 8 years ago

Oh, good catch! Indeed that is in Apple's docs and I just breezed over it.

The function calling conventions used in the ARMv6 environment are the same as those used in the Procedure Call Standard for the ARM Architecture (release 1.07), with the following exceptions:

  • Large data types (larger than 4 bytes) are 4-byte aligned.
  • …

There's no 32-bit tvOS, so we shouldn't need to worry about that. Feel free to submit a pull request upstream.

jrose-apple commented 8 years ago

It looks like the arm64/aarch64 conventions are different too, if/when that becomes interesting.

modocache commented 8 years ago

@jrose-apple https://github.com/apple/swift/pull/1271 :+1: