Introducing the C API #279

Kiyoshika · 2023-11-25T01:30:36Z

This introduces the first set of C bindings for gosub which, at the moment, just exposes the render tree that we can use in Swift to render the text on the screen.

Ideally, we will build out an "engine API" and wrap that so the user agents can use, instead of using gosub-internal data structures, but right now this builds the foundation for the C API.

EDIT: unsafe code has been completely moved out of the core engine and instead a wrapper has been introduced in the bindings to convert between engine structs and C-friendly structs.
I'm trying to keep as much unsafe code out of the engine as possible, but in some cases this is unavoidable to keep the structs compatible with the C interface. We may be able to move this stuff out of the engine at some point.

Currently the only known problem with the C API is that we are leaking memory from the DocumentHandle held by the RenderTree. I'm currently investigating this but unfortunately can't find the root cause after trying various different ways to "free" this memory. We may have to use something other than a DocumentHandle to get around it... not sure yet

After this is merged, I'd like to write some docs on the gosub wiki / github wiki about how to use the C API "safely" which will also be useful to others building native clients (the main consumers of the API)

src/render_tree.rs

Kiyoshika · 2023-11-25T01:34:35Z

src/render_tree.rs

@@ -152,7 +172,7 @@ pub struct Node {
 impl Node {
    pub fn new() -> Self {
        Self {
-            node_type: NodeType::Root,
+            node_type: NodeType::Root(true),


This is a dummy value that needs to be included otherwise it seems like it's ignored going from Rust -> C

src/render_tree.rs

Kiyoshika · 2023-11-25T01:37:24Z

src/render_tree.rs

@@ -330,107 +349,4 @@ mod tests {
            panic!()
        }
    }
-
-    #[test]
-    fn text_nodes() {


This test is moved to gosub-bindings/tests

Not sure if you want to have this tests only over the C API

yeah I was on the fence about this... my thought was the main purpose of this portion of the test was more relevant for the C API, and it might be good to have both tests but then I'd have to maintain two versions of the same test, and I'd rather have the C API be right (which would also imply the render tree implementation is correct.) But I can see the argument for having it in the render tree itself as well

src/render_tree/text.rs

Sharktheone

I don't really like the usage of mut ptrs in the engine itself. I think, there must be a other alternative to deal with this. Maybe you can add other struct variants for example CTextNode and convert it to this. But I'm not sure, what the performance would say about this.

gosub-bindings/src/lib.rs

Sharktheone · 2023-11-25T14:01:45Z

gosub-bindings/src/lib.rs

+///
+/// Moves an owning pointer to the render_tree using Box::into_raw() to the C API.
+/// This pointer MUST be passed to gosub_render_tree_free() after usage for proper cleanup.
+pub unsafe extern "C" fn gosub_render_tree_init(html: *const c_char) -> *mut RenderTree {


For now, this is okay, but i guess we want this most likely to be a URL and not the HTML document, so we can use the browser's dns system

Yeah, I would want to move to the DNS system eventually, but was not available at the time I wrote this

Are we guaranteed that the Swift / Objective C side will never free this *const c_char pointer while we're in Rust code?

Sharktheone · 2023-11-25T14:06:23Z

gosub-bindings/src/lib.rs

+    chars.set_confidence(Confidence::Certain);
+
+    let doc = DocumentBuilder::new_document();
+    let parse_result = Html5Parser::parse_document(&mut chars, Document::clone(&doc), None);


I've noticed here, we maybe should not return a Result with a Vec<ParseError> as the Ok type. Maybe i missed something here?

Not sure, @jaytaph would have more insight to that, I am just a consumer of the parser :P

Sharktheone · 2023-11-25T14:13:18Z

gosub-bindings/src/lib.rs

+/// This takes ownership of the pointer from the C API and transfers it to Rust so it can
+/// be deallocated.
+pub unsafe extern "C" fn gosub_render_tree_iterator_free(tree_iterator: *mut TreeIterator) {
+    let _ = Box::from_raw(tree_iterator);


Have you tested using drop_in_place here? At leased you didn't wrote something in Zulip.

I did but it segfaulted. There's something odd about the document, I noted it down for myself for future research but could not figure it out yet.

I might move away from DocumentHandle and try Box-ing the Document itself to see if that would work

src/render_tree.rs

src/render_tree/text.rs

Kiyoshika · 2023-11-25T15:05:03Z

Converting to draft for the following changes:

Introduce different struct to avoid raw pointers and unsafe code in the engine (and use String again). Convert to this new struct in the bindings that will convert the String to a raw pointer and transfer ownership to the C API
Change f32 back to f64 and update C API to use double instead of float
move the #[no_mangle] tags down to the actual function instead of above the comments

TODO in the future (not in this PR, can create issues/tasks for these)

switch to @jaytaph's DNS system
investigate the memory leak caused by DocumentHandle, consider moving to different pointer type and using Document directly

Sharktheone

Some minor things, but at most of them feel free to ignore.
Note that i cannot say too much about the C side.

gosub-bindings/src/gosub_api.c

gosub-bindings/src/lib.rs

src/render_tree.rs

Sharktheone · 2023-11-25T19:49:17Z

src/render_tree.rs

@@ -330,107 +349,4 @@ mod tests {
            panic!()
        }
    }
-
-    #[test]
-    fn text_nodes() {


Not sure if you want to have this tests only over the C API

Kiyoshika · 2023-11-26T01:24:31Z

@Sharktheone added your suggestions and also switched the render_tree_init to use a stack-allocated struct instead of allocating one on the heap

Sharktheone

LGTM, just a minor thing, feel free to ignore.

Sharktheone · 2023-11-26T01:31:48Z

gosub-bindings/include/gosub_api.h

+/// Initialize a render tree by passing a stack-allocated
+/// struct by address.
+void render_tree_init(struct render_tree_t *render_tree, const char *html);


I don't know too much about this, but shouldn't it be possible to create the render_tree_t in the function? I've seen this approach in many different C Libraries, so it's probably the best solution, when we want to have it on the stack.

You can't return a pointer to something stack-allocated in a function, C doesn't transfer ownership. If you tried dereferencing it, you would segfault

Unless they are doing something that I've never heard of before... I feel like that may be possible in C++ with its move semantics, but I haven't really seen anything like that in C

Unless you mean returning the struct by value, I guess I didn't really think of that, but not something I do often since it seems kind of weird to copy the value around (even tho the compiler might do some optimizations on return values)

Yes, after some thought, just returning the struct by value might be fine in this case, it's very small anyways

ah, okay. I don't have much C experience. I mostly used C++ and not raw C.

If you look at my personal github, I came from mostly raw C... but usually the structs I work with are relatively large so returning by value didn't come across my mind since I am so used to returning pointers

Yes, after some thought, just returning the struct by value might be fine in this case, it's very small anyways

I guess, you can just leave it the way it currently is, but if you want to change it just do it. I don't want to make you too much work. 🙂

I think I will leave it as a parameter and return the int to check if a failure occurred. Then I think this will be ready..

gosub-bindings/src/gosub_api.c

Sharktheone

LGTM

emwalker

I'm fully out of my depth. I have only dabbled in C over the years, so I have no way to approve this PR beyond the impression that you and @Sharktheone seem to know C a lot more than I do. Godspeed.

emwalker · 2023-11-26T02:56:10Z

gosub-bindings/Makefile

+	./$(TEST_SRC_DIR)/render_tree_test
+
+format:
+	cargo clippy --fix --allow-dirty --allow-staged


I am having trouble building the bindings and test targets on my Ubuntu laptop. Is there some setup that I need to do to get started?

You should only have to cd gosub-bindings, cargo build then run the make files

Ah, I see. Yes, that works.

I see that that leaves a target directory under gosub-bindings. It seems that we're now getting into the realm of a Cargo workspace, which is what I'm thinking we should switch to at some point soon if you want to keep this crate separate from the main one in this repo. (Workspaces allow you to publish a lot of crates under a single repo.)

I also thought about this a while ago, but we only had one crate at that time.

yeah, I've been thinking of if it would be beneficial to move this out of the engine (which currently lists it as a dependency if you look at the .toml) but I wasn't sure what to do.

From the user agent perspective, it would only need to build the bindings (which would build the engine anyways since it's a dependency)

Might be as easy as this directory structure:

gosub-engine/

engine/

Cargo.toml

bindings/

Cargo.toml

Cargo.toml (for all the crates)

$ cat Cargo.toml [workspace] members = [ "engine", "bindings", ]

emwalker · 2023-11-26T03:08:18Z

gosub-bindings/src/lib.rs

+///
+/// Moves an owning pointer to the render_tree using Box::into_raw() to the C API.
+/// This pointer MUST be passed to gosub_render_tree_free() after usage for proper cleanup.
+pub unsafe extern "C" fn gosub_render_tree_init(html: *const c_char) -> *mut RenderTree {


Are we guaranteed that the Swift / Objective C side will never free this *const c_char pointer while we're in Rust code?

Kiyoshika · 2023-11-26T03:19:30Z

@emwalker for some reason I can't reply to your comment

Are we guaranteed that the Swift Objective C side will never free this *const c_char pointer while we're in Rust code?

When this function is called from swift and the pointer is passed, only the Rust code (technically the C wrapper) will be doing work until after the scope closes and Swift continues, so we wouldn't be able to accidentially free this while it's being used by Rust/C

Kiyoshika · 2023-11-26T03:23:18Z

Relevant swift portions if you're curious:

Calling render tree constructor (swift wrapper) - https://github.com/gosub-browser/gosub-client-macos/blob/110d52daaebe09154b2e422cbf2f63d33be51d9d/gosub-client-macos/ContentView.swift#L18C14-L18C14

The internals that call the C code - https://github.com/gosub-browser/gosub-client-macos/blob/110d52daaebe09154b2e422cbf2f63d33be51d9d/gosub-client-macos/RenderTree.swift#L8C14-L8C14

Sharktheone · 2023-11-26T03:31:00Z

I'm fully out of my depth. I have only dabbled in C over the years, so I have no way to approve this PR beyond the impression that you and @Sharktheone seem to know C a lot more than I do. Godspeed.

I also don't have too much knowledge about C. My main understanding comes from C++ and only minimal C. But I've read some libraries that are coded in C and have a bit of understanding from C because of that, but also not too much.

Kiyoshika · 2023-11-26T03:35:58Z

I've written quite a lot of C (as that makes up almost my entire github history) but none of it was professional and almost all of it was entirely solo that I've learned over time. But I've never really used it in the context of interacting with other languages (except one case with Python and ctypes for a numpy clone I wrote forever ago)

emwalker · 2023-11-26T03:42:39Z

This talk by a Mozilla developer that wrote a Rust crate that is used in Firefox goes into some challenges he ran into with dropping pointers in a C++/Rust FFI integration that vaguely sound a little like the problem you were running into:

https://media.ccc.de/v/rustfest18-5-a_rust_crate_that_also_quacks_like_a_modern_c_library#t=1480

Might be a red herring.

Kiyoshika · 2023-11-26T03:46:45Z

My next step is to go down this rabbit hole and try to figure out how to clean up the leak. I think using Rc<> will present a lot of challenges with cyclic reference counts that can lead to leaks, but not sure how that's happening in the specific case of the render tree

Kiyoshika commented Nov 25, 2023

View reviewed changes

src/render_tree.rs Outdated Show resolved Hide resolved

Kiyoshika commented Nov 25, 2023

View reviewed changes

src/render_tree.rs Outdated Show resolved Hide resolved

Kiyoshika commented Nov 25, 2023

View reviewed changes

src/render_tree/text.rs Outdated Show resolved Hide resolved

Kiyoshika commented Nov 25, 2023

View reviewed changes

src/render_tree/text.rs Outdated Show resolved Hide resolved

Kiyoshika requested a review from a team November 25, 2023 01:39

Sharktheone requested changes Nov 25, 2023

View reviewed changes

Kiyoshika marked this pull request as draft November 25, 2023 15:00

Kiyoshika force-pushed the feat/ffi-bindings branch from ac897dd to 2930625 Compare November 25, 2023 18:01

introduce C API

a20016a

Kiyoshika force-pushed the feat/ffi-bindings branch from aae1ac0 to a20016a Compare November 25, 2023 18:11

Kiyoshika marked this pull request as ready for review November 25, 2023 18:13

Merge branch 'main' into feat/ffi-bindings

cc793ab

Kiyoshika requested a review from a team November 25, 2023 18:20

Sharktheone requested changes Nov 25, 2023

View reviewed changes

Kiyoshika requested a review from Sharktheone November 26, 2023 01:22

Sharktheone approved these changes Nov 26, 2023

View reviewed changes

add suggestions from code review

765e25c

Kiyoshika force-pushed the feat/ffi-bindings branch from 468c81d to 765e25c Compare November 26, 2023 02:14

Sharktheone approved these changes Nov 26, 2023

View reviewed changes

emwalker approved these changes Nov 26, 2023

View reviewed changes

Kiyoshika merged commit c81302c into main Nov 26, 2023
4 checks passed

Kiyoshika deleted the feat/ffi-bindings branch November 26, 2023 03:41

Introducing the C API #279

Introducing the C API #279

Conversation

Kiyoshika commented Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiyoshika commented Nov 25, 2023 • edited Loading

Sharktheone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiyoshika commented Nov 26, 2023

Sharktheone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone left a comment

Choose a reason for hiding this comment

emwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiyoshika Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Kiyoshika commented Nov 26, 2023

Kiyoshika commented Nov 26, 2023

Sharktheone commented Nov 26, 2023

Kiyoshika commented Nov 26, 2023

emwalker commented Nov 26, 2023 • edited Loading

Kiyoshika commented Nov 26, 2023

Kiyoshika commented Nov 25, 2023 •

edited

Loading

emwalker Nov 26, 2023 •

edited

Loading

Kiyoshika commented Nov 25, 2023 •

edited

Loading

Sharktheone Nov 26, 2023 •

edited

Loading

emwalker Nov 26, 2023 •

edited

Loading

Kiyoshika Nov 26, 2023 •

edited

Loading

emwalker Nov 26, 2023 •

edited

Loading

emwalker commented Nov 26, 2023 •

edited

Loading