Add Rust support #37

MaloJaffre · 2018-05-07T19:58:23Z

This reuses part of the C generator, by using a C++ <=> C <=> Rust FFI.
The linking is done with whole-archive to properly merge C/C++ and Rust static libraries.

testall.sh succeeds on my machine, and I've manually verified the correctness of the generated prologin2017 environment.
Related to prologin/sadm#102.

zopieux · 2018-05-07T21:20:46Z

Woaw, that's a nice contribution, thanks! We'll look into it as soon as possible.

MaloJaffre · 2018-05-11T10:47:24Z

I've now updated this PR with a new generic struct Array<T> and a method free to handle arrays more ergonomically than with the previous arrays type_array of the C API.

seirl · 2018-05-11T11:14:35Z

tools/generator/files/rules.mk


 ld_flags      = $(_LDFLAGS)
-cmd_ld_shared = $(CXX) $(filter %.o %.a,$^) $(ld_flags) -shared -o $@ $(_LDLIBS)
+ifeq ($(STECHEC_LANG),rust)
+  cmd_ld_shared = $(CXX) -Wl,--whole-archive $(filter %.o %.a,$^) \


Can't all of those go directly into $ldflags?

The problem is the -Wl,--whole-archive argument which is required to be before the libraries linked.
I could add a pre-ld-flags variable to generalize this, but I'm not very proficient in Makefiles writing, and this is a very special case of cmd_ld_shared.

I think LDFLAGS is supposed to be before the objects, and what goes after is in LDLIBS. Can you still compile the other champions if you just put $ldflags before $(filter...)?

I've tested and it works for every language, except php and haskell, because I've not managed to build them.

seirl · 2018-05-11T11:18:09Z

tools/generator/gen/generator_c.rb

-  def build
-    @path = Pathname.new($install_path) + "c"
+  def build(lang = "c")
+    @path = Pathname.new($install_path) + lang


I agree that the design here isn't ideal, but I think it would still be a bit more elegant to subclass CCxxFileGenerator and override build() there.

seirl · 2018-05-11T11:20:42Z

tools/generator/test/champions/prologin.rs

+
+/// Called 10K times to test if things work well.
+#[no_mangle]
+pub unsafe extern "C" fn test()


Ideally I'd think it would be great to have a wrapper that does the extern "C" and the demangling, and those wrappers would call the user functions, which I think would be a bit prettier (and less boilerplate for the contestant). If you don't have time to do it, that's fine, but that would be a good improvement.

seirl · 2018-05-11T11:22:07Z

I added a few comments of things that could be better. Just so you know, we're definitely planning on testing that for next week's finals.

This reuses part of the C generator, by using a C++ <=> C <=> Rust FFI. The linking is done with whole-archive to properly merge C/C++ and Rust static libraries. Related to prologin/sadm#102.

MaloJaffre · 2018-05-11T12:23:02Z

@seirl I think I've now addressed all the comments made so far.

Diff with previous version:

diff --git a/tools/generator/files/rules.mk b/tools/generator/files/rules.mk
index 4e77b20..914ce32 100644
--- a/tools/generator/files/rules.mk
+++ b/tools/generator/files/rules.mk
@@ -183,13 +183,7 @@ cmd_ghc     = $(GHC) -pgml $(CXX) -O9 -dynamic --make -shared -fPIC \
 cmd_rustc   = $(RUSTC) $(rustc_flags) $< -o $@
 
 ld_flags      = $(_LDFLAGS)
-ifeq ($(STECHEC_LANG),rust)
-  cmd_ld_shared = $(CXX) -Wl,--whole-archive $(filter %.o %.a,$^) \
-                  -Wl,--no-whole-archive -lm -lrt -ldl -lpthread -lstdc++ \
-                  -shared -fPIC -o $@ $(_LDLIBS)
-else
-  cmd_ld_shared = $(CXX) $(filter %.o %.a,$^) $(ld_flags) -shared -o $@ $(_LDLIBS)
-endif
+cmd_ld_shared = $(CXX) $(ld_flags) $(filter %.o %.a,$^) $(_LDLIBS) -shared -o $@
 
 cmd_clean     = $(RM) $(_cleanfiles)
 cmd_distclean = $(RM) $(_dcleanfiles)
diff --git a/tools/generator/gen/generator_c.rb b/tools/generator/gen/generator_c.rb
index 0a397ce..b7cc5a1 100644
--- a/tools/generator/gen/generator_c.rb
+++ b/tools/generator/gen/generator_c.rb
@@ -232,8 +232,8 @@ EOF
     @f.close
   end
 
-  def build(lang = "c")
-    @path = Pathname.new($install_path) + lang
+  def build
+    @path = Pathname.new($install_path) + "c"
     @source_file = 'interface.cc'
     @header_file = 'interface.hh'
 
diff --git a/tools/generator/gen/generator_rust.rb b/tools/generator/gen/generator_rust.rb
index 72c9e16..863cedf 100644
--- a/tools/generator/gen/generator_rust.rb
+++ b/tools/generator/gen/generator_rust.rb
@@ -35,6 +35,17 @@ def rust_proto(fn)
   "fn #{fn.name}(#{args})" + ret
 end
 
+class CCxxFileGeneratorForRust < CCxxFileGenerator
+  def build
+    @path = Pathname.new($install_path) + "rust"
+    @source_file = 'interface.cc'
+    @header_file = 'interface.hh'
+
+    generate_source
+    generate_header
+  end
+end
+
 class RustFileGenerator < FileGenerator
   def print_multiline_comment(str, prestr = '')
     str.each_line {|s| @f.print prestr, "// ", s }
@@ -116,18 +127,23 @@ EOF
       @f.print "pub ", rust_proto(f), ";\n"
     end
     @f.puts "}", ""
+    for_each_user_fun do |f|
+      @f.puts"#[no_mangle]"
+      @f.print "pub unsafe extern \"C\" ", rust_proto(f)
+      args = f.args.map { |arg| arg.name }.join(", ")
+      print_body "  super::#{f.name}(#{args})"
+    end
     @f.close
   end
 
   def generate_user
     @f = File.open(@path + @user_file, 'w')
     @f.puts "#![allow(unused)]"
-    @f.puts "mod api;"
+    @f.puts "pub mod api;"
     @f.puts "use api::*;"
     @f.puts "use std::os::raw::{c_double, c_int, c_void};", ""
     for_each_user_fun do |f|
-      @f.puts"#[no_mangle]"
-      @f.print "pub extern \"C\" ", rust_proto(f)
+      @f.print rust_proto(f)
       print_body "  // fonction a completer"
     end
     @f.close
@@ -148,6 +164,9 @@ lib_TARGETS = #{target}
 # Evite de toucher a ce qui suit
 #{target}-dists += #{$conf['conf']['player_filename']}.h interface.hh #{@api_file}
 #{target}-srcs += interface.cc #{@user_file}
+#{target}-ldflags = -fPIC -Wl,--whole-archive
+#{target}-ldlibs  = -Wl,--no-whole-archive -lm -lrt -ldl -lpthread -lstdc++
+
 STECHEC_LANG=rust
 include ../includes/rules.mk
     EOF
@@ -156,7 +175,7 @@ include ../includes/rules.mk
 
 
   def build()
-    CCxxFileGenerator.new.build("rust")
+    CCxxFileGeneratorForRust.new.build
 
     @path = Pathname.new($install_path) + "rust"
     @user_file = $conf['conf']['player_filename'] + '.rs'
diff --git a/tools/generator/test/champions/prologin.rs b/tools/generator/test/champions/prologin.rs
index 2dee6b5..4440852 100644
--- a/tools/generator/test/champions/prologin.rs
+++ b/tools/generator/test/champions/prologin.rs
@@ -1,12 +1,11 @@
 #![allow(bad_style, unused)]
-mod api;
+pub mod api;
 use api::*;
 use std::os::raw::{c_double, c_int, c_void};
 use std::slice::from_raw_parts;
 
 /// Called 10K times to test if things work well.
-#[no_mangle]
-pub unsafe extern "C" fn test()
+unsafe fn test()
 {
     assert!(CONST_VAL/4 == 10);
     assert!(CONST_DOUBLE_2/2. == 668.5);

seirl · 2018-05-11T12:26:04Z

Thanks, looking into that. Btw, I'd much rather have multiple commits that we squash when merging the PR than diffs in the comments ;-)

seirl · 2018-05-11T12:32:27Z

I think you're missing the makefile_rust.rb?

MaloJaffre · 2018-05-11T12:37:06Z

@seirl Thank you very much for your review, and I'll try not to squash commits in the future.

It seems that makefile_c.rb is not used for example for generating a client in C, because the code is directly in generator_c.rb, but I maybe miss something?

seirl · 2018-05-11T12:41:51Z

Yeah, there are two different makefiles that we need to have. The one you're refering to is used for local matches, but the server needs makefile_XXX to generate its own makefile:

stechec2/tools/generator/generator.rb

Lines 119 to 145 in e1c6e61

    
           def make_server 
        
             $languages.each do |x| 
        
               require "gen/makefile_" + x 
        
             end 
        
             # create directory 
        
             install_path = Pathname.new($install_path) 
        
             install_path.mkpath 
        
             CMakefile.new.build_metaserver(install_path) 
        
             CSharpMakefile.new.build_metaserver(install_path) 
        
             CxxMakefile.new.build_metaserver(install_path) 
        
             JavaMakefile.new.build_metaserver(install_path) 
        
             # PascalMakefile.new.build_metaserver(install_path) 
        
             CamlMakefile.new.build_metaserver(install_path) 
        
             HaskellMakefile.new.build_metaserver(install_path) 
        
             # LuaMakefile.new.build_metaserver(install_path) 
        
             PythonMakefile.new.build_metaserver(install_path) 
        
             PhpMakefile.new.build_metaserver(install_path) 
        
             # PrologMakefile.new.build_metaserver(install_path) 
        
             # RubyMakefile.new.build_metaserver(install_path) TODO 
        
             # JsMakefile.new.build_metaserver(install_path) TODO 
        
             # copy some used files 
        
             path = Pathname.new(PKGDATADIR) + "files" 
        
             FileUtils.cp((path + "toposort.py").to_s, install_path.to_s) 
        
             FileUtils.cp((path + "rules.mk").to_s, install_path.to_s) 
        
           end

MaloJaffre · 2018-05-11T12:58:40Z

@seirl Thanks for this hint, I've now pushed a new commit adding that file, but I was unable to find a way to test it.

seirl · 2018-05-11T20:45:23Z

This looks good, I'll try to test that for you from my local setup, don't worry.

seirl · 2018-05-13T18:36:11Z

Merging to ease internal testing, we'll iterate from this if we find any issues.

MaloJaffre · 2018-05-13T18:57:42Z

@seirl Thanks again for your review!

Namarand · 2018-05-14T14:47:17Z

@MaloJaffre All API functions are currently unsafe, making them quite painful to use. One solution is to wrap all function calls in an unsafe block, allowing the user to use directly the functions. Does this look good to you? If yes, I'll do it asap.

MaloJaffre · 2018-05-14T16:42:53Z

@Namarand Good idea!
Everything in the API is actually safe, excepting free and all function that takes an Array<T> (ptr must be pointing to a valid allocation of len eleements).
Apart from this detail, this wrapping looks good to me.
You could rename the api module to ffi and put the nice wrapping in api to cleanly separate the exposed API from the internal implementation.

seirl · 2018-05-14T16:47:12Z

@MaloJaffre I really don't like the Array implementation. I think the wrapper function should copy the array in a native rust array, we really don't want a free().

MaloJaffre · 2018-05-14T16:53:56Z

@seirl @Namarand I don't like the rawness of the exposed API neither, so I will send a PR to add a nice wrapping (with ffi and api modules) asap.

seirl · 2018-05-14T16:59:52Z

@MaloJaffre I think the main problem is that you used the C api, so instead of doing a nice conversion C++ -> Rust it does C++ -> C -> Rust, so you have to go through all the hacks that are made just to have usable arrays in C. I would like it a lot more if you could just populate your Rust arrays from the C++ vectors. Do you think that's doable?

MaloJaffre · 2018-05-14T17:14:46Z

@seirl I will try to do it by directly allocating a Rust Vec<T> in C++, but the interface will still pass trough extern "C" functions because C++ mangling is not implemented in rustc

seirl · 2018-05-14T17:15:51Z

It's not a problem to pass through extern "C" functions, but I just don't think you should use the C generator. The C++ generator already contains those unmangled function exports.

Namarand · 2018-05-14T17:18:50Z

Actually, https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html does quite a good job for converting C array to slice. Maybe we can use that ? We can also convert them to Vec<> thanx to the to_vec() method.

MaloJaffre · 2018-05-14T17:31:30Z

@seirl I will rework the API to avoid the use of the C generator asap.
@Namarand Good idea, I will definitely use this function in the rewrite.

As discussed in prologin#37. This doesen't yet eliminate the C layer, because it seems very difficult to allocate and construct in C++ a `Vec<T>` (its layout is unspecified so that its fields can be reordered by rustc, and it is incompatible with malloced buffers), but all these horrible `Array<T>` are now hidden in a new `ffi` module. Nevertheless, this greatly simplifies the API usage, as seen in the `prologin.rs` test.

As discussed in #37. This doesen't yet eliminate the C layer, because it seems very difficult to allocate and construct in C++ a `Vec<T>` (its layout is unspecified so that its fields can be reordered by rustc, and it is incompatible with malloced buffers), but all these horrible `Array<T>` are now hidden in a new `ffi` module. Nevertheless, this greatly simplifies the API usage, as seen in the `prologin.rs` test.

MaloJaffre force-pushed the add-rust branch 4 times, most recently from aab2b54 to 4187789 Compare May 11, 2018 10:37

seirl reviewed May 11, 2018

View reviewed changes

Add Rust support

c2c4422

This reuses part of the C generator, by using a C++ <=> C <=> Rust FFI. The linking is done with whole-archive to properly merge C/C++ and Rust static libraries. Related to prologin/sadm#102.

MaloJaffre force-pushed the add-rust branch from 4187789 to c2c4422 Compare May 11, 2018 12:22

Add makefile-rust.rb

b4b9f1b

MaloJaffre force-pushed the add-rust branch from d50690e to b4b9f1b Compare May 11, 2018 13:00

seirl merged commit f48b54e into prologin:master May 13, 2018

MaloJaffre deleted the add-rust branch May 13, 2018 18:57

MaloJaffre mentioned this pull request May 15, 2018

Wrap the Rust FFI in an idiomatic API #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rust support #37

Add Rust support #37

MaloJaffre commented May 7, 2018

zopieux commented May 7, 2018

MaloJaffre commented May 11, 2018

seirl May 11, 2018

MaloJaffre May 11, 2018

seirl May 11, 2018

MaloJaffre May 11, 2018

seirl May 11, 2018

seirl May 11, 2018

seirl commented May 11, 2018

MaloJaffre commented May 11, 2018

seirl commented May 11, 2018

seirl commented May 11, 2018

MaloJaffre commented May 11, 2018

seirl commented May 11, 2018 •

edited

Loading

MaloJaffre commented May 11, 2018 •

edited

Loading

seirl commented May 11, 2018

seirl commented May 13, 2018

MaloJaffre commented May 13, 2018

Namarand commented May 14, 2018 •

edited

Loading

MaloJaffre commented May 14, 2018 •

edited

Loading

seirl commented May 14, 2018

MaloJaffre commented May 14, 2018

seirl commented May 14, 2018

MaloJaffre commented May 14, 2018

seirl commented May 14, 2018

Namarand commented May 14, 2018

MaloJaffre commented May 14, 2018 •

edited

Loading

Add Rust support #37

Add Rust support #37

Conversation

MaloJaffre commented May 7, 2018

zopieux commented May 7, 2018

MaloJaffre commented May 11, 2018

seirl May 11, 2018

Choose a reason for hiding this comment

MaloJaffre May 11, 2018

Choose a reason for hiding this comment

seirl May 11, 2018

Choose a reason for hiding this comment

MaloJaffre May 11, 2018

Choose a reason for hiding this comment

seirl May 11, 2018

Choose a reason for hiding this comment

seirl May 11, 2018

Choose a reason for hiding this comment

seirl commented May 11, 2018

MaloJaffre commented May 11, 2018

seirl commented May 11, 2018

seirl commented May 11, 2018

MaloJaffre commented May 11, 2018

seirl commented May 11, 2018 • edited Loading

MaloJaffre commented May 11, 2018 • edited Loading

seirl commented May 11, 2018

seirl commented May 13, 2018

MaloJaffre commented May 13, 2018

Namarand commented May 14, 2018 • edited Loading

MaloJaffre commented May 14, 2018 • edited Loading

seirl commented May 14, 2018

MaloJaffre commented May 14, 2018

seirl commented May 14, 2018

MaloJaffre commented May 14, 2018

seirl commented May 14, 2018

Namarand commented May 14, 2018

MaloJaffre commented May 14, 2018 • edited Loading

seirl commented May 11, 2018 •

edited

Loading

MaloJaffre commented May 11, 2018 •

edited

Loading

Namarand commented May 14, 2018 •

edited

Loading

MaloJaffre commented May 14, 2018 •

edited

Loading

MaloJaffre commented May 14, 2018 •

edited

Loading