Skip to content

Basic support for Win64 C lib ABI#9387

Merged
bcardiff merged 3 commits intocrystal-lang:masterfrom
oprypin:winabi
Jun 3, 2020
Merged

Basic support for Win64 C lib ABI#9387
bcardiff merged 3 commits intocrystal-lang:masterfrom
oprypin:winabi

Conversation

@oprypin
Copy link
Member

@oprypin oprypin commented May 30, 2020

Also un-does #5851

Based on

rust$ git checkout 29ac04402d53d358a1f6200bea45a301ff05b2d1
rust/src/librustc_trans/trans$ git diff cabi_x86.rs cabi_x86_win64.rs -w --no-index
See diff (and actually all but the last hunk are no-ops!)
diff --git a/cabi_x86.rs b/cabi_x86_win64.rs
index 028d20f30..9b34c3bf2 100644
--- a/cabi_x86.rs
+++ b/cabi_x86_win64.rs
@@ -8,14 +8,14 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use self::Strategy::*;
 use llvm::*;
-use trans::cabi::{ArgType, FnType};
-use trans::type_::Type;
 use super::common::*;
 use super::machine::*;
+use trans::cabi::{ArgType, FnType};
+use trans::type_::Type;
+
+// Win64 ABI: http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx
 
-enum Strategy { RetValue(Type), RetPointer }
 pub fn compute_abi_info(ccx: &CrateContext,
                           atys: &[Type],
                           rty: Type,
@@ -26,35 +26,13 @@ pub fn compute_abi_info(ccx: &CrateContext,
     if !ret_def {
         ret_ty = ArgType::direct(Type::void(ccx), None, None, None);
     } else if rty.kind() == Struct {
-        // Returning a structure. Most often, this will use
-        // a hidden first argument. On some platforms, though,
-        // small structs are returned as integers.
-        //
-        // Some links:
-        // http://www.angelcode.com/dev/callconv/callconv.html
-        // Clang's ABI handling is in lib/CodeGen/TargetInfo.cpp
-
-        let t = &ccx.sess().target.target;
-        let strategy = if t.options.is_like_osx || t.options.is_like_windows {
-            match llsize_of_alloc(ccx, rty) {
-                1 => RetValue(Type::i8(ccx)),
-                2 => RetValue(Type::i16(ccx)),
-                4 => RetValue(Type::i32(ccx)),
-                8 => RetValue(Type::i64(ccx)),
-                _ => RetPointer
-            }
-        } else {
-            RetPointer
+        ret_ty = match llsize_of_alloc(ccx, rty) {
+            1 => ArgType::direct(rty, Some(Type::i8(ccx)), None, None),
+            2 => ArgType::direct(rty, Some(Type::i16(ccx)), None, None),
+            4 => ArgType::direct(rty, Some(Type::i32(ccx)), None, None),
+            8 => ArgType::direct(rty, Some(Type::i64(ccx)), None, None),
+            _ => ArgType::indirect(rty, Some(StructRetAttribute))
         };
-
-        match strategy {
-            RetValue(t) => {
-                ret_ty = ArgType::direct(rty, Some(t), None, None);
-            }
-            RetPointer => {
-                ret_ty = ArgType::indirect(rty, Some(StructRetAttribute));
-            }
-        }
     } else {
         let attr = if rty == Type::i1(ccx) { Some(ZExtAttribute) } else { None };
         ret_ty = ArgType::direct(rty, None, None, attr);
@@ -63,11 +41,12 @@ pub fn compute_abi_info(ccx: &CrateContext,
     for &t in atys {
         let ty = match t.kind() {
             Struct => {
-                let size = llsize_of_alloc(ccx, t);
-                if size == 0 {
-                    ArgType::ignore(t)
-                } else {
-                    ArgType::indirect(t, Some(ByValAttribute))
+                match llsize_of_alloc(ccx, t) {
+                    1 => ArgType::direct(rty, Some(Type::i8(ccx)), None, None),
+                    2 => ArgType::direct(rty, Some(Type::i16(ccx)), None, None),
+                    4 => ArgType::direct(rty, Some(Type::i32(ccx)), None, None),
+                    8 => ArgType::direct(rty, Some(Type::i64(ccx)), None, None),
+                    _ => ArgType::indirect(t, Some(ByValAttribute))
                 }
             }
             _ => {

Currently depends on #9384, so skip the first commit in this review.

oprypin added 2 commits May 30, 2020 18:03
To confirm that the newly added file only moves things:

    $ git diff -w HEAD~:spec/compiler/codegen/c_abi/c_abi_x86_64_spec.cr spec/compiler/codegen/c_abi/c_abi_spec.cr
@oprypin

This comment has been minimized.

@RX14
Copy link
Member

RX14 commented Jun 1, 2020

How was this working with C libs before then?

@oprypin
Copy link
Member Author

oprypin commented Jun 1, 2020

Passing or returning structs never worked. Just that CrSFML never uses either of those so it happened to work, almost everything else doesn't

@oprypin
Copy link
Member Author

oprypin commented Jun 1, 2020

Wait hold on, I didn't realize that Crystal stdlib itself already has many dependencies on external libs so how do they work?

I only saw that libchipmunk didn't work at all without this change and now it does

@oprypin
Copy link
Member Author

oprypin commented Jun 1, 2020

Looks like all those libs also never require passing structs directly (rather than through a pointer)

@jhass
Copy link
Member

jhass commented Jun 1, 2020

Passing a full struct isn't the most common C API indeed IME. Usually you want your caller to allocate them and give you a reference to fill for them. Or you keep the struct to yourself and just give them an opaque reference to pass back to you.

@RX14 RX14 added platform:windows Windows support based on the MSVC toolchain / Win32 API topic:compiler:codegen labels Jun 1, 2020
@straight-shoota straight-shoota added this to the 0.35.0 milestone Jun 2, 2020
@bcardiff bcardiff merged commit 48e9289 into crystal-lang:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform:windows Windows support based on the MSVC toolchain / Win32 API topic:compiler:codegen

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants