Skip to content
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

Remove whitelisting of functions #2

Merged
merged 1 commit into from
Dec 27, 2019
Merged

Remove whitelisting of functions #2

merged 1 commit into from
Dec 27, 2019

Conversation

NilsIrl
Copy link
Contributor

@NilsIrl NilsIrl commented Dec 26, 2019

This decreases build time and prevents functions that should be
whitelisted to not be.

For example, when working on my fork of the leptess API, I didn't have access to the file format enums.

And anyway, <leptonica/allheaders.h> is the api. There is no reason some whitelisting should be done on top of it.

Also increase bindgen version.

This increases build speed and prevents functions that should be
whitelisted to not be.

Also increase bindgen version.
@ccouzens
Copy link
Owner

Thank you @NilsIrl.

I'm getting a warning when running the tests:

warning: `extern` block uses type `u128`, which is not FFI-safe
    --> /home/chris/Documents/git/github.com/ccouzens/leptonica-sys/target/debug/build/leptonica-sys-3634b718d278f931/out/bindings.rs:1448:10
     |
1448 |     ) -> u128;
     |          ^^^^ not FFI-safe
     |
     = note: `#[warn(improper_ctypes)]` on by default
     = note: 128-bit integers don't currently have a known stable ABI

Is it possible to prevent this without hiding it? I'll investigate too.

I think the reason I didn't do it this way initially is because I was worried about it including functions from the c-standard library like printf. But this doesn't seem to be a problem.

@ccouzens
Copy link
Owner

The functions generating these warnings are:

// part of <stdlib.h> 
extern "C" {
    pub fn strtold(
        __nptr: *const ::std::os::raw::c_char,
        __endptr: *mut *mut ::std::os::raw::c_char,
    ) -> u128;
}

// part of <stdlib.h> 
extern "C" {
    pub fn qecvt(
        __value: u128,
        __ndigit: ::std::os::raw::c_int,
        __decpt: *mut ::std::os::raw::c_int,
        __sign: *mut ::std::os::raw::c_int,
    ) -> *mut ::std::os::raw::c_char;
}

// part of <stdlib.h> 
extern "C" {
    pub fn qfcvt(
        __value: u128,
        __ndigit: ::std::os::raw::c_int,
        __decpt: *mut ::std::os::raw::c_int,
        __sign: *mut ::std::os::raw::c_int,
    ) -> *mut ::std::os::raw::c_char;
}

// part of <stdlib.h> 
extern "C" {
    pub fn qgcvt(
        __value: u128,
        __ndigit: ::std::os::raw::c_int,
        __buf: *mut ::std::os::raw::c_char,
    ) -> *mut ::std::os::raw::c_char;
}

// part of <stdlib.h> 
extern "C" {
    pub fn qecvt_r(
        __value: u128,
        __ndigit: ::std::os::raw::c_int,
        __decpt: *mut ::std::os::raw::c_int,
        __sign: *mut ::std::os::raw::c_int,
        __buf: *mut ::std::os::raw::c_char,
        __len: usize,
    ) -> ::std::os::raw::c_int;
}

// part of <stdlib.h> 
extern "C" {
    pub fn qfcvt_r(
        __value: u128,
        __ndigit: ::std::os::raw::c_int,
        __decpt: *mut ::std::os::raw::c_int,
        __sign: *mut ::std::os::raw::c_int,
        __buf: *mut ::std::os::raw::c_char,
        __len: usize,
    ) -> ::std::os::raw::c_int;
}

I'll see if I can exclude stdlib and any other libraries.

@NilsIrl
Copy link
Contributor Author

NilsIrl commented Dec 27, 2019

This seems to be a known issue

rust-lang/rust-bindgen#1549

@NilsIrl
Copy link
Contributor Author

NilsIrl commented Dec 27, 2019

I think the reason I didn't do it this way initially is because I was worried about it including functions from the c-standard library like printf.

These functions are indeed included

It that's really a problem. These could be removed

@ccouzens
Copy link
Owner

It that's really a problem. These could be removed

Changing wrapper.h to be

#define _STDLIB_H       1

#include <leptonica/allheaders.h>

Seems to exclude stdlib.h. The warning goes away, and the tests still pass

@ccouzens
Copy link
Owner

Actually, I'll merge it as is. The warnings don't show for dependent crates.

@ccouzens ccouzens merged commit c9ff2fe into ccouzens:master Dec 27, 2019
ccouzens added a commit that referenced this pull request Dec 27, 2019
This defines more values for @NilsIrl

#2

> For example, when working on my fork of the leptess API, I didn't have access to the file format enums.
@ccouzens
Copy link
Owner

I've pushed this as version 0.3.0.

I'm currently updating tesseract-sys and tesseract-rs to use it.

@ccouzens
Copy link
Owner

I didn't have access to the file format enums.

@NilsIrl, can you let me know one of the enums you need? I'll add a test so to make sure it doesn't get removed in future

ccouzens added a commit to antimatter15/tesseract-rs that referenced this pull request Dec 27, 2019
@ccouzens ccouzens mentioned this pull request Dec 27, 2019
@NilsIrl NilsIrl deleted the no_whitelist branch December 27, 2019 18:26
ccouzens added a commit that referenced this pull request Dec 27, 2019
NilsIrl requires the enums from
https://github.com/DanBloomberg/leptonica/blob/50c3c353f75b0a5c97b645b3c050dab66243ee20/src/imageio.h#L91-L112

I've added tests to make sure they don't get removed in the future.

Ref:
#2 (comment)
ccouzens added a commit to ccouzens/tesseract-sys that referenced this pull request Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants