-
Notifications
You must be signed in to change notification settings - Fork 229
FFI support #2572
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
base: master
Are you sure you want to change the base?
FFI support #2572
Conversation
|
I believe these two classes are the only two defined by the C extension, correct? I am taking the approach of trying to mimic the C ext's API but implement what it does via FFI (not yet in this PR). |
|
I've made an attempt to start binding the FFI backend. I had to come up with my own Makefile and figured out generally how to use the API, but I'm still getting a crash when running anything through Help wanted! |
The thing in #2826? |
|
@ParadoxV5 I looked through that and I'm not sure if it fixes anything for me. I suspect it's something I'm not setting up or initializing properly. It fails with either CRuby or JRuby. It can be tested pretty easily by running diff --git a/lib/rbs.rb b/lib/rbs.rb
index d2b04d36..4c78316f 100644
--- a/lib/rbs.rb
+++ b/lib/rbs.rb
@@ -70,8 +70,8 @@ require "rbs/type_alias_regularity"
require "rbs/collection"
begin
- require "rbs_extension"
-rescue LoadError
+# require "rbs_extension"
+# rescue LoadError
require "rbs/ffi/parser"
require "rbs/ffi/location"
endI was trying to follow the implementation of the C extension as implement my FFI-based |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@headius I want to confirm if the code initializes the global constant pool. Line 486 in 088d66a
(And as @ParadoxV5 says, I'm working to delete the global constant pool to make the parser thread safe.) |
|
@soutaro My reading of the code showed that rbs_parser_new calls that function for me on the constant pool it carries. Is there another constant pool I need to initialize? Thread safety is an excellent goal! Happy to help if I can, perhaps by testing it on JRuby once I get past this issue? |
|
@CufeHaco I appreciate your interest but it seems that perhaps you or Claude misunderstood what I am trying to do here. I don't think voltage monitoring has anything to do with memory seg faults which is what I'm running into in the current code. I gave permission for @soutaro to hide your comments so they don't confuse anyone else finding this PR for the first time. |
|
@soutaro I think that might have been it! The following patch appears to fix the crash. I'm unclear why there's a constant pool on both the parser and in a C global, but I'm glad you're working to eliminate it! diff --git a/include/rbs/util/rbs_constant_pool.h b/include/rbs/util/rbs_constant_pool.h
index 30d569f6..81ff085d 100644
--- a/include/rbs/util/rbs_constant_pool.h
+++ b/include/rbs/util/rbs_constant_pool.h
@@ -82,6 +82,7 @@ typedef struct {
} rbs_constant_pool_t;
// A global constant pool for storing permenant keywords, such as the names of location children in `parser.c`.
+RBS_EXPORTED_FUNCTION
extern rbs_constant_pool_t *RBS_GLOBAL_CONSTANT_POOL;
/**
@@ -91,6 +92,7 @@ extern rbs_constant_pool_t *RBS_GLOBAL_CONSTANT_POOL;
* @param capacity The initial capacity of the pool.
* @return Whether the initialization succeeded.
*/
+RBS_EXPORTED_FUNCTION
bool rbs_constant_pool_init(rbs_constant_pool_t *pool, uint32_t capacity);
/**
diff --git a/lib/rbs/ffi/parser.rb b/lib/rbs/ffi/parser.rb
index c8095472..7188af42 100644
--- a/lib/rbs/ffi/parser.rb
+++ b/lib/rbs/ffi/parser.rb
@@ -180,8 +180,12 @@ module RBS
attach_function :rbs_parser_free, [:pointer], :void
attach_function :rbs_parse_type, [:pointer, :pointer, :bool, :bool, :bool], :bool
attach_function :rbs_parse_signature, [:pointer, :pointer], :bool
+ attach_function :rbs_constant_pool_init, [:pointer, :uint32], :bool
+ attach_variable :RBS_GLOBAL_CONSTANT_POOL, :pointer
end
+ Native.rbs_constant_pool_init(Native.RBS_GLOBAL_CONSTANT_POOL, 7)
+
class Parser
def self.new_parser(str, start_pos, end_pos) |
This adds the following: * A very basic Makefile to build a dynamic library from the RBS sources. * Exports for a few of the RBS API functions * Beginning FFI bindings for some of those functions It appears to call into the library correctly, but crashes deep in the process when trying to add to the constant pool.
|
Things are moving along and I've added struct definitions for most of the core types. This may or may not be the best way to use this library from non-C-extension languages but it's a quick way to get it functional. Regardless of how the library's native types and functions are accessed, there's still quite a bit of C extension code to be reproduced in Ruby. The current logic works to parse signatures and get a list of nodes from the RBS parser. This is running on JRuby using just FFI and the dynamic library version of RBS. |
This PR will build out support for loading the RBS library via FFI, avoiding the C extension on implementations that don't support the CRuby extension API.