Skip to content

Add withVaList() for environments without ObjC #339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2015

Conversation

cielavenir
Copy link
Contributor

Currently Linux edition lacks getVaList() and withVaList().
I agree that getVaList() is not available due to memory management model, but I believe that withVaList() is possible.

Sample code is here: http://qiita.com/cielavenir/items/2598d47b97a7c9caf970

@jrose-apple
Copy link
Contributor

va_list is very platform-specific, not just architecture-specific. I suspect that's the real reason it isn't available on Linux yet—the Darwin x86_64 implementation may not be correct there. @gparker42?

@cielavenir
Copy link
Contributor Author

At least I have confirmed that va_list works on Linux x86-64.
https://paiza.io/projects/dXhgyflelzB-WlVLpcr8GA?locale=en-us

@jckarter
Copy link
Contributor

jckarter commented Dec 8, 2015

There are edge cases where OS X diverges from Linux with unions, but I doubt we handle those properly in CVaList on either platform.

@gparker42
Copy link
Contributor

The OS X x86_64 and Linux x86_64 calling conventions are identical or nearly so. I have no idea how va_list itself is stored on Linux but if it operates correctly then it should be good enough.

@cielavenir, you should remove the XFAIL: linux in test 1_stdlib/VarArgs.swift and verify that it runs correctly with your change.

@cielavenir
Copy link
Contributor Author

It is taking too much time to build swift even with 8GB memory... Do you have a way for quick testing...?

@@ -429,4 +430,388 @@ final public class VaListBuilder {

#endif

#else

import CoreFoundation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use CoreFoundation in the standard library, that creates dependency loops.

@gribozavr
Copy link
Contributor

I'm sorry, but the code looks like it was copy-pasted from the other side of the #if branch. May I ask that you factor it in a more fine grained way and avoid massive duplication?

@cielavenir
Copy link
Contributor Author

Although I cannot do so much for VaListBuilder, other parts could be compressed.
By the way, could I load memcpy (not _memcpy) safely? The latter wants Builtin.addressof(), which (I think) is not available.

@gribozavr
Copy link
Contributor

@cielavenir Please use initializeFrom or moveAssignFrom methods instead of memcpy().

@cielavenir
Copy link
Contributor Author

initializeFrom source must be UnsafeMutablePointer, which is not available without Builtin.addressof.

@gribozavr
Copy link
Contributor

Doesn't just &x work as the source?

@gribozavr
Copy link
Contributor

You can also use withUnsafeMutablePointer(x) { memcpy from $0 ... }.

@cielavenir
Copy link
Contributor Author

Thank you. Now I was able to reduce duplication as much as possible.
The next problem is now that VarArgs AutoreleasingUnsafeMutablePointer test must be removed, for Linux does not support it.

args.append(AutoreleasingUnsafeMutablePointer<AnyObject>(
UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674)))
//args.append(AutoreleasingUnsafeMutablePointer<AnyObject>(
// UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove the test. Instead, conditionalize it on #if _runtime(_ObjC).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this? This c_vprintf stuff is exactly duplicate but I cannot avoid now.

#if _runtime(_ObjC)
  args.append(AutoreleasingUnsafeMutablePointer<AnyObject>(
        UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674)))

  // CHECK: {{pointers: '(0x)?0*12345670' '(0x)?0*12345671' '(0x)?0*12345672' '(0x)?0*12345673' '(0x)?0*12345674'}}
  withVaList(args) {
    c_vprintf(format, $0)
  }
#else
  // CHECK: {{pointers: '(0x)?0*12345670' '(0x)?0*12345671' '(0x)?0*12345672' '(0x)?0*12345673'}}
  withVaList(args) {
    c_vprintf(format, $0)
  }
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... except that won't work with FileCheck, it does not know about #if. We need to do something like this:

// RUN: %target-run-stdlib-swift -parse-stdlib %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-runtime

#if _runtime(_ObjC)
... code
// CHECK-objc: ...
#else
... code
// CHECK-native: ...
#endif

@cielavenir
Copy link
Contributor Author

Now I have reverted most stuff and the compilation is still fine. But,

swift/test/1_stdlib/VarArgs.swift:9:8: error: no such module 'CoreFoundation'
import CoreFoundation // vprintf

There is ../swift-corelibs-foundation, so this error is quite strange.

@cielavenir
Copy link
Contributor Author

Note: I promise all commits will be rebased after approval.

@gribozavr
Copy link
Contributor

We can't depend on CoreFoundation in the standard library, that would create circular dependencies.

@cielavenir
Copy link
Contributor Author

OK. How does OSX version achieve vprintf? The Swift module? In Linux, import Swift does not load libc.

Note: import Glibc caused:

/tmp/VarArgs-7905d6.o: In function `_TFF4main9my_printfFtSSGSaPs11CVarArgType___T_U_FVs14CVaListPointerT_':
/tmp/VarArgs-7905d6.o:(.text+0x921): undefined reference to `vprintf'
/tmp/VarArgs-7905d6.o: In function `_TTSf2n_d_i___TFF4main13test_varArgs1FT_T_U_FVs14CVaListPointerT_':
/tmp/VarArgs-7905d6.o:(.text+0x151a): undefined reference to `vprintf'
/tmp/VarArgs-7905d6.o: In function `_TFF4main13test_varArgs3FT_T_U_FVs14CVaListPointerT_':
/tmp/VarArgs-7905d6.o:(.text+0x1f01): undefined reference to `vprintf'
/usr/bin/ld: /home/***/***/swift/build/Ninja-DebugAssert/swift-linux-x86_64/test-linux-x86_64/1_stdlib/Output/VarArgs.swift.tmp/a.out: hidden symbol `vprintf' isn't defined
/usr/bin/ld: final link failed: Bad value
clang-3.8: error: linker command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)

@cielavenir
Copy link
Contributor Author

Uh... This code works... Why test is not working? I'm lost...

$ cat > 0.swift
import Glibc
var n=1
withVaList([n]){vprintf("%d\n",$0)}
$ ../build/Ninja-DebugAssert/swift-linux-x86_64/bin/swift 0.swift
1

@cielavenir cielavenir force-pushed the patch-1 branch 3 times, most recently from 44f677e to 3d81f46 Compare December 11, 2015 10:20
@cielavenir
Copy link
Contributor Author

Now I was able to tweak (the header area of) VarArgs test and have confirmed there are no regressions. So, I carefully rebased commits and now the difference is so small now.

@parinyaprasatsan
Copy link

Good

@gparker42
Copy link
Contributor

Implementation looks good.

The test can be simplified further.

You can get rid of the CHECK complications around AutoreleasingUnsafeMutablePointer by using the same placeholder technique that you used for CGFloat.

c_vprintf() is a historical artifact that can be removed now. CGFloat can be handled using a typealias instead. Do it like this:

#if os(Linux)
import Glibc
typealias CGFloat = Double
#else 
import Darwin
import CoreGraphics
#endif 

Then just use vprintf() and CGFloat everywhere with no other runtime checks. I tested this on the OS X side and it works there.

@cielavenir
Copy link
Contributor Author

Updated according to your advice. Thank you.
However, my development machine has been broken and requires repair. I cannot perform test for a while...

@gribozavr
Copy link
Contributor

Just let us know when you're happy with the patch :)

@cielavenir
Copy link
Contributor Author

Hopefully my machine started up (for now) and I was able to test current patch. It is all right to be merged.

@gparker42
Copy link
Contributor

Looks good. Test passes on OS X. Thanks!

gparker42 added a commit that referenced this pull request Dec 12, 2015
Add withVaList() for environments without ObjC
@gparker42 gparker42 merged commit 1cc2fdd into swiftlang:master Dec 12, 2015
@cielavenir
Copy link
Contributor Author

Thank you so much!

slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
protect unistd.h inclusion with HAVE_UNISTD_H
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
Remove apparently fixed Dollar SourceKit XFAIL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants