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

Win: define Win32 API functions #4832

Merged
merged 14 commits into from Sep 8, 2017
Merged

Win: define Win32 API functions #4832

merged 14 commits into from Sep 8, 2017

Conversation

txe
Copy link
Contributor

@txe txe commented Aug 15, 2017

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.


require "winerror.cr"

data = uninitialized LibWindows::WSAData
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a top-level variable?

Copy link
Contributor Author

@txe txe Aug 15, 2017

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

end

struct WIN32_FIND_DATA_A
dwFileAttributes : DWord
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we should stick to same namings. Reasons:

  1. It's easier to lookup for documention;
  2. Avoids juggling with different names;
  3. Bindings should eventually be generated automatically; we shouldn't have to rewrite anything if/when that happens.

Copy link
Contributor Author

@txe txe Aug 15, 2017

Choose a reason for hiding this comment

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

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.

src/winerror.cr Outdated
@@ -0,0 +1,2104 @@
class WinError < Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try to figure out then

Copy link
Contributor

@RX14 RX14 Aug 15, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

RX14 commented Aug 15, 2017

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.

@txe txe mentioned this pull request Aug 16, 2017
@ysbaddaden
Copy link
Contributor

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
Copy link
Contributor

RX14 commented Aug 16, 2017

@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
Copy link
Contributor Author

txe commented Aug 16, 2017

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

@txe
Copy link
Contributor Author

txe commented Aug 17, 2017

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

@ysbaddaden
Copy link
Contributor

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
Copy link
Contributor Author

txe commented Aug 17, 2017

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
Copy link
Contributor

RX14 commented Aug 17, 2017

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

@txe
Copy link
Contributor Author

txe commented Aug 17, 2017

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.

src/winerror.cr Outdated

def winerror_to_errno(winerror)
case winerror
when 2; return 2
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@txe txe Aug 17, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -27,6 +27,7 @@ class Crystal::Program
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) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use the "windows" flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is given in #4832 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't see that.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

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...

src/winerror.cr Outdated
when 267; return 20
when 1816; return 12
else return Errno::EINVAL
when ERROR_FILE_NOT_FOUND ; return Errno::ENOENT
Copy link
Contributor

Choose a reason for hiding this comment

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

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

when ERROR_FILE_NOT_FOUND then Errno::ENOENT

daylight_bias : Long
end

struct FileTime
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplication: there is already FILETIME above.

@txe
Copy link
Contributor Author

txe commented Aug 18, 2017

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?

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

txe commented Aug 18, 2017

Thanks!

src/winerror.cr Outdated
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)
Copy link
Contributor

@RX14 RX14 Aug 18, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

txe commented Aug 18, 2017

IO::FileDescriptor is implemented

@RX14
Copy link
Contributor

RX14 commented Aug 18, 2017

Where? It's not in this PR.

@txe
Copy link
Contributor Author

txe commented Aug 18, 2017

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

@sdogruyol
Copy link
Member

Can't wait for other PRs 😍

@oprypin
Copy link
Member

oprypin commented Aug 30, 2017

So why did Windows stuff end up in LibC?

@txe
Copy link
Contributor Author

txe commented Aug 30, 2017

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
Copy link
Member

oprypin commented Aug 30, 2017

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
Copy link
Contributor Author

txe commented Aug 30, 2017

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
Copy link
Contributor Author

txe commented Sep 7, 2017

Any progress on approving the PR?

@RX14
Copy link
Contributor

RX14 commented Sep 7, 2017

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
Copy link
Member

asterite commented Sep 7, 2017

@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
Copy link
Contributor Author

txe commented Sep 8, 2017

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

@asterite
Copy link
Member

asterite commented Sep 8, 2017

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

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Shall we merge?

@RX14 RX14 merged commit b87c856 into crystal-lang:master Sep 8, 2017
@RX14
Copy link
Contributor

RX14 commented Sep 8, 2017

🎉

@RX14 RX14 added this to the Next milestone Sep 8, 2017
@sdogruyol sdogruyol mentioned this pull request Sep 9, 2017
24 tasks
@RX14 RX14 mentioned this pull request Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants