Win: define Win32 API functions by txe · Pull Request #4832 · crystal-lang/crystal

txe · 2017-08-15T17:49:21Z

I'd like to begin merging source from txe/crystal-win:win with master doing it bit by bit.
This PR adds some basic stuff which will be used by code related to Windows:

Win32 API functions
WinError class

I suppose the file and function naming is controversial so I'm prepared to change the naming if you give appropriate one.

straight-shoota · 2017-08-15T21:06:02Z

src/lib_windows.cr

+
+require "winerror.cr"
+
+data = uninitialized LibWindows::WSAData


Does this need to be a top-level variable?

I will remove these lines because as I see nothing from Winsock dll is used.

RX14 · 2017-08-15T21:24:42Z

src/lib_windows.cr

@@ -0,0 +1,253 @@
+@[CallConvention("X86_StdCall")]


Just a quick question, have we tried without this? If we're using --target x86_64-pc-windows-msvc the default calling convention for everything should be X86_StdCall so it should work without.

RX14 · 2017-08-15T21:25:44Z

src/lib_windows.cr

+  end
+
+  struct WIN32_FIND_DATA_A
+    dwFileAttributes : DWord


Renaming these struct members to be more crystally such as file_attributes here would be great.

No, we should stick to same namings. Reasons:

It's easier to lookup for documention;

Avoids juggling with different names;

Bindings should eventually be generated automatically; we shouldn't have to rewrite anything if/when that happens.

just my 2 cents. I partly agree. Struct names have to be the same due to Microsoft documentation. But struct member naming can be changed because:

otherwise they look alien

it's still easy to find them in documentation due to struct naming

I don't think this binding will be regenerated eventually (because API aren't changed for decades and other reasons). But even so, it would be easy to keep previous changed names during regeneration (it's just simple engineering problem)

And more importantly, what to do with the current function naming, for example:
fun duplicate_handle = DuplicateHandle(...)
Shoud I rename duplicate_handle as DuplicateHandle?
Sorry for long reading.

RX14 · 2017-08-15T22:04:05Z

src/winerror.cr

@@ -0,0 +1,2104 @@
+class WinError < Exception


I deeply disagree with creating a WinError class. Errno is platform-specific, and it needs to go. Exception hierarchy should never depend on the platform you're running on.

I thought about this. The problem is that errno also exists on Windows and it's used for C functions like _open link and Errno class is already working for Windows. So mostly Win32 API functions use GetLastError, "posix" subsystem (some functionality will be used in crystal) uses errno. So how to use one class for these different things simultaneously?

You somewhat abstract system errors or map one to the other.

Ok, I'll try to figure out then

For now mapping to errno will work but we must remove errno before the first release. We should abstract the exception hierarchy to be platform specific. There's a good document on how python did this that we can take inspiration from. Doing that is beyond the scope of this pr though.

ysbaddaden

A few rules I would like this to have, following the the existing platform specific rules for POSIX systems:

Bindings should live under src/lib_c/arch-sys-abi, for example src/lib_c/x86_64-pc-win32/c (I'm not sure of the exact target) and src/i686-pc-win32.
File and directory structure should follow the C headers counterpart (allowing to eventually generate these automatically).
Keep exact naming of structs and fields (just like you did); it simplifies looking for documention.

This is all so such bindings can be generated automatically, make sure each architecture has a specific set of bindings (they usually have tiny differences) and documentation for and definitions in the original C headers can be found quickly and easily.

RX14 · 2017-08-15T23:41:01Z

The correct target is x86_64-pc-windows I think. Some of the current targets are incorrectly normalised. It turns out that target triples are actually messier than I ever thought. For example x86_64-linux-gnu is actually x86_64-pc-linux-gnu with x86_64 being the arch, pc being the vendor, and linux-gnu being the os. Autotools has a 2000 line shell script just to normalise these.

ysbaddaden · 2017-08-16T13:21:47Z

Vendor in target triples is decorative and can be skipped safely, which we're doing in Crystal. We only keep arch-sys-abi. I know that windows must be in the target triple for LLVM to generate a windows PE object (at least when cross compiling). For example the cygwin target is i686-windows-cygnus.

Looking into the rust code, the windows targets seem to be the following, with a translation to GNU targets (for GCC?):

LLVM	GNU
i686-pc-windows-msvc	i686-pc-win32
x86_64-pc-windows-msvc	x86_64-pc-win32
i686-pc-windows-gnu	i686-w64-mingw32
x86_64-pc-windows-gnu	x86_64-w64-mingw32

Since the vendor should be omitted, it would seem the target should be <arch>-windows-msvc? Thought I'm not sure there are API differences between MSVC and MINGW (only ABI differences).

RX14 · 2017-08-16T14:52:42Z

@ysbaddaden The problem is we don't just keep arch-sys-abi, we use a terrible heuristic which deletes the second field if the triple is 4 fields long. In the case of "x86_64-unknown-openbsd" and others like it, we don't skip the vendor. We should, the correct subfolder of lib_c should be "x86_64-openbsd". We should use config.sub from libtool, or something like it, to normalise the target to include the vendor before we delete it. We also don't normalise amd64 to x86_64.

txe · 2017-08-16T15:04:34Z

FYI, in win branch this way is used to find windows target path.

txe · 2017-08-17T09:54:55Z

I'm no sure. Did I fulfill the requests or I need to do something else?

ysbaddaden · 2017-08-17T13:46:09Z

You can't assume windows to mean x86_64-windows-gnu, that's wrong:

it could be i686, arm or aarch64 architectures;
it could be Cygwin (<arch>-windows-cygnus);
it could use MSVC (<arch>-windows-msvc).

In fact, you don't need to change anything in Crystal::CrystalPath#add_target_path, unless the LLVM host target on windows with LLVM compiled with MinGW isn't <arch>-pc-windows-gnu, in which case it should be modified —please tell me what llvm-config --host-target reports.

Now, you can't assume windows to mean the Win32 API too, because it could be Cygwin (<arch>-windows-cygnus). We should instead inject a win32 flag when the triple is sys=windows && abi=(msvc || gnu) in Crystal::Program#parse_flags.

txe · 2017-08-17T15:01:03Z

If I run llvm-config --host-target on Linux on Windows, it reports x86_64-pc-linux-gnu.
There is no native precompiled llvm for Windows, but clang -v reports x86_64-pc-windows-msvc
Also, crystal uses --cross-compile --target x86_64-pc-windows-msvc19.0.0 to build windows binary.

RX14 · 2017-08-17T15:10:15Z

@txe "x86_64-windows-msvc" is the correct triple, but you can't define all triples containing windows to be that.

txe · 2017-08-17T15:53:35Z

I installed MSYS2 and installed LLVM (mingw-w64-x86_64-llvm) there. This version reports --host-target as x86_64-w64-windows-gnu. Maybe I will find time to build LLVM with mingw by myself.

ysbaddaden · 2017-08-17T16:37:37Z

src/winerror.cr

+
+  def winerror_to_errno(winerror)
+    case winerror
+    when    2; return 2


This is a bunch of magical numbers. I have no idea what they mean. Where do the value come from?

There is an undocumented function _dosmaperr. Python uses this functions to generate a mapper. Because the mapper has not be changed for 6 years, I supposed it's quite correct and just copied those numbers.

It would be great if you could replace these numbers with their respective constants.

RX14 · 2017-08-18T08:10:34Z

src/compiler/crystal/semantic/flags.cr

    set.add "freebsd" if set.any?(&.starts_with?("freebsd"))
    set.add "openbsd" if set.any?(&.starts_with?("openbsd"))
    set.add "unix" if set.any? { |flag| %w(cygnus darwin freebsd linux openbsd).includes?(flag) }
+    set.add "win32" if set.any?(&.starts_with?("windows")) && set.any? { |flag| %w(gnu msvc).includes?(flag) }


Why not just use the "windows" flag?

The reason is given in #4832 (comment)

Sorry, I didn't see that.

ysbaddaden

This is starting to look very good.

I just personally wish functions, structs and fields naming would keep their original form: SYSTEMTIME, TIME_ZONE_INFORMATION, wDayOfWeek, ...

The problem are functions, since Windows decided they should be CamelCase... maybe prefix them with an _ or keep them as underscored...

ysbaddaden · 2017-08-18T11:28:14Z

src/winerror.cr

-    when  267; return 20
-    when 1816; return 12
-    else       return Errno::EINVAL
+    when ERROR_FILE_NOT_FOUND           ; return Errno::ENOENT


You don't need semicolons. In fact you don't even need return:

when ERROR_FILE_NOT_FOUND then Errno::ENOENT

ysbaddaden · 2017-08-18T11:33:00Z

src/lib_c/x86_64-windows-msvc/c/winapi.cr

+    daylight_bias : Long
+  end
+
+  struct FileTime


Duplication: there is already FILETIME above.

txe · 2017-08-18T11:39:13Z

The problem are functions, since Windows decided they should be CamelCase... maybe prefix them with an _ or keep them as underscored...

I didn't get it. Could you give an example of what to do?

ysbaddaden

Looks perfect to me!

I believe keeping the original case is important, but I understand that keeping the function names with a leading underscore may not be the best choice. If someone doesn't like that, maybe we can agree on something better. Otherwise, perfect!

txe · 2017-08-18T12:52:49Z

Thanks!

RX14 · 2017-08-18T13:55:12Z

src/winerror.cr

+    buffer = uninitialized UInt8[256]
+    size = LibC._FormatMessageA(LibC::FORMAT_MESSAGE_FROM_SYSTEM, nil, code, 0, buffer, buffer.size, nil)
+    details = String.new(buffer.to_unsafe, size).strip
+    super "#{message}: [WinError #{code}, #{details}]", winerror_to_errno(code)


Just a tiny subjective nitpick, can we have brackets here (super("#{message}: [WinError #{code}, #{details}]", winerror_to_errno(code))), it looks weird without (I actually missed the winerror_to_errno call).

txe · 2017-08-18T13:59:39Z

IO::FileDescriptor is implemented

RX14 · 2017-08-18T14:02:59Z

Where? It's not in this PR.

txe · 2017-08-18T14:08:11Z

It's in my branch, I'm going to make a number of small PRs, so that will help to keep things simple.

sdogruyol · 2017-08-18T14:08:41Z

Can't wait for other PRs 😍

oprypin · 2017-08-30T20:41:53Z

So why did Windows stuff end up in LibC?

txe · 2017-08-30T20:52:41Z

Because everything in /lib_c/.../c/ rests in LibC?
Although I see an advantage to have another module for windows stuff, it would be easy to look for them (for example, during refactoring)

oprypin · 2017-08-30T20:59:56Z

lib_c is really just an unfortunate name for the folder with platform-specific headers. The C standard library is a real thing (and Windows headers are certainly not in it) but the line has been blurred. Oh well, much like with other arguments, this could be seen as out of scope and something that could be changed later instead of delaying progress now.

txe · 2017-08-30T21:29:14Z

I thought a bit about flag :win32. Maybe :windows is enough? I'm just afraid having several 'looking the same' flags will confuse sooner or later. In case cygwin, we can always add flag for it.

txe · 2017-09-07T11:15:55Z

Any progress on approving the PR?

RX14 · 2017-09-07T12:04:56Z

I maintain that we should find a less ugly way of naming these functions than starting with _. If I google get_std_handle I still get docs for GetStdHandle. For struct members, as long as the name is easy to look up it's very easy to work out which name goes where. I don't recommend renaming anything, I recommend slightly cleaning up the names.

asterite · 2017-09-07T20:35:18Z

@txe Even though function names can start with uppercase, that will only happen in the next release. So until then you'll probably have to use the underscore...

This reverts commit f416f6c. revert the previous commit

txe · 2017-09-08T13:19:01Z

So, I suppose, it doesn't make sense to merge this PR until the next release?

asterite · 2017-09-08T13:21:39Z

@txe I think it does make sense. Later we can rename the function names.

RX14

Shall we merge?

RX14 · 2017-09-08T22:23:45Z

🎉

txe added 2 commits August 15, 2017 20:19

function import from Windows shared libs

54c41eb

run file through format tool

6467ece

straight-shoota reviewed Aug 15, 2017

View reviewed changes

RX14 reviewed Aug 15, 2017

View reviewed changes

ysbaddaden requested changes Aug 15, 2017

View reviewed changes

txe mentioned this pull request Aug 16, 2017

Exceptions on windows #4835

Closed

txe added 2 commits August 16, 2017 17:48

put winapi bindings under src/lib_c/x86_64-windows-gnu

b380d2d

WinError is subclass Errno

9ab0fe2

run file through format tool

47669e3

txe added 2 commits August 17, 2017 19:27

x86_64-windows-gnu was renamed x86_64-windows-msvc

27b96fa

injected 'win32' flag

51ef5c6

ysbaddaden reviewed Aug 17, 2017

View reviewed changes

RX14 reviewed Aug 18, 2017

View reviewed changes

replace magic numbers with their respective constants

8501cdc

ysbaddaden reviewed Aug 18, 2017

View reviewed changes

txe added 3 commits August 18, 2017 14:43

removed a duplication

6fa4c19

refactored winerror.cr

7392588

keep exact naming of structs and field as in MSDN

9dc4c97

ysbaddaden approved these changes Aug 18, 2017

View reviewed changes

RX14 reviewed Aug 18, 2017

View reviewed changes

add brackets

9610937

txe mentioned this pull request Aug 30, 2017

Win: add Crystal::System::DirHandle #4903

Closed

asterite mentioned this pull request Sep 7, 2017

Let fun names and method calls start with Uppercase #4934

Merged

fun can start with with an uppercase letter

f416f6c

Revert "fun can start with with an uppercase letter"

00a6232

This reverts commit f416f6c. revert the previous commit

RX14 approved these changes Sep 8, 2017

View reviewed changes

asterite approved these changes Sep 8, 2017

View reviewed changes

RX14 merged commit b87c856 into crystal-lang:master Sep 8, 2017

RX14 added kind:feature topic:stdlib labels Sep 8, 2017

RX14 added this to the Next milestone Sep 8, 2017

sdogruyol mentioned this pull request Sep 9, 2017

[WIP] Windows #3582

Closed

24 tasks

RX14 mentioned this pull request Dec 1, 2017

"hello world" windows port #5339

Merged

straight-shoota mentioned this pull request Dec 21, 2017

Coordinate porting to Windows #5430

Closed

22 tasks

RX14 mentioned this pull request Jan 5, 2018

Implement Crystal::System::Random for win32 #5539

Merged


		require "winerror.cr"

		data = uninitialized LibWindows::WSAData

Uh oh!

Conversation

txe commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txe Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txe Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

RX14 commented Aug 15, 2017

Uh oh!

ysbaddaden commented Aug 16, 2017

Uh oh!

RX14 commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

txe commented Aug 16, 2017

Uh oh!

txe commented Aug 17, 2017

Uh oh!

ysbaddaden commented Aug 17, 2017

Uh oh!

txe commented Aug 17, 2017

Uh oh!

RX14 commented Aug 17, 2017

Uh oh!

txe commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txe Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txe commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

txe Aug 15, 2017 •

edited

Loading

txe Aug 15, 2017 •

edited

Loading

RX14 Aug 15, 2017 •

edited

Loading

RX14 commented Aug 16, 2017 •

edited

Loading

txe Aug 17, 2017 •

edited

Loading

txe commented Aug 18, 2017 •

edited

Loading

RX14 Aug 18, 2017 •

edited

Loading

oprypin commented Aug 30, 2017 •

edited

Loading