Skip to content
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

Porting to cygwin #1108

Merged
merged 3 commits into from Feb 23, 2016

Conversation

Projects
None yet
5 participants
@tinysun212
Copy link
Contributor

commented Jan 26, 2016

Rebased PR from previous.
I made several changes set for cygwin64 port.

The standard library can be used and It is possible to compile and run or interpret a simple hello swift source, but REPL, swift-build and swift-lldb does not work.
Currently, build process is not good, and many bugs may exist in.

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2016

I'll made more patches for stdlib/public/stubs/Stubs.cpp, stdlib/public/stubs/UnicodeNormalization.cpp,
include/swift/Basic/Dwarf.h.

@gribozavr gribozavr referenced this pull request Jan 27, 2016

Closed

Porting to cygwin #1010


// On Cygwin, std::once_flag can not be used because it is larger than the
// platform word.
typedef unsigned long swift_once_t;

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

uintptr_t might be a better fit?

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Jan 28, 2016

Author Contributor

It seems other implementations (Linux, Cygwin internal.. ) didn't use a pointer type, so I chosen that.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 1, 2016

Member

We don't want a pointer, we want a pointer-sized int.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 2, 2016

Author Contributor

I misunderstand about uintptr_t. I'll change to it. Thank you.

// A wrapper function is introduced.
struct PrivPair {
var negative : UInt
var positive : UInt

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Please remove spaces before the colons in var declarations.

// If the return type of a C/C++ function is a structure,
// the calling convension is different from caller's.
// A wrapper function is introduced.
struct PrivPair {

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

internal struct _PrivPair.

What does PrivPair stand for?

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Jan 28, 2016

Author Contributor

just means a pair that used in this place..

@warn_unused_result
func swift_class_getInstanceExtents(theClass: AnyClass)
-> (negative: UInt, positive: UInt) {
var pair = PrivPair(negative : 0, positive : 0)

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Please drop the spaces before the colons.

#if os(Windows)
// FIXME: This fix is for 'crash at hook(output.left)' in cygwin.
// Proper fix is needed. see: https://bugs.swift.org/browse/SR-612
let _playgroundPrintHook : ((String)->Void)? = nil

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Please drop the space before the colon.

_addImageTypeMetadataRecordsBlock(records, recordsSize);

dlclose(handle);
return 0;

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Could we simplify the code to this:

if (records) {
  _addImageTypeMetadataRecordsBlock(records, recordsSize);
}
dlclose(handle);
return 0;
@@ -34,6 +37,10 @@ static_assert(std::is_same<swift_once_t, dispatch_once_t>::value,
static_assert(sizeof(swift_once_t) <= sizeof(void*),
"swift_once_t must be no larger than the platform word");

#if defined(__CYGWIN__)
static std::mutex mutex_;

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Please use a more descriptive name. For example, swiftOnceMutex.

@@ -41,6 +48,18 @@ void swift::swift_once(swift_once_t *predicate, void (*fn)(void *)) {
#if defined(__APPLE__)
dispatch_once_f(predicate, nullptr, fn);
#else
#if defined(__CYGWIN__)

mutex_.lock();

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

Please add a fixme (and file a bug) that notes that this implementation does a global lock, which is much worse than what we have on other platforms. Each swift_once should synchronize on the token.

size_t size, void *data),
void *data);
uint8_t *_swift_getSectionDataPE(void *handle, const char *section_name,
unsigned long *section_size);

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

sectionName and sectionSize.

@@ -332,6 +333,31 @@ static int _addImageProtocolConformances(struct dl_phdr_info *info,
dlclose(handle);
return 0;
}
#elif defined(__CYGWIN__)
static int _addImageProtocolConformances(struct dl_phdr_info *info,

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 27, 2016

Collaborator

This looks almost identical to the other piece of code above. Could we somehow factor it to avoid duplication?

This comment has been minimized.

Copy link
@modocache

modocache Jan 28, 2016

Collaborator

That would be great! The Android armv7 port (not yet upstream-able) also has this sort of duplication:

It would be awesome if this could be factored so that it's easier for all ports. 👍

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 6, 2016

Author Contributor

In my opnion, there are three kinds of platform specific implementation factors in performing '_initializeCallbacksToInspectDylib()'.

First, the function for seaching loaded dls/images/DLLs is diffrent for every platform.
The functions are _dyld_register_func_for_add_image(), dl_iterate_phdr(), android_iterate_libs(), _swift_dl_iterate_phdr(). Some of these are supported by system library (Apple , Linux) and others are implemented for porting (Android, Cygwin).
For that reason, there are conditional preprocessing in two '_initializeCallbacksToInspectDylib()'.

Second, the callback function which calling by the searching function is different.
Although the callback function name is same for each platform, the prototype is different. The reason why is because the different searching function specifies different prototype of callback.

Finally, the method for inspecting a section of dl/image/DLL is different.
This method is directly supported by system library getsectiondata() in Apple, or implemented with POSIX dlsym() and linker script in Linux/Android, or implemented with accessing PE/COFF structure in Cygwin. Except Apple, they all use POSIX dlopen()/dlclose() for accessing a dl/image/DLL.

I think it is most important to generalize two '_initializeCallbacksToInspectDylib()' from where the duplication is begining for every platform.

How about introducing general function _swift_inspectDylibs() as follows?

a sketch : unify.cpp

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 12, 2016

Author Contributor

@modocache , @gribozavr , How about this snippet code unify.cpp for dylib callbacks?

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 12, 2016

Collaborator

@tinysun212 That sounds like a good abstraction approach to me!

const unsigned DWARFVersion = 3;
#endif

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 28, 2016

Collaborator

Please avoid #if in the compiler source. Could you make this an IRGen option, for example?

@@ -40,4 +40,8 @@ def host_target():
if machine == 'amd64':
return 'freebsd-x86_64'

elif system == 'CYGWIN_NT-10.0':

This comment has been minimized.

Copy link
@gribozavr

gribozavr Jan 28, 2016

Collaborator

Just curious, what is the 10.0 version? Does it change with, say, Windows releases?

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 1, 2016

Author Contributor

I used Windows 10. Yes, It depend on Windows releases. I found https://en.wikipedia.org/wiki/Uname .

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Jan 28, 2016

@tinysun212 Thank you for your updates! I made more comments, please take a look.

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Jan 28, 2016

This is looking really good now, by the way!

#if os(Windows)
// If the return type of a C/C++ function is a structure,
// the calling convension is different from caller's.
// A wrapper function is introduced.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 1, 2016

Member

Can you change TwoWordPair::Return in the runtime to be an __int128 on Win64 instead of exposing the C calling convention difference in Swift code?

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 2, 2016

Author Contributor

I'll try it.

function(context);
} else
swiftOnceMutex.unlock();
}

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 1, 2016

Member

Does Cygwin's libstdc++ not have a std::call_once implementation we can use?

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 2, 2016

Collaborator

It has one, but its once flag is two words long.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 2, 2016

Author Contributor

Gcc with libstdc++ has good std::call_once. But clang didn't work for std::call_once. The main reason is clang does not support the TLS feature in Cygwin and libstdc++'s call_once needed TLS. I used an work around code for it which is in namespace std and sizeof(once_flag) was 16. So using above code _swift_once_f is better.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 2, 2016

Member

I see. It'd be nice to fix that in clang eventually, but I guess this is OK as a workaround.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 15, 2016

Member

Is its once_flag statically zero-initialized? We could make IRGen's OnceTy the right target-dependent size.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 16, 2016

Author Contributor

Does it mean that we can use std::once_flag of two words size in Cygwin, if IRGen is changed together ?
(The problem can be ignored that clang with Cygwin runtime library need a patch to use the std::call_once. I think it will be solved in the future or there is a workaround.)

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 16, 2016

Member

Yeah, if IRGen generates OnceTy as the same LLVM type as std::once_flag then that should work.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 16, 2016

Member

We can try that later, though, I don't want to hold this up longer.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 17, 2016

Author Contributor

If you ok, I want to do this on other PR, soon or after to this PR. I think the smaller PR, the better.

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 18, 2016

Member

Agreed.

#if __arm__ || __i386__
#if __arm__ || __i386__ || defined(__CYGWIN__)
#if defined(__CYGWIN__)
using Return = unsigned __int128;

This comment has been minimized.

Copy link
@jckarter

jckarter Feb 6, 2016

Member

Can you use enum class Return : unsigned __int128, or is that not supported? It'd be good to avoid implicit conversions from raw integer types.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 6, 2016

Author Contributor

I encountered an error when I first tried same as yours, so committed with 'using' keyword. Just, I retried it again, no error occurred at this time. It works fine now. Perhaps I missed something when I commit.
I'll replace the 'using' to 'enum' syntax, thank you.

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2016

I'm waiting reviews for correction. If there is nothing to correct, what should I do with this PR?

@gribozavr

View changes

stdlib/public/stubs/Stubs.cpp Outdated
ValueStream.imbue(std::locale::classic());
ValueStream << Value;
std::string ValueString(ValueStream.str());
int i = ValueString.length();

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 12, 2016

Collaborator

The variable should be declared as size_t instead of a cast on the line below.

@gribozavr

View changes

stdlib/public/stubs/Stubs.cpp Outdated
@@ -98,7 +107,7 @@ extern "C" uint64_t swift_uint64ToString(char *Buffer, intptr_t BufferLength,
/*Negative=*/false);
}

#if defined(__APPLE__) || defined(__FreeBSD__)
#if defined(__APPLE__) || defined(__FreeBSD__) || defined(__CYGWIN__)
static inline locale_t getCLocale() {

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 12, 2016

Collaborator

I think if you add an exclusion for Cygwin and avoid defining this function on Cygwin, you can avoid having to define typedef void *locale_t; above.

@gribozavr

View changes

stdlib/public/stubs/Stubs.cpp Outdated
// These are wrapper functions only for C locale.
static long double strtold_l(const char *nptr, char **EndPtr, locale_t) {
std::string lastLocale = setlocale(LC_ALL, "");
setlocale(LC_ALL, "C");

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 12, 2016

Collaborator

Isn't this setlocale call affecting the whole app? Possibly changing the behavior for other threads?

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 12, 2016

Author Contributor

Oh, It is affecting all threads. I'll add _configthreadlocale(_ENABLE_PER_THREAD_LOCALE) somewhere soon.

This comment has been minimized.

Copy link
@tinysun212

tinysun212 Feb 12, 2016

Author Contributor

I incorrectly knew about the function _configthreadlocale(). It was a MSVC runtime library, not Cygwin. Maybe I should use the formatting feature of istream with locale.

@tinysun212 tinysun212 force-pushed the tinysun212:porting-to-cygwin branch Feb 13, 2016

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2016

This PR conflicted again. I don't know how to resolve this. Should I rebase to top of master and recreate another PR ?

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2016

@tinysun212 You can rebase, squash, and git push -f to the same branch just one commit. Github will automatically update the PR. Alternatively, you can submit a new PR, whichever is simpler.

@tinysun212 tinysun212 force-pushed the tinysun212:porting-to-cygwin branch Feb 13, 2016

@tinysolver

This comment has been minimized.

Copy link

commented Feb 13, 2016

I rebased and squashed all previous commits to one. And fixed some compile errors. Thank you.

@gribozavr

View changes

stdlib/public/runtime/CygwinPort.cpp Outdated

int lastRet = 0;

// HMODULE modules[1024];

This comment has been minimized.

Copy link
@gribozavr

gribozavr Feb 13, 2016

Collaborator

Remove commented code?

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2016

@swift-ci Please test

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2016

@tinysun212 Would you mind fixing the CMake issues that the buildbot found? You'd need to use set(LLVM_OPTIONAL_SOURCES ...) for the appropriate files.

@tinysun212 tinysun212 force-pushed the tinysun212:porting-to-cygwin branch Feb 18, 2016

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2016

@swift-ci Please test

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2016

Currently, I have Linux and Windows platforms for development and not OS X platform.
Could somebody tell me the point that cause the test failed or any hint to solve this ?

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2016

@tinysun212 Sorry, the OS X tests failing is not your fault. The master branch is broken now, we are working to get it fixed. This should land imminently and we'll re-run tests then.

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2016

@swift-ci Please test

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@tinysun212 I'm sorry, could you fix the conflicts? The patch LGTM, I will merge immediately!

@tinysun212 tinysun212 force-pushed the tinysun212:porting-to-cygwin branch to e06c713 Feb 22, 2016

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@tinysun212 Thank you!

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@swift-ci Please smoke test

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@tinysun212 CI found an issue on Linux:

CMake Error at /home/buildnode/jenkins/workspace/swift-PR-Linux@2/Ninja-ReleaseAssert/llvm-linux-x86_64/share/llvm/cmake/LLVMProcessSources.cmake:83 (message):
  Found unknown source file
  /home/buildnode/jenkins/workspace/swift-PR-Linux@2/swift/stdlib/public/runtime/CygwinPort.cpp

To fix, please merge the two definitions of LLVM_OPTIONAL_SOURCES into one (file stdlib/public/runtime/CMakeLists.txt).

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@swift-ci Please smoke test

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2016

@tinysun212 Sorry, CI is not happy again. You need to merge the definition into the top one, because there is an add_swift_library in between.

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2016

@gribozavr I'm sorry repetitive fail in same position. In this time, I tested the cmake-running (not build) in Linux.

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 23, 2016

@swift-ci Please test

@tinysun212

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2016

OS X tests failed again. I think it is not my fault. Let me know if my action is needed.

@gribozavr

This comment has been minimized.

Copy link
Collaborator

commented Feb 23, 2016

@tinysun212 Yes, that's existing breakage. Merging! Thank you for working on this!

gribozavr added a commit that referenced this pull request Feb 23, 2016

@gribozavr gribozavr merged commit 7235595 into apple:master Feb 23, 2016

1 of 2 checks passed

Swift Test OS X Platform Build finished. 16014 tests run, 0 skipped, 249 failed.
Details
Swift Test Linux Platform Build finished. 7966 tests run, 0 skipped, 0 failed.
Details

@modocache modocache referenced this pull request Feb 25, 2016

Merged

Port to Android #1442

@tinysun212 tinysun212 deleted the tinysun212:porting-to-cygwin branch Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.