use bindgen to auto generate ffi rust code #398

fredchenbj · 2019-12-03T04:01:34Z

refer to #381.

just use old c.h and bindgen to gen ffi's rust code. and fix to pass cargo build.

this is step 1, later replace with rocksdb's c.h to gen code.

Signed-off-by: fredchenbj [email protected]

Connor1996 · 2019-12-04T02:41:40Z

Please fix the conflicts

fredchenbj · 2019-12-04T02:46:04Z

Please fix the conflicts

@Connor1996 I have fixed the conficts.

Connor1996 · 2019-12-04T02:50:20Z

librocksdb_sys/crocksdb/crocksdb/c.h

@@ -940,10 +943,10 @@ extern C_ROCKSDB_LIBRARY_API void crocksdb_options_enable_statistics(
    crocksdb_options_t*, unsigned char);
 extern C_ROCKSDB_LIBRARY_API void crocksdb_options_reset_statistics(
    crocksdb_options_t*);
-extern C_ROCKSDB_LIBRARY_API bool crocksdb_load_latest_options(
+extern C_ROCKSDB_LIBRARY_API unsigned char crocksdb_load_latest_options(


bindgen generates FFI based on c.h, but why we modify c.h?

@Connor1996 this function was added by us, so not conform to c's style.

rocksdb's c.h has convention:
Bools have the type unsigned char (0 == false; rest == true)

can you extract them to a separate PR?

step 1 finished. Signed-off-by: fredchenbj <[email protected]>

Signed-off-by: fredchenbj <[email protected]>

yiwu-arbug · 2019-12-04T04:28:23Z

@fredchenbj awesome changes! Some general questions:

Can you summarize what's changed in this PR? Seems mostly typing changes. Why are they needed?
The PR seems not including the generated code. Should we checkin the generated code or not?
What's the overall plan for migration?

Thanks.

BusyJay · 2019-12-04T05:54:08Z

It introduces clang dependency, which can be a pain to fix. You can learn how to avoid it from tikv/grpc-rs.

Connor1996 · 2019-12-04T06:14:55Z

@BusyJay You mean pre-generate binding? Yeah, that's what we plan to do. For easy reviewing, we plan to do it step by step.

Connor1996 · 2019-12-04T06:51:40Z

src/table_properties_collector.rs

@@ -26,7 +28,7 @@ use std::slice;
 /// TablePropertiesCollector object per table and then call it sequentially
 pub trait TablePropertiesCollector {
    /// Will be called when a new key/value pair is inserted into the table.
-    fn add(&mut self, key: &[u8], value: &[u8], entry_type: DBEntryType, seq: u64, file_size: u64);
+    fn add(&mut self, key: &[i8], value: &[i8], entry_type: DBEntryType, seq: u64, file_size: u64);


why changing u8 to i8?

for const char* in c.h is transferred to *const libc::c_char by bindgen, which requires i8. All transfer like this is for that. @Connor1996

see references:

rust std implementation https://doc.rust-lang.org/src/std/ffi/c_str.rs.html#975,

https://doc.rust-lang.org/std/os/raw/type.c_char.html,

https://stackoverflow.com/questions/47684111/make-string-type-compatible-with-arm/47684200.

i8 or u8 is only due to how C char represents in different platforms. But the data is always one-byte long. As str in Rust is represented as &[u8], so we must convert it to u8.

got it. I would fix it.

Connor1996 · 2019-12-04T07:31:32Z

src/event_listener.rs


 impl WriteStallInfo {
    pub fn cf_name(&self) -> &str {
        unsafe { fetch_str!(crocksdb_writestallinfo_cf_name(&self.0)) }
    }
-    pub fn cur(&self) -> WriteStallCondition {
+    pub fn cur(&self) -> crocksdb_writestallcondition_t {


All public functions/traits of src/xxx.rs should be unchanged, otherwise TiKV or other users may change code dramatically. Consider rename type crocksdb_writestallcondition_t to WriteStallCondition. Same for other enums like DBEntryType.

fredchenbj · 2019-12-04T09:23:20Z

@fredchenbj awesome changes! Some general questions:

Can you summarize what's changed in this PR? Seems mostly typing changes. Why are they needed?

The PR seems not including the generated code. Should we checkin the generated code or not?

What's the overall plan for migration?

Thanks.

@yiwu-arbug Thanks for your questions.

for 3, the migration plan has three steps: 1) use the old crocksdb's c.h and bindgen to generate ffi rust code, and make it work for all dependency code; 2) replace with rocksdb's c.h, and make all code in c.h conform to rocksdb's style, here we would only include rocksdb's c.h and not change it at all; 3) try to use rocksdb cpp api to generate ffi rust code. This pr is only step 1.

for 1, this pr has mainly three changes: 1) change crocksdb's c.h and c.cc to conform to bindgen's requirement; 2) use bindgen to generate ffi rust code; 3) change code in rust-rocksdb, including many type changes, to make those code could call the code geneated by bindgen.

for 2, the generated code is in target/debug/build/librocksdb_sys-f21a7cfdab79a481/out/bindings.rs, which is auto generated by cargo build. IMHO, we don't need checkin it.

yiwu-arbug · 2019-12-06T21:38:18Z

@fredchenbj for 3, can you give more details for step 1)? Does the generated code provide difference interface than the existing rust API? If so, how much work is it to update tikv? If that work is huge, is there way to split the work into small bits?

Also for 3, if we will trying generating code from c++ directly, how useful step 2) would be?

Currently the wrapper doesn't handle Status return from rocksdb well. It convert the struct to string and miss the typing. I hope with step 3) we can change it to convert to rust Result.

fredchenbj changed the title ~~[DNM] use bindgen to auto generate ffi rs code~~ [DNM] use bindgen to auto generate ffi rust code Dec 3, 2019

Connor1996 changed the title ~~[DNM] use bindgen to auto generate ffi rust code~~ use bindgen to auto generate ffi rust code Dec 4, 2019

Connor1996 reviewed Dec 4, 2019

View reviewed changes

fredbjer added 2 commits December 4, 2019 11:32

use old c.h and bindgen to gen ffi rs code

942426c

step 1 finished. Signed-off-by: fredchenbj <[email protected]>

fix conflicts

7f4b70e

Signed-off-by: fredchenbj <[email protected]>

fredchenbj force-pushed the fredchenbj/bindgen_ffi branch from cc31d36 to 7f4b70e Compare December 4, 2019 03:54

Connor1996 reviewed Dec 4, 2019

View reviewed changes

This was referenced Dec 9, 2019

change c.h and c.cc for bindgen ffi #401

Merged

Use bindgen to auto generate FFI #381

Open

Merge branch 'master' into fredchenbj/bindgen_ffi

d8239de

fredchenbj mentioned this pull request Dec 11, 2019

bindgen: pre-generate bindings #402

Merged

fredchenbj mentioned this pull request Jan 7, 2020

replace manual ffi code with bindgen generated code #418

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use bindgen to auto generate ffi rust code #398

use bindgen to auto generate ffi rust code #398

fredchenbj commented Dec 3, 2019 •

edited

Loading

Connor1996 commented Dec 4, 2019

fredchenbj commented Dec 4, 2019 •

edited

Loading

Connor1996 Dec 4, 2019

fredchenbj Dec 4, 2019

fredchenbj Dec 4, 2019

Connor1996 Dec 4, 2019

Connor1996 Dec 4, 2019

fredchenbj Dec 4, 2019

yiwu-arbug commented Dec 4, 2019

BusyJay commented Dec 4, 2019

Connor1996 commented Dec 4, 2019

Connor1996 Dec 4, 2019

fredchenbj Dec 4, 2019 •

edited

Loading

Connor1996 Dec 4, 2019 •

edited

Loading

fredchenbj Dec 4, 2019

Connor1996 Dec 4, 2019 •

edited

Loading

fredchenbj Dec 4, 2019

fredchenbj commented Dec 4, 2019 •

edited

Loading

yiwu-arbug commented Dec 6, 2019

use bindgen to auto generate ffi rust code #398

Are you sure you want to change the base?

use bindgen to auto generate ffi rust code #398

Conversation

fredchenbj commented Dec 3, 2019 • edited Loading

Connor1996 commented Dec 4, 2019

fredchenbj commented Dec 4, 2019 • 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

Choose a reason for hiding this comment

yiwu-arbug commented Dec 4, 2019

BusyJay commented Dec 4, 2019

Connor1996 commented Dec 4, 2019

Choose a reason for hiding this comment

fredchenbj Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Connor1996 Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredchenbj commented Dec 4, 2019 • edited Loading

yiwu-arbug commented Dec 6, 2019

fredchenbj commented Dec 3, 2019 •

edited

Loading

fredchenbj commented Dec 4, 2019 •

edited

Loading

fredchenbj Dec 4, 2019 •

edited

Loading

Connor1996 Dec 4, 2019 •

edited

Loading

Connor1996 Dec 4, 2019 •

edited

Loading

fredchenbj commented Dec 4, 2019 •

edited

Loading