C/C++ Bindings #618

TheBlueMatt · 2020-05-11T00:09:52Z

This is a super rough draft first pass at generating C bindings. The code to do so is horrendous, but the resulting C bindings are not completely insane. Thanks to @arik-so for initial passes at cbindgen and helping work through what could work and what downstream clients wont be able to use. Things left to do:

Map Result and Option things
Map Vec things
Map enums with contained values into C union + disciminant
Get the trait jump table for concrete structs (allowing us to use eg ChannelManager as ChannelMessageHandler)
_free functions to release resources
Trait destructors
(de)serialization
Document the memory management model
Carefully walk the generated code to check memory model.

codecov · 2020-05-11T05:24:32Z

Codecov Report

Merging #618 into master will decrease coverage by 16.53%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master     #618       +/-   ##
===========================================
- Coverage   91.38%   74.84%   -16.54%     
===========================================
  Files          35       55       +20     
  Lines       21727    24437     +2710     
===========================================
- Hits        19855    18290     -1565     
- Misses       1872     6147     +4275

Impacted Files	Coverage Δ
lightning-c-bindings/src/bitcoin/network.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/c_types/derived.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/c_types/mod.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/chain/chaininterface.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/chain/keysinterface.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/chain/transaction.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/lib.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/ln/chan_utils.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/ln/channelmanager.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/ln/channelmonitor.rs	`0.00% <0.00%> (ø)`
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dd8b3e...0ab7f90. Read the comment docs.

jkczyz

No need to address these right away. I'm going to keep to send comments out in small bursts as I read through the code. Hopefully a few more rounds today.

jkczyz · 2020-05-13T18:50:07Z

c-bindings-gen/src/types.rs

+					// Really not sure where syn gets '=' from here -
+					// it somehow represents '///' or '//!'


I think it is because /// ... and //! .. are syntactic sugar for [doc = " ..."].

https://doc.rust-lang.org/rustdoc/the-doc-attribute.html

Huh. Funny it even parses them that way.

jkczyz · 2020-05-13T18:50:41Z

c-bindings-gen/src/types.rs

+		match token_iter.next().unwrap() {
+			TokenTree::Literal(lit) => {
+				let line = format!("{}", lit);
+				if line.contains("(C-not exported)") {


Instead of muddying up our docs, could we define an attribute instead?

Hmm. We could change the line, but I actually kinda like putting it in the docs - it lets us link people to the rustdoc output even if they're using the library via C.

I'm not sure I follow the part about linking docs.

What I meant was we should define attributes that we use to annotate rust-lightning. Then this code checks for that attribute rather than trying to parse Rust docs.

Right, the use of docs serves a dual purpose - both to allow us to make decisions at codegen time, and to actually document the fact that something isn't exported. Currently I've been sending people to the docs.rs docs for C bindings as well, though we could try to change that.

jkczyz · 2020-05-13T19:00:16Z

c-bindings-gen/src/types.rs

+	for attr in attrs.iter() {
+		let tokens_clone = attr.tokens.clone();
+		let mut token_iter = tokens_clone.into_iter();
+		if let Some(token) = token_iter.next() {


This may be clearer if the path was examined first before iterating over the tokens. Especially given we can represent NoExport and Rename using attributes rather than with special comments, thus examining the tokens is not always necessary.

c-bindings-gen/src/types.rs

c-bindings-gen/src/main.rs

c-bindings-gen/src/types.rs

c-bindings-gen/src/main.rs

jkczyz

Would you mind providing me a brain dump of the following?

High-level motivation for the transformation code
Each transformation and rationale for it

This would help me get a better understanding of the design decisions and allow me to provide more useful feedback. I'm happy to take that and make a README markdown which we can evolve along the way. I think we'll want something like that for posterity anyhow.

Otherwise, I'm not sure I'm making good use of my time understanding and trying to discern the rationale behind the generated code. Staring at it is kinda frying my brain honestly. :P

jkczyz · 2020-05-14T21:44:55Z

lightning-c-bindings/src/chain/chaininterface.rs

+
+use lightning::chain::chaininterface::FeeEstimator as lnFeeEstimator;
+impl lnFeeEstimator for FeeEstimator {
+	fn get_est_sat_per_1000_weight(&self, confirmation_target: lightning::chain::chaininterface::ConfirmationTarget) -> u64 {


For parameter types, you may be able to write this as lnConfirmationTarget since the use statement for it is given earlier.

Or as that only possible because ConfirmationTarget is defined in this file?

I think in this case its because its defined in this file. There are a few cases where it isnt. While maybe we could add imports, its probably not worth the effort.

jkczyz · 2020-05-14T22:41:31Z

lightning-c-bindings/src/c_types/mod.rs

+use bitcoin::secp256k1::key::PublicKey as SecpPublicKey;
+use bitcoin::secp256k1::key::SecretKey as SecpSecretKey;


Is there any way we can take advantage of the fact that these are Rust bindings for C code?

I think in theory, yea, they wrap underlying ffi/C types explicitly and you can use those, though that then assumes the caller is in C and/or using libsecp under the hood. I presume that this would be annoying to deal with if you were not, and byte arrays are universal. That said, it does result in an extra copy or two, including for private keys, which kinda sucks.

jkczyz · 2020-05-14T23:25:47Z

lightning-c-bindings/src/ln/msgs.rs

+use lightning::ln::msgs::ChannelMessageHandler as lnChannelMessageHandler;
+impl lnChannelMessageHandler for ChannelMessageHandler {
+	fn handle_open_channel(&self, their_node_id: &bitcoin::secp256k1::key::PublicKey, their_features: lightning::ln::features::InitFeatures, msg: &lightning::ln::msgs::OpenChannel) {
+		(self.handle_open_channel)(self.this_arg, crate::c_types::PublicKey::from_rust(their_node_id), crate::ln::features::InitFeatures { inner: Box::into_raw(Box::new(their_features)) }, &OpenChannel { inner: msg })


Why do types like PublicKey need to be serialized while types like InitFeatures do not?

Fundamentally because there's a C equivalent (for a public key namely a 33 byte string). We could just serialize out InitFeatures into C (though its obviously more complicated since its not fixed-length), but then we'd lose things like "does_support_feature_X" (well, we dont have those now cause we cant run the macro to figure out the generated code, but at least for other types this holds).

valentinewallace

Other than these last few nits, looks good!

valentinewallace · 2020-09-04T16:55:16Z

lightning-c-bindings/demo.c

+
+	LDKEventsProvider prov = ChannelManager_as_EventsProvider(&cm);
+	LDKCVec_EventZ events = (prov.get_and_clear_pending_events)(prov.this_arg);
+	assert((unsigned long)events.data < 4096); // There's an offset, but it should still be an offset against null in the 0 page


Wondering, what's the purpose of this check? (and it's casting events.data as a pointer, right?)

It checks that there was no allocation/malloc done for the data.

lightning-c-bindings/README.md

TheBlueMatt · 2020-09-10T22:27:01Z

I think this is ready to hit merge!

valentinewallace · 2020-09-10T22:41:58Z

I think this is ready to hit merge!

You probably saw this, but the check_bindings failure looks legit

TheBlueMatt · 2020-09-10T23:04:58Z

Oops, I'd accidentally dropped one commit in rebase, fixed now.

This cleans upa few last cases of functions/objects which our C bindings generator doesn't know how to read.

In general, it maps: * Traits to a struct with a void* and a list of function pointers, emulating what the compiler will do for a dyn trait anyway, * Structs as a struct with a single opaque pointer to the underlying type and a flag to indicate ownership. While this is a bit less effecient than just a direct pointer, it neatly lets us expose in the public interface the concept of ownership by setting a flag in the generated struct. * Unit enums as enums with each type copied over and conversion functions, * Non-unit enums have each field converted back and forth with a type flag and a union across all the C-mapped fields.

Including: * A script to automatically generate all the rest, * Cargo.toml and cbindgen.toml, * manually-written wrapper types for a few types

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch 3 times, most recently from e80f27a to 5daa4b0 Compare May 11, 2020 01:40

joemphilips mentioned this pull request May 11, 2020

WIP: introduce wrapper package for C# FFI. #569

Closed

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch 10 times, most recently from 92f61cf to f9c969d Compare May 11, 2020 05:13

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch 5 times, most recently from 91b5eb4 to 0935d67 Compare May 12, 2020 17:10

TheBlueMatt mentioned this pull request May 12, 2020

Pre-C Bindings Cleanup #620

Merged

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch 2 times, most recently from 6373e67 to b370eda Compare May 12, 2020 19:37

jkczyz self-requested a review May 12, 2020 22:04

jkczyz reviewed May 13, 2020

View reviewed changes

jkczyz reviewed May 14, 2020

View reviewed changes

c-bindings-gen/src/types.rs Outdated Show resolved Hide resolved

jkczyz reviewed May 14, 2020

View reviewed changes

c-bindings-gen/src/main.rs Outdated Show resolved Hide resolved

jkczyz reviewed May 15, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch from bfa9148 to 3c85f1a Compare May 16, 2020 02:31

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch 6 times, most recently from e654d7e to cdfc9d1 Compare September 8, 2020 23:38

valentinewallace reviewed Sep 9, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch from cdfc9d1 to bb3d6aa Compare September 10, 2020 15:51

TheBlueMatt mentioned this pull request Sep 10, 2020

Bump rust-bitcoin and friends #686

Merged

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch from bb3d6aa to 9be38b9 Compare September 10, 2020 22:26

TheBlueMatt added 12 commits September 10, 2020 21:58

Remove a few final ::-containing paths in generic args

bd5f242

This cleans upa few last cases of functions/objects which our C bindings generator doesn't know how to read.

Add annotations for things which we cannot (yet) expose

5254d6b

Add all the manually-generated bits for the c-bindings crate

1af8a46

Including: * A script to automatically generate all the rest, * Cargo.toml and cbindgen.toml, * manually-written wrapper types for a few types

Add automatically generated C/C++ wrapper bindings

96b2927

Update the root Cargo.toml to point to the new subcrate

76eaa61

LTO in release mode by default, which is really critical for C libs

af9d1cf

Add CI run for bindings generation (though we'll allow it to fail)

9f0fa04

Add bindings README

4f6f34f

Apply -O1 only to tests, not all dev (which is used for bindings)

9e03087

Specify panic = abort in profiles for lightning-c-bindings

3c12b0d

Use Debian experimental in CI to match LLVM between rustc and clang

0ab7f90

TheBlueMatt force-pushed the 2020-05-sample-c-bindings branch from af00220 to 0ab7f90 Compare September 11, 2020 02:03

TheBlueMatt merged commit 253af8d into lightningdevkit:master Sep 11, 2020

jkczyz mentioned this pull request Sep 11, 2020

Failure generating documentation with multiple lightning libraries #689

Closed

This was referenced Jun 23, 2021

Standalone C++ bindings lightningdevkit/ldk-c-bindings#32

Open

Broader C bindings demo client lightningdevkit/ldk-c-bindings#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C/C++ Bindings #618

C/C++ Bindings #618

TheBlueMatt commented May 11, 2020 •

edited

Loading

codecov bot commented May 11, 2020 •

edited

Loading

jkczyz left a comment

jkczyz May 13, 2020

TheBlueMatt May 16, 2020

jkczyz May 13, 2020

TheBlueMatt May 16, 2020

jkczyz May 19, 2020

TheBlueMatt May 24, 2020

jkczyz May 13, 2020

jkczyz left a comment

jkczyz May 14, 2020

TheBlueMatt May 22, 2020

jkczyz May 14, 2020

TheBlueMatt May 22, 2020

jkczyz May 14, 2020

TheBlueMatt May 22, 2020

valentinewallace left a comment

valentinewallace Sep 4, 2020

TheBlueMatt Sep 9, 2020

TheBlueMatt commented Sep 10, 2020

valentinewallace commented Sep 10, 2020

TheBlueMatt commented Sep 10, 2020

		// Really not sure where syn gets '=' from here -
		// it somehow represents '///' or '//!'

		use bitcoin::secp256k1::key::PublicKey as SecpPublicKey;
		use bitcoin::secp256k1::key::SecretKey as SecpSecretKey;

C/C++ Bindings #618

C/C++ Bindings #618

Conversation

TheBlueMatt commented May 11, 2020 • edited Loading

codecov bot commented May 11, 2020 • edited Loading

Codecov Report

jkczyz 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

jkczyz 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

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 10, 2020

valentinewallace commented Sep 10, 2020

TheBlueMatt commented Sep 10, 2020

TheBlueMatt commented May 11, 2020 •

edited

Loading

codecov bot commented May 11, 2020 •

edited

Loading